diff --git a/src/agents/sandbox/util/tar_utils.py b/src/agents/sandbox/util/tar_utils.py index 55adbd77e4..15746e2b62 100644 --- a/src/agents/sandbox/util/tar_utils.py +++ b/src/agents/sandbox/util/tar_utils.py @@ -59,6 +59,20 @@ def _validate_symlink_target( if not member.issym() or allow_external_symlink_targets: return + # Member names already reject Windows drive and separator syntax. Targets are parsed + # as POSIX, so `C:/Windows` and `..\\..\\Windows` would otherwise read as ordinary + # relative components and pass the checks below. + if PureWindowsPath(member.linkname).drive: + raise UnsafeTarMemberError( + member=member.name, + reason=f"windows drive symlink target not allowed: {member.linkname}", + ) + if "\\" in member.linkname: + raise UnsafeTarMemberError( + member=member.name, + reason=f"windows path separator in symlink target: {member.linkname}", + ) + target = PurePosixPath(member.linkname) if target.is_absolute(): raise UnsafeTarMemberError( @@ -157,7 +171,11 @@ def strip_tar_member_prefix(data: io.IOBase, *, prefix: str | Path) -> io.IOBase out.seek(0) with tarfile.open(fileobj=out, mode="r:*") as tar: - validate_tarfile(tar) + # Persisting a snapshot reads a workspace that already exists, so an external + # symlink here is something the workspace already contained rather than + # something an archive is introducing. Hydration is where that link would + # take effect, and every hydrate path rejects it. + validate_tarfile(tar, allow_external_symlink_targets=True) out.seek(0) return cast(io.IOBase, out) except Exception: @@ -259,7 +277,7 @@ def validate_tarfile( skip_rel_paths: Iterable[str | Path] = (), root_name: str | None = None, allow_symlinks: bool = True, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """Validate a workspace tar before handing it to a local or remote extractor. @@ -345,7 +363,7 @@ def validate_tar_bytes( reject_symlink_rel_paths: Iterable[str | Path] = (), skip_rel_paths: Iterable[str | Path] = (), root_name: str | None = None, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """Validate raw workspace tar bytes with the shared safe tar policy.""" @@ -369,7 +387,7 @@ def safe_extract_tarfile( tar: tarfile.TarFile, *, root: Path, - allow_external_symlink_targets: bool = True, + allow_external_symlink_targets: bool = False, ) -> None: """ Safely extract a tar archive into `root`. diff --git a/tests/extensions/sandbox/test_blaxel.py b/tests/extensions/sandbox/test_blaxel.py index 3fe1d0d93a..8b98629ef1 100644 --- a/tests/extensions/sandbox/test_blaxel.py +++ b/tests/extensions/sandbox/test_blaxel.py @@ -1178,8 +1178,8 @@ def _make_tar_with_symlink_and_file(*, symlink_name: str, target: str, file_name class TestValidateTarBytes: - def _validate(self, raw: bytes) -> None: - validate_tar_bytes(raw) + def _validate(self, raw: bytes, *, allow_external_symlink_targets: bool = False) -> None: + validate_tar_bytes(raw, allow_external_symlink_targets=allow_external_symlink_targets) def test_valid_tar(self) -> None: raw = _make_tar({"hello.txt": b"content", "subdir/": None}) @@ -1210,7 +1210,7 @@ def test_tar_member_under_archive_symlink_rejected(self) -> None: file_name="link.txt/nested.txt", ) with pytest.raises(ValueError, match="descends through symlink"): - self._validate(raw) + self._validate(raw, allow_external_symlink_targets=True) def test_corrupt_tar_rejected(self) -> None: with pytest.raises(ValueError, match="invalid tar"): diff --git a/tests/sandbox/test_tar_utils.py b/tests/sandbox/test_tar_utils.py index 50402557c6..5475ce4417 100644 --- a/tests/sandbox/test_tar_utils.py +++ b/tests/sandbox/test_tar_utils.py @@ -16,6 +16,7 @@ safe_tar_member_rel_path, strip_tar_member_prefix, validate_tar_bytes, + validate_tarfile, ) @@ -70,9 +71,11 @@ def _fifo(name: str) -> _Member: return _Member(member) -def _safe_extract(raw: bytes, root: Path) -> None: +def _safe_extract(raw: bytes, root: Path, *, allow_external_symlink_targets: bool = False) -> None: with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: - safe_extract_tarfile(tar, root=root) + safe_extract_tarfile( + tar, root=root, allow_external_symlink_targets=allow_external_symlink_targets + ) def test_safe_extract_tarfile_preserves_venv_style_symlinks(tmp_path: Path) -> None: @@ -88,8 +91,8 @@ def test_safe_extract_tarfile_preserves_venv_style_symlinks(tmp_path: Path) -> N _symlink("./uv-project/.venv/bin/python", "python3"), ) - validate_tar_bytes(raw) - _safe_extract(raw, tmp_path) + validate_tar_bytes(raw, allow_external_symlink_targets=True) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) assert (tmp_path / "uv-project" / "main.py").read_text() == 'print("snapshot smoke")\n' assert os.readlink(tmp_path / "uv-project" / ".venv" / "lib64") == "lib" @@ -165,6 +168,68 @@ def test_validate_tar_bytes_allows_internal_symlink_target_in_strict_mode() -> N validate_tar_bytes(raw, allow_external_symlink_targets=False) +def test_validate_tar_bytes_rejects_external_symlink_target_by_default() -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tar_bytes(raw) + + +def test_validate_tarfile_rejects_external_symlink_target_by_default() -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + + with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + validate_tarfile(tar) + + +def test_safe_extract_tarfile_rejects_external_symlink_target_by_default(tmp_path: Path) -> None: + raw = _tar_bytes(_symlink("leak", "/etc/passwd")) + root = tmp_path / "root" + root.mkdir() + + with tarfile.open(fileobj=io.BytesIO(raw), mode="r:*") as tar: + with pytest.raises(UnsafeTarMemberError, match="absolute symlink target not allowed"): + safe_extract_tarfile(tar, root=root) + + assert not (root / "leak").exists() + + +@pytest.mark.parametrize( + "target", + [ + "C:/Windows/System32", + r"C:\Windows\System32", + r"..\..\Windows", + r"\\server\share", + ], +) +def test_validate_tar_bytes_rejects_windows_symlink_targets_by_default(target: str) -> None: + # Member names already reject this syntax. Targets are parsed as POSIX, so these + # would otherwise read as ordinary relative components. + raw = _tar_bytes(_symlink("link", target)) + + with pytest.raises(UnsafeTarMemberError, match="windows (drive symlink target|path separator)"): + validate_tar_bytes(raw) + + +def test_validate_tar_bytes_allows_windows_symlink_targets_when_opted_in() -> None: + raw = _tar_bytes(_symlink("link", "C:/Windows/System32")) + + validate_tar_bytes(raw, allow_external_symlink_targets=True) + + +def test_strip_tar_member_prefix_still_persists_external_symlink_targets() -> None: + # Persisting reads a workspace that already contains the link. Hydration is where it + # would take effect, and every hydrate path rejects it. + raw = _tar_bytes(_dir("workspace"), _symlink("workspace/python", "/usr/bin/python3")) + + normalized = strip_tar_member_prefix(io.BytesIO(raw), prefix="workspace") + + with tarfile.open(fileobj=normalized, mode="r:*") as tar: + assert tar.getnames() == [".", "python"] + + def test_strip_tar_member_prefix_returns_workspace_relative_archive() -> None: raw = _tar_bytes( _dir("workspace"), @@ -200,7 +265,7 @@ def test_strip_tar_member_prefix_rewrites_pax_path_headers() -> None: def test_safe_extract_tarfile_can_rehydrate_existing_leaf_symlink(tmp_path: Path) -> None: raw = _tar_bytes(_symlink("link.txt", "/usr/local/bin/python3")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) assert os.readlink(tmp_path / "link.txt") == "/usr/local/bin/python3" raw = _tar_bytes(_symlink("link.txt", "target-v2.txt")) @@ -239,7 +304,7 @@ def test_safe_extract_tarfile_can_replace_existing_leaf_symlink_with_file( tmp_path: Path, ) -> None: raw = _tar_bytes(_symlink("python", "/usr/local/bin/python3")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) raw = _tar_bytes(_file("python", b"real file")) @@ -252,7 +317,7 @@ def test_safe_extract_tarfile_can_replace_existing_leaf_symlink_with_directory( tmp_path: Path, ) -> None: raw = _tar_bytes(_symlink("bin", "/usr/local/bin")) - _safe_extract(raw, tmp_path) + _safe_extract(raw, tmp_path, allow_external_symlink_targets=True) raw = _tar_bytes(_dir("bin"), _file("bin/python", b"real file")) @@ -292,27 +357,36 @@ def test_validate_tar_bytes_rejects_members_under_archive_symlink() -> None: ) with pytest.raises(UnsafeTarMemberError, match="descends through symlink"): - validate_tar_bytes(raw) + validate_tar_bytes(raw, allow_external_symlink_targets=True) def test_validate_tar_bytes_can_reject_specific_symlink_path() -> None: raw = _tar_bytes(_symlink("workspace", "/tmp/outside")) with pytest.raises(UnsafeTarMemberError, match="symlink member not allowed: workspace"): - validate_tar_bytes(raw, reject_symlink_rel_paths={Path("workspace")}) + validate_tar_bytes( + raw, + reject_symlink_rel_paths={Path("workspace")}, + allow_external_symlink_targets=True, + ) def test_validate_tar_bytes_specific_symlink_rejection_normalizes_dot_prefix() -> None: raw = _tar_bytes(_symlink("./workspace", "/tmp/outside")) with pytest.raises(UnsafeTarMemberError, match="symlink member not allowed: workspace"): - validate_tar_bytes(raw, reject_symlink_rel_paths={"workspace"}) + validate_tar_bytes( + raw, + reject_symlink_rel_paths={"workspace"}, + allow_external_symlink_targets=True, + ) def test_validate_tar_bytes_specific_symlink_rejection_does_not_reject_children() -> None: validate_tar_bytes( _tar_bytes(_dir("workspace"), _symlink("workspace/link", "/tmp/outside")), reject_symlink_rel_paths={"workspace"}, + allow_external_symlink_targets=True, )