From 9bd3a648865879a73d1c9d652ac2a021d2c5b87c Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Thu, 5 Dec 2024 19:27:17 +0100 Subject: [PATCH] cachi2: allow only relative paths For security reasons, only relative paths within cloned remote source can be specified by users Don't allow to point to symlink pointing out of cloned remote source Signed-off-by: Martin Basti --- atomic_reactor/plugins/cachi2_init.py | 4 +- atomic_reactor/utils/cachi2.py | 44 +++++++++++++ tests/plugins/test_cachi2_init.py | 28 ++++++++ tests/utils/test_cachi2.py | 95 +++++++++++++++++++++++++++ 4 files changed, 170 insertions(+), 1 deletion(-) diff --git a/atomic_reactor/plugins/cachi2_init.py b/atomic_reactor/plugins/cachi2_init.py index 485c8c23b..ad09145ba 100644 --- a/atomic_reactor/plugins/cachi2_init.py +++ b/atomic_reactor/plugins/cachi2_init.py @@ -25,7 +25,7 @@ ) from atomic_reactor.plugin import Plugin from atomic_reactor.util import map_to_user_params -from atomic_reactor.utils.cachi2 import remote_source_to_cachi2, clone_only +from atomic_reactor.utils.cachi2 import remote_source_to_cachi2, clone_only, validate_paths class Cachi2InitPlugin(Plugin): @@ -129,6 +129,8 @@ def process_remote_sources(self) -> List[Dict[str, Any]]: remote_source_data["ref"] ) + validate_paths(source_path_app, remote_source_data.get("packages", {})) + if clone_only(remote_source_data): # OSBS is doing all work here self.write_metadata(source_path) diff --git a/atomic_reactor/utils/cachi2.py b/atomic_reactor/utils/cachi2.py index c2c7d943b..fe11a791c 100644 --- a/atomic_reactor/utils/cachi2.py +++ b/atomic_reactor/utils/cachi2.py @@ -9,10 +9,54 @@ """ from typing import Any, Callable, Dict, Optional, Tuple, List +from pathlib import Path +import os.path from packageurl import PackageURL +def validate_paths(repo_path: Path, remote_sources_packages: dict) -> None: + """Paths must be relative and within cloned repo""" + def is_path_ok(path_string): + path = Path(path_string) + if path.is_absolute(): + return False + + # using real repo to properly resolve and block bad symlinks + full_path = (repo_path/path).resolve() + + # using commonpath to be compatible with py3.8 + if os.path.commonpath((full_path, repo_path)) != str(repo_path): + return False + + return True + + for pkg_mgr, options in remote_sources_packages.items(): + if not options: + continue + + for option in options: + for key, val in option.items(): + if key == "path": + if not is_path_ok(val): + raise ValueError( + f"{pkg_mgr}:{key}: path '{val}' must be relative " + "within remote source repository" + ) + elif ( + pkg_mgr == "pip" and + key in ("requirements_files", "requirements_build_files") + ): + for v in val: + if not is_path_ok(v): + raise ValueError( + f"{pkg_mgr}:{key}: path '{v}' must be relative " + "within remote source repository" + ) + else: + raise ValueError(f"unexpected key '{key}' in '{pkg_mgr}' config") + + def remote_source_to_cachi2(remote_source: Dict[str, Any]) -> Dict[str, Any]: """Converts remote source into cachi2 expected params. diff --git a/tests/plugins/test_cachi2_init.py b/tests/plugins/test_cachi2_init.py index 4cfb192fa..e2dcbba4b 100644 --- a/tests/plugins/test_cachi2_init.py +++ b/tests/plugins/test_cachi2_init.py @@ -335,6 +335,34 @@ def test_multiple_remote_sources_non_unique_names(workflow): assert result is None +def test_path_out_of_repo(workflow, mocked_cachi2_init): + """Should fail when path is outside of repository""" + container_yaml_config = dedent("""\ + remote_sources: + - name: bit-different + remote_source: + repo: https://git.example.com/team/repo.git + ref: a55c00f45ec3dfee0c766cea3d395d6e21cc2e5a + packages: + gomod: + - path: "/out/of/repo" + """) + mock_repo_config(workflow, data=container_yaml_config) + + reactor_config = dedent("""\ + allow_multiple_remote_sources: True + """) + mock_reactor_config(workflow, reactor_config) + + err_msg = ( + "gomod:path: path '/out/of/repo' must be relative within remote source repository" + ) + with pytest.raises(ValueError) as exc_info: + mocked_cachi2_init(workflow).run() + + assert err_msg in str(exc_info) + + def test_dependency_replacements(workflow): run_plugin_with_args(workflow, dependency_replacements={"dep": "something"}, expect_error="Dependency replacements are not supported by Cachi2") diff --git a/tests/utils/test_cachi2.py b/tests/utils/test_cachi2.py index f8d59ca2a..67b4531b1 100644 --- a/tests/utils/test_cachi2.py +++ b/tests/utils/test_cachi2.py @@ -6,12 +6,15 @@ of the BSD license. See the LICENSE file for details. """ +from pathlib import Path + from atomic_reactor.utils.cachi2 import ( convert_SBOM_to_ICM, remote_source_to_cachi2, gen_dependency_from_sbom_component, generate_request_json, clone_only, + validate_paths, ) import pytest @@ -106,6 +109,98 @@ def test_remote_source_to_cachi2_conversion(input_remote_source, expected_cachi2 assert remote_source_to_cachi2(input_remote_source) == expected_cachi2 +@pytest.mark.parametrize("remote_source_packages,expected_err", [ + pytest.param( + {"gomod": [{"path": "/path/to/secret"}]}, + "gomod:path: path '/path/to/secret' must be relative within remote source repository", + id="absolute_path" + ), + pytest.param( + {"gomod": [{"unknown": "/path/to/secret"}]}, + "unexpected key 'unknown' in 'gomod' config", + id="unknown_option" + ), + pytest.param( + {"gomod": [{"path": "path/../../../to/secret"}]}, + ( + "gomod:path: path 'path/../../../to/secret' must be relative " + "within remote source repository" + ), + id="path_traversal" + ), + pytest.param( + { + "pip": [{ + "path": "/src/web", + }] + }, + "pip:path: path '/src/web' must be relative within remote source repository", + id="pip_absolute_path" + ), + pytest.param( + { + "pip": [{ + "requirements_files": ["requirements.txt", "/requirements-extras.txt"], + }] + }, + ( + "pip:requirements_files: path '/requirements-extras.txt' " + "must be relative within remote source repository" + ), + id="pip_requirements_files_absolute_path" + ), + pytest.param( + { + "pip": [{ + "requirements_build_files": [ + "requirements-build.txt", "/requirements-build-extras.txt" + ] + }] + }, + ( + "pip:requirements_build_files: path '/requirements-build-extras.txt' " + "must be relative within remote source repository" + ), + id="pip_requirements_build_files_absolute_path" + ) +]) +def test_remote_source_invalid_paths(tmpdir, remote_source_packages, expected_err): + """Test conversion of remote_source (cachito) configuration from container yaml + into cachi2 params""" + with pytest.raises(ValueError) as exc_info: + validate_paths(Path(tmpdir), remote_source_packages) + + assert str(exc_info.value) == expected_err + + +def test_remote_source_path_to_symlink_out_of_repo(tmpdir): + """If cloned repo contains symlink that points out fo repo, + path in packages shouldn't be allowed""" + tmpdir_path = Path(tmpdir) + + symlink_target = tmpdir_path/"dir_outside" + symlink_target.mkdir() + + cloned = tmpdir_path/'app' + cloned.mkdir() + + symlink = cloned/"symlink" + symlink.symlink_to(symlink_target, target_is_directory=True) + + remote_source_packages = { + "pip": [{ + "path": "symlink", + }] + } + + expected_err = "pip:path: path 'symlink' must be relative within remote source repository" + + with pytest.raises(ValueError) as exc_info: + validate_paths(cloned, remote_source_packages) + + assert str(exc_info.value) == expected_err + + @pytest.mark.parametrize(('sbom', 'expected_icm'), [ pytest.param( {