From 9c65e35dcb3000e38d30865f6be692e62a5ce313 Mon Sep 17 00:00:00 2001 From: aviruthen <91846056+aviruthen@users.noreply.github.com> Date: Wed, 28 Oct 2026 14:00:50 -0700 Subject: [PATCH 1/2] fix: validate symlinks stay within source root during model packaging --- .../src/sagemaker/core/common_utils.py | 132 +++++- sagemaker-core/src/sagemaker/core/fw_utils.py | 8 + .../tests/unit/test_common_utils.py | 393 ++++++++++++++++++ sagemaker-core/tests/unit/test_fw_utils.py | 92 ++++ 4 files changed, 605 insertions(+), 20 deletions(-) diff --git a/sagemaker-core/src/sagemaker/core/common_utils.py b/sagemaker-core/src/sagemaker/core/common_utils.py index f76faba505..d0af1c278e 100644 --- a/sagemaker-core/src/sagemaker/core/common_utils.py +++ b/sagemaker-core/src/sagemaker/core/common_utils.py @@ -92,6 +92,9 @@ "/root", "/var/lib", "/opt/ml/metadata", + "/proc", + "/sys", + "/dev", ] logger = logging.getLogger(__name__) @@ -668,13 +671,11 @@ def _validate_source_directory(source_directory): # Resolve symlinks to get the actual path abs_source = abspath(realpath(source_directory)) - # Check if the source path is under any sensitive directory - for sensitive_path in _SENSITIVE_SYSTEM_PATHS: - if abs_source != "/" and abs_source.startswith(sensitive_path): - raise ValueError( - f"source_directory cannot access sensitive system paths. " - f"Got: {source_directory} (resolved to {abs_source})" - ) + if _is_sensitive_system_path(abs_source): + raise ValueError( + f"source_directory cannot access sensitive system paths. " + f"Got: {source_directory} (resolved to {abs_source})" + ) def _validate_dependency_path(dependency): @@ -694,13 +695,104 @@ def _validate_dependency_path(dependency): # Resolve symlinks to get the actual path abs_dependency = abspath(realpath(dependency)) - # Check if the dependency path is under any sensitive directory + if _is_sensitive_system_path(abs_dependency): + raise ValueError( + f"dependency path cannot access sensitive system paths. " + f"Got: {dependency} (resolved to {abs_dependency})" + ) + + +def _validate_inference_script_path(inference_script): + """Validate that an inference script path is safe to use. + + Ensures the inference script path does not access restricted system locations. + + Args: + inference_script (str): The inference script path to validate. + + Raises: + ValueError: If the path is not allowed. + """ + if not inference_script: + return + + # Resolve symlinks to get the actual path + abs_script = abspath(realpath(inference_script)) + + if _is_sensitive_system_path(abs_script): + raise ValueError( + f"inference_script cannot access sensitive system paths. " + f"Got: {inference_script} (resolved to {abs_script})" + ) + + +def _resolved_sensitive_system_paths(): + """Return the sensitive system paths, both as configured and with symlinks resolved. + + Paths such as ``/etc`` or a symlinked home directory resolve to a different location + (for example ``/private/etc`` on macOS), so both forms are checked. + + Returns: + list[str]: Normalized absolute sensitive paths. + """ + resolved = [] for sensitive_path in _SENSITIVE_SYSTEM_PATHS: - if abs_dependency != "/" and abs_dependency.startswith(sensitive_path): - raise ValueError( - f"dependency path cannot access sensitive system paths. " - f"Got: {dependency} (resolved to {abs_dependency})" - ) + for candidate in (normpath(abspath(sensitive_path)), normpath(realpath(sensitive_path))): + if candidate not in resolved: + resolved.append(candidate) + return resolved + + +def _is_sensitive_system_path(resolved_path): + """Check whether an already resolved absolute path is under a sensitive system path. + + Matching is done on whole path components, so ``~/.aws`` does not match ``~/.awsfoo``. + + Args: + resolved_path (str): An absolute path with symlinks resolved. + + Returns: + bool: True if resolved_path is a sensitive path or nested under one. + """ + resolved_path = normpath(resolved_path) + if resolved_path == os.path.sep: + return False + return any( + _is_within_base(resolved_path, sensitive_path) + for sensitive_path in _resolved_sensitive_system_paths() + ) + + +def _validate_symlinks_within_root(path, description): + """Validate that symlinks nested under a directory resolve to locations inside it. + + Copying or archiving a directory follows nested symlinks, so a symlink that points + outside the directory would pull in content from elsewhere on the file system. + Symlinks that resolve to a location inside the directory are allowed. A path that + is not a directory has no nested entries and is not checked here. + + Args: + path (str): The directory to check. + description (str): Name of the input, used in the error message. + + Raises: + ValueError: If a nested symlink resolves outside of ``path``. + """ + if not path or not os.path.isdir(path): + return + + root = normpath(realpath(abspath(path))) + for dirpath, dirnames, filenames in os.walk(path): + for name in dirnames + filenames: + entry = joinpath(dirpath, name) + if not os.path.islink(entry): + continue + target = normpath(realpath(entry)) + if not _is_within_base(target, root): + raise ValueError( + f"{description} contains a symlink that resolves outside of {path}: " + f"{entry} -> {target}" + ) def _create_or_update_code_dir( @@ -711,11 +803,10 @@ def _create_or_update_code_dir( resolved_code_dir = _get_resolved_path(code_dir) # Validate that code_dir does not resolve to a sensitive system path - for sensitive_path in _SENSITIVE_SYSTEM_PATHS: - if resolved_code_dir != "/" and resolved_code_dir.startswith(sensitive_path): - raise ValueError( - f"Invalid code_dir path: {code_dir} resolves to sensitive system path {resolved_code_dir}" - ) + if _is_sensitive_system_path(resolved_code_dir): + raise ValueError( + f"Invalid code_dir path: {code_dir} resolves to sensitive system path {resolved_code_dir}" + ) if source_directory and source_directory.lower().startswith("s3://"): local_code_path = os.path.join(tmp, "local_code.tar.gz") @@ -725,12 +816,13 @@ def _create_or_update_code_dir( custom_extractall_tarfile(t, code_dir) elif source_directory: - # Validate source_directory for security _validate_source_directory(source_directory) + _validate_symlinks_within_root(source_directory, "source_directory") if os.path.exists(code_dir): shutil.rmtree(code_dir) shutil.copytree(source_directory, code_dir) else: + _validate_inference_script_path(inference_script) if not os.path.exists(code_dir): os.mkdir(code_dir) try: @@ -742,8 +834,8 @@ def _create_or_update_code_dir( raise for dependency in dependencies: - # Validate dependency path for security _validate_dependency_path(dependency) + _validate_symlinks_within_root(dependency, "dependency") lib_dir = os.path.join(code_dir, "lib") if os.path.isdir(dependency): shutil.copytree(dependency, os.path.join(lib_dir, os.path.basename(dependency))) diff --git a/sagemaker-core/src/sagemaker/core/fw_utils.py b/sagemaker-core/src/sagemaker/core/fw_utils.py index 231fc19811..ade9b05d61 100644 --- a/sagemaker-core/src/sagemaker/core/fw_utils.py +++ b/sagemaker-core/src/sagemaker/core/fw_utils.py @@ -460,6 +460,14 @@ def tar_and_upload_dir( "Inputted directory for storing newly generated temporary directory does " f"not exist: '{settings.local_download_dir}'" ) + # create_tar_file follows symlinks, so ensure nested symlinks stay inside each input. + if directory: + utils._validate_symlinks_within_root(directory, "source_dir") + else: + utils._validate_symlinks_within_root(script, "entry_point") + for dependency in dependencies: + utils._validate_symlinks_within_root(dependency, "dependency") + local_download_dir = None if settings is None else settings.local_download_dir tmp = tempfile.mkdtemp(dir=local_download_dir) encrypt_artifact = True if settings is None else settings.encrypt_repacked_artifacts diff --git a/sagemaker-core/tests/unit/test_common_utils.py b/sagemaker-core/tests/unit/test_common_utils.py index 83145ab242..d1530d4add 100644 --- a/sagemaker-core/tests/unit/test_common_utils.py +++ b/sagemaker-core/tests/unit/test_common_utils.py @@ -2781,6 +2781,399 @@ def test_create_or_update_code_dir_validates_dependencies(self): ) +def _fake_home_with_decoy(tmp_path): + """Create a fake home directory holding a decoy credentials file.""" + home = tmp_path / "home" + aws_dir = home / ".aws" + aws_dir.mkdir(parents=True) + decoy = aws_dir / "credentials" + decoy.write_text("decoy-not-a-real-credential") + return home, decoy + + +def _source_tree(tmp_path, name="source"): + """Create a small source directory with an entry script.""" + source = tmp_path / name + (source / "pkg").mkdir(parents=True) + (source / "inference.py").write_text("# inference code") + (source / "pkg" / "helper.py").write_text("# helper code") + return source + + +class TestValidateSymlinksWithinRoot: + """Test _validate_symlinks_within_root function.""" + + def test_allows_tree_without_symlinks(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + _validate_symlinks_within_root(str(_source_tree(tmp_path)), "source_directory") + + def test_allows_symlinks_inside_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + os.symlink(source / "pkg" / "helper.py", source / "helper_link.py") + os.symlink("pkg", source / "pkg_link") + os.symlink("../inference.py", source / "pkg" / "relative_link.py") + + _validate_symlinks_within_root(str(source), "source_directory") + + def test_allows_root_that_is_a_symlink(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + os.symlink(source / "inference.py", source / "pkg" / "entry_link.py") + root_link = tmp_path / "source_link" + os.symlink(source, root_link) + + _validate_symlinks_within_root(str(root_link), "source_directory") + + def test_rejects_nested_file_symlink_outside_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + _, decoy = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + os.symlink(decoy, source / "pkg" / "config.ini") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_rejects_nested_directory_symlink_outside_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + home, _ = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + (source / "pkg" / "deep").mkdir() + os.symlink(home / ".aws", source / "pkg" / "deep" / "settings") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_rejects_relative_symlink_escaping_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + os.symlink("../../home/.aws/credentials", source / "pkg" / "config.ini") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_rejects_symlink_into_sibling_with_shared_prefix(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + sibling = tmp_path / "source-other" + sibling.mkdir() + (sibling / "data.txt").write_text("outside") + os.symlink(sibling / "data.txt", source / "data.txt") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_rejects_proc_symlink(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + os.symlink("/proc/self/environ", source / "env.txt") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_ignores_files_and_missing_paths(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + script = tmp_path / "train.py" + script.write_text("# train") + + _validate_symlinks_within_root(str(script), "entry_point") + _validate_symlinks_within_root(str(tmp_path / "missing"), "entry_point") + _validate_symlinks_within_root(None, "entry_point") + + +class TestSensitiveSystemPathMatching: + """Test denylist matching used by the source, dependency, and script validators.""" + + @pytest.mark.parametrize("path", ["/proc", "/proc/self/environ", "/sys/kernel", "/dev/null"]) + def test_rejects_pseudo_file_systems(self, path): + from sagemaker.core.common_utils import ( + _validate_dependency_path, + _validate_inference_script_path, + _validate_source_directory, + ) + + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_source_directory(path) + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_dependency_path(path) + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_inference_script_path(path) + + def test_rejects_etc_after_symlink_resolution(self): + """/etc resolves to /private/etc on macOS; both forms must be rejected.""" + from sagemaker.core.common_utils import _validate_source_directory + + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_source_directory("/etc") + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_source_directory(os.path.realpath("/etc")) + + def test_rejects_path_under_symlinked_denylist_entry(self, tmp_path): + """A denylist entry that is itself a symlink (e.g. a symlinked HOME) still matches.""" + from sagemaker.core.common_utils import _validate_source_directory + + real_home, _ = _fake_home_with_decoy(tmp_path) + linked_home = tmp_path / "linked_home" + os.symlink(real_home, linked_home) + + with patch( + "sagemaker.core.common_utils._SENSITIVE_SYSTEM_PATHS", [str(linked_home / ".aws")] + ): + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_source_directory(str(real_home / ".aws")) + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_source_directory(str(linked_home / ".aws")) + + def test_matches_whole_path_components(self, tmp_path): + from sagemaker.core.common_utils import _validate_dependency_path + + home, _ = _fake_home_with_decoy(tmp_path) + lookalike = home / ".awsfoo" + lookalike.mkdir() + + with patch("sagemaker.core.common_utils._SENSITIVE_SYSTEM_PATHS", [str(home / ".aws")]): + _validate_dependency_path(str(lookalike)) + with pytest.raises(ValueError, match="cannot access sensitive system paths"): + _validate_dependency_path(str(home / ".aws")) + + def test_root_directory_is_not_matched(self): + from sagemaker.core.common_utils import _is_sensitive_system_path + + assert _is_sensitive_system_path("/") is False + + +class TestCreateOrUpdateCodeDirSymlinks: + """Test symlink handling in _create_or_update_code_dir.""" + + @staticmethod + def _model_dir(tmp_path): + model_dir = tmp_path / "model" + model_dir.mkdir() + return model_dir + + def test_rejects_source_directory_with_outside_file_symlink(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + _, decoy = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + os.symlink(decoy, source / "pkg" / "config.ini") + model_dir = self._model_dir(tmp_path) + + with pytest.raises(ValueError, match="source_directory contains a symlink"): + _create_or_update_code_dir( + str(model_dir), "inference.py", str(source), [], None, str(tmp_path) + ) + assert not (model_dir / "code").exists() + + def test_rejects_source_directory_with_outside_directory_symlink(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + home, _ = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + os.symlink(home / ".aws", source / "pkg" / "settings") + model_dir = self._model_dir(tmp_path) + + with pytest.raises(ValueError, match="source_directory contains a symlink"): + _create_or_update_code_dir( + str(model_dir), "inference.py", str(source), [], None, str(tmp_path) + ) + assert not (model_dir / "code").exists() + + def test_rejects_dependency_with_outside_symlink(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + home, decoy = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + dep = tmp_path / "my_lib" + (dep / "nested").mkdir(parents=True) + os.symlink(decoy, dep / "nested" / "config.ini") + model_dir = self._model_dir(tmp_path) + + with pytest.raises(ValueError, match="dependency contains a symlink"): + _create_or_update_code_dir( + str(model_dir), "inference.py", str(source), [str(dep)], None, str(tmp_path) + ) + assert not (model_dir / "code" / "lib").exists() + + def test_rejects_dependency_with_outside_directory_symlink(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + home, _ = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + dep = tmp_path / "my_lib" + dep.mkdir() + os.symlink(home / ".aws", dep / "settings") + model_dir = self._model_dir(tmp_path) + + with pytest.raises(ValueError, match="dependency contains a symlink"): + _create_or_update_code_dir( + str(model_dir), "inference.py", str(source), [str(dep)], None, str(tmp_path) + ) + + def test_rejects_inference_script_resolving_to_sensitive_path(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + home, decoy = _fake_home_with_decoy(tmp_path) + script_link = tmp_path / "inference.py" + os.symlink(decoy, script_link) + model_dir = self._model_dir(tmp_path) + + with patch("sagemaker.core.common_utils._SENSITIVE_SYSTEM_PATHS", [str(home / ".aws")]): + with pytest.raises(ValueError, match="inference_script cannot access"): + _create_or_update_code_dir( + str(model_dir), str(script_link), None, [], None, str(tmp_path) + ) + assert not (model_dir / "code").exists() + + def test_rejects_proc_inference_script(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + model_dir = self._model_dir(tmp_path) + + with pytest.raises(ValueError, match="inference_script cannot access"): + _create_or_update_code_dir( + str(model_dir), "/proc/self/environ", None, [], None, str(tmp_path) + ) + + def test_copies_symlinks_inside_root(self, tmp_path): + from sagemaker.core.common_utils import _create_or_update_code_dir + + source = _source_tree(tmp_path) + os.symlink(source / "pkg" / "helper.py", source / "helper_link.py") + os.symlink("pkg", source / "pkg_link") + dep = tmp_path / "my_lib" + dep.mkdir() + (dep / "util.py").write_text("# util") + os.symlink("util.py", dep / "util_link.py") + model_dir = self._model_dir(tmp_path) + + _create_or_update_code_dir( + str(model_dir), "inference.py", str(source), [str(dep)], None, str(tmp_path) + ) + + code_dir = model_dir / "code" + assert (code_dir / "helper_link.py").read_text() == "# helper code" + assert (code_dir / "pkg_link" / "helper.py").read_text() == "# helper code" + assert (code_dir / "lib" / "my_lib" / "util_link.py").read_text() == "# util" + + +class TestRepackModelSymlinks: + """End-to-end repack_model checks with local file:// URIs (no S3).""" + + @staticmethod + def _model_tar(tmp_path): + model_src = tmp_path / "model_src" + model_src.mkdir() + (model_src / "model.pth").write_text("weights") + model_tar = tmp_path / "model.tar.gz" + with tarfile.open(model_tar, "w:gz") as tar: + tar.add(model_src, arcname=".") + return model_tar + + @staticmethod + def _session(): + session = Mock() + session.settings = None + return session + + @staticmethod + def _archive_names(path): + with tarfile.open(path, "r:gz") as tar: + return tar.getnames() + + def test_rejects_source_directory_with_outside_symlink(self, tmp_path): + from sagemaker.core.common_utils import repack_model + + _, decoy = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + os.symlink(decoy, source / "pkg" / "config.ini") + output = tmp_path / "output.tar.gz" + + with pytest.raises(ValueError, match="symlink that resolves outside"): + repack_model( + "inference.py", + str(source), + [], + f"file://{self._model_tar(tmp_path)}", + f"file://{output}", + self._session(), + ) + assert not output.exists() + + def test_rejects_dependency_with_outside_symlink(self, tmp_path): + from sagemaker.core.common_utils import repack_model + + home, _ = _fake_home_with_decoy(tmp_path) + source = _source_tree(tmp_path) + dep = tmp_path / "my_lib" + dep.mkdir() + os.symlink(home / ".aws", dep / "settings") + output = tmp_path / "output.tar.gz" + + with pytest.raises(ValueError, match="symlink that resolves outside"): + repack_model( + "inference.py", + str(source), + [str(dep)], + f"file://{self._model_tar(tmp_path)}", + f"file://{output}", + self._session(), + ) + assert not output.exists() + + def test_repacks_symlinks_inside_root(self, tmp_path): + from sagemaker.core.common_utils import repack_model + + source = _source_tree(tmp_path) + os.symlink(source / "pkg" / "helper.py", source / "helper_link.py") + output = tmp_path / "output.tar.gz" + + repack_model( + "inference.py", + str(source), + [], + f"file://{self._model_tar(tmp_path)}", + f"file://{output}", + self._session(), + ) + + names = {name.lstrip("/") for name in self._archive_names(output)} + assert "code/helper_link.py" in names + assert "code/inference.py" in names + + +class TestCreateTarFileSymlinks: + """create_tar_file is shared with model uploads and keeps following symlinks.""" + + def test_follows_symlink_outside_source_root(self, tmp_path): + """Hugging Face cache snapshots link into ../../blobs, outside the snapshot root.""" + from sagemaker.core.common_utils import create_tar_file + + blobs = tmp_path / "blobs" + blobs.mkdir() + (blobs / "abc123").write_text("weights") + snapshot = tmp_path / "snapshots" / "rev" + snapshot.mkdir(parents=True) + os.symlink("../../blobs/abc123", snapshot / "model.safetensors") + + tar_path = create_tar_file([str(snapshot / "model.safetensors")], str(tmp_path / "t.tgz")) + + with tarfile.open(tar_path, "r:gz") as tar: + member = tar.getmember("model.safetensors") + assert member.isfile() + assert tar.extractfile(member).read() == b"weights" + + class TestDownloadFileSpotCheck: """Spot-check behavior in common_utils.download_file.""" diff --git a/sagemaker-core/tests/unit/test_fw_utils.py b/sagemaker-core/tests/unit/test_fw_utils.py index 966b10079c..b0c4239e47 100644 --- a/sagemaker-core/tests/unit/test_fw_utils.py +++ b/sagemaker-core/tests/unit/test_fw_utils.py @@ -15,6 +15,9 @@ from __future__ import absolute_import import json +import os +import tarfile + import pytest from unittest.mock import Mock, patch @@ -264,6 +267,95 @@ def test_tar_and_upload_dir_local_file( assert result.s3_prefix == "s3://test-bucket/prefix/sourcedir.tar.gz" assert result.script_name == "train.py" + @staticmethod + def _decoy(tmp_path): + """Create a decoy credentials file under a fake home directory.""" + aws_dir = tmp_path / "home" / ".aws" + aws_dir.mkdir(parents=True) + decoy = aws_dir / "credentials" + decoy.write_text("decoy-not-a-real-credential") + return decoy + + @staticmethod + def _source_dir(tmp_path): + source = tmp_path / "source" + (source / "pkg").mkdir(parents=True) + (source / "train.py").write_text("print('hello')") + return source + + @staticmethod + def _upload(tmp_path, source, dependencies=None, on_upload=None): + mock_session = Mock() + mock_session.region_name = "us-west-2" + upload_file = mock_session.resource.return_value.Object.return_value.upload_file + upload_file.side_effect = on_upload + return tar_and_upload_dir( + session=mock_session, + bucket="test-bucket", + s3_key_prefix="prefix", + script="train.py", + directory=str(source), + dependencies=dependencies, + settings=Mock(local_download_dir=str(tmp_path), encrypt_repacked_artifacts=False), + ) + + @patch("sagemaker.core.common_utils.create_tar_file") + def test_tar_and_upload_dir_rejects_directory_with_outside_symlink( + self, mock_create_tar, tmp_path + ): + source = self._source_dir(tmp_path) + os.symlink(self._decoy(tmp_path), source / "pkg" / "config.ini") + + with pytest.raises(ValueError, match="source_dir contains a symlink"): + self._upload(tmp_path, source) + mock_create_tar.assert_not_called() + + @patch("sagemaker.core.common_utils.create_tar_file") + def test_tar_and_upload_dir_rejects_dependency_with_outside_symlink( + self, mock_create_tar, tmp_path + ): + source = self._source_dir(tmp_path) + dep = tmp_path / "my_lib" + dep.mkdir() + os.symlink(self._decoy(tmp_path).parent, dep / "settings") + + with pytest.raises(ValueError, match="dependency contains a symlink"): + self._upload(tmp_path, source, dependencies=[str(dep)]) + mock_create_tar.assert_not_called() + + @patch("sagemaker.core.common_utils.create_tar_file") + def test_tar_and_upload_dir_rejects_entry_point_directory_with_outside_symlink( + self, mock_create_tar, tmp_path + ): + """Without source_dir, a directory passed as the script is still checked.""" + script_dir = tmp_path / "code" + script_dir.mkdir() + os.symlink(self._decoy(tmp_path), script_dir / "config.ini") + + with pytest.raises(ValueError, match="entry_point contains a symlink"): + tar_and_upload_dir( + session=Mock(), + bucket="test-bucket", + s3_key_prefix="prefix", + script=str(script_dir), + ) + mock_create_tar.assert_not_called() + + def test_tar_and_upload_dir_packages_symlinks_inside_root(self, tmp_path): + source = self._source_dir(tmp_path) + (source / "pkg" / "helper.py").write_text("# helper") + os.symlink("pkg/helper.py", source / "helper_link.py") + captured = {} + + def _capture(tar_file, ExtraArgs=None): + with tarfile.open(tar_file, "r:gz") as tar: + captured["names"] = set(tar.getnames()) + + result = self._upload(tmp_path, source, on_upload=_capture) + + assert result.s3_prefix == "s3://test-bucket/prefix/sourcedir.tar.gz" + assert {"train.py", "helper_link.py", "pkg/helper.py"} <= captured["names"] + class TestFrameworkNameFromImage: """Test framework_name_from_image function.""" From 6422b71c6f5dd583e4288763f82fcce9c620bd03 Mon Sep 17 00:00:00 2001 From: aviruthen <91846056+aviruthen@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:35:02 -0700 Subject: [PATCH 2/2] fix: clarify packaging symlink errors and allow /dev/shm scratch paths --- .../src/sagemaker/core/common_utils.py | 38 ++++++++++++-- .../tests/unit/test_common_utils.py | 52 +++++++++++++++++++ 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/sagemaker-core/src/sagemaker/core/common_utils.py b/sagemaker-core/src/sagemaker/core/common_utils.py index d0af1c278e..b21c05ca16 100644 --- a/sagemaker-core/src/sagemaker/core/common_utils.py +++ b/sagemaker-core/src/sagemaker/core/common_utils.py @@ -97,6 +97,12 @@ "/dev", ] +# Locations under _SENSITIVE_SYSTEM_PATHS that are allowed, such as shared memory used +# as scratch space. +_SENSITIVE_SYSTEM_PATH_EXCEPTIONS = [ + "/dev/shm", +] + logger = logging.getLogger(__name__) TagsDict = Dict[str, Union[str, PipelineVariable]] @@ -733,20 +739,34 @@ def _resolved_sensitive_system_paths(): (for example ``/private/etc`` on macOS), so both forms are checked. Returns: - list[str]: Normalized absolute sensitive paths. + tuple[str, ...]: Normalized absolute sensitive paths. + """ + return _resolve_path_variants(tuple(_SENSITIVE_SYSTEM_PATHS)) + + +@lru_cache(maxsize=8) +def _resolve_path_variants(paths): + """Return each path both as configured and with symlinks resolved, without duplicates. + + Args: + paths (tuple[str, ...]): Paths to resolve. + + Returns: + tuple[str, ...]: Normalized absolute paths. """ resolved = [] - for sensitive_path in _SENSITIVE_SYSTEM_PATHS: - for candidate in (normpath(abspath(sensitive_path)), normpath(realpath(sensitive_path))): + for path in paths: + for candidate in (normpath(abspath(path)), normpath(realpath(path))): if candidate not in resolved: resolved.append(candidate) - return resolved + return tuple(resolved) def _is_sensitive_system_path(resolved_path): """Check whether an already resolved absolute path is under a sensitive system path. Matching is done on whole path components, so ``~/.aws`` does not match ``~/.awsfoo``. + Paths under ``_SENSITIVE_SYSTEM_PATH_EXCEPTIONS`` (for example ``/dev/shm``) are allowed. Args: resolved_path (str): An absolute path with symlinks resolved. @@ -757,6 +777,11 @@ def _is_sensitive_system_path(resolved_path): resolved_path = normpath(resolved_path) if resolved_path == os.path.sep: return False + if any( + _is_within_base(resolved_path, allowed_path) + for allowed_path in _resolve_path_variants(tuple(_SENSITIVE_SYSTEM_PATH_EXCEPTIONS)) + ): + return False return any( _is_within_base(resolved_path, sensitive_path) for sensitive_path in _resolved_sensitive_system_paths() @@ -791,7 +816,10 @@ def _validate_symlinks_within_root(path, description): if not _is_within_base(target, root): raise ValueError( f"{description} contains a symlink that resolves outside of {path}: " - f"{entry} -> {target}" + f"{entry} -> {target}. Symlinks under {description} must point to files " + f"or directories inside it. Replace the symlink with a copy of its " + f"target, or remove it from {path} (for example, move a virtual " + f"environment or other tooling directory out of {description})." ) diff --git a/sagemaker-core/tests/unit/test_common_utils.py b/sagemaker-core/tests/unit/test_common_utils.py index d1530d4add..8edc9dcdf7 100644 --- a/sagemaker-core/tests/unit/test_common_utils.py +++ b/sagemaker-core/tests/unit/test_common_utils.py @@ -2880,6 +2880,40 @@ def test_rejects_proc_symlink(self, tmp_path): with pytest.raises(ValueError, match="symlink that resolves outside"): _validate_symlinks_within_root(str(source), "source_directory") + def test_rejects_dangling_symlink_outside_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + os.symlink(tmp_path / "does-not-exist", source / "missing.txt") + + with pytest.raises(ValueError, match="symlink that resolves outside"): + _validate_symlinks_within_root(str(source), "source_directory") + + def test_allows_dangling_symlink_inside_root(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + os.symlink("pkg/not-created-yet.py", source / "pending.py") + + _validate_symlinks_within_root(str(source), "source_directory") + + def test_virtualenv_layout_error_is_actionable(self, tmp_path): + from sagemaker.core.common_utils import _validate_symlinks_within_root + + source = _source_tree(tmp_path) + interpreter = tmp_path / "python3" + interpreter.write_text("") + (source / ".venv" / "bin").mkdir(parents=True) + os.symlink(interpreter, source / ".venv" / "bin" / "python") + + with pytest.raises(ValueError) as exc_info: + _validate_symlinks_within_root(str(source), "source_directory") + + message = str(exc_info.value) + assert os.path.join(".venv", "bin", "python") in message + assert "Replace the symlink with a copy of its target" in message + assert "virtual environment" in message + def test_ignores_files_and_missing_paths(self, tmp_path): from sagemaker.core.common_utils import _validate_symlinks_within_root @@ -2946,6 +2980,24 @@ def test_matches_whole_path_components(self, tmp_path): with pytest.raises(ValueError, match="cannot access sensitive system paths"): _validate_dependency_path(str(home / ".aws")) + def test_allows_shared_memory_scratch_space(self): + from sagemaker.core.common_utils import _is_sensitive_system_path + + assert _is_sensitive_system_path("/dev/shm") is False + assert _is_sensitive_system_path("/dev/shm/model/code") is False + assert _is_sensitive_system_path("/dev/null") is True + + def test_reflects_updates_to_sensitive_paths(self, tmp_path): + from sagemaker.core.common_utils import _is_sensitive_system_path + + target = tmp_path / "restricted" + target.mkdir() + + assert _is_sensitive_system_path(str(target)) is False + with patch("sagemaker.core.common_utils._SENSITIVE_SYSTEM_PATHS", [str(target)]): + assert _is_sensitive_system_path(str(target / "file")) is True + assert _is_sensitive_system_path(str(target)) is False + def test_root_directory_is_not_matched(self): from sagemaker.core.common_utils import _is_sensitive_system_path