Skip to content

Commit 4f2cdfd

Browse files
authored
fix: anchor tar member validation to extract_path in sagemaker-core (#6195)
custom_extractall_tarfile falls back to filtering members with _get_safe_members when tarfile.data_filter is unavailable (Python < 3.12, before the 3.9.17 / 3.10.12 / 3.11.4 backports). That fallback had two containment defects that combined to let an archive member be written outside the extraction directory: - _get_safe_members anchored its check to the process working directory (_get_resolved_path("")) rather than the directory the archive is extracted into. It now takes the base as an argument, and custom_extractall_tarfile passes the resolved extract_path. - _is_bad_path compared paths with str.startswith, so a sibling directory sharing a textual prefix with the base (e.g. base "/tmp/extract" and "/tmp/extract-evil/f") was treated as contained. Containment is now checked with os.path.commonpath, via a shared _is_within_base helper that _validate_extracted_paths uses as well. Absolute member paths are now rejected outright, since joinpath would otherwise silently discard the base for them. Both defects were required for the escape: relative member resolution is base-independent under normpath, so a plain "../../x" member does not escape on its own. The working-directory anchoring is what made a prefix-matching sibling ("<cwd>" vs "<cwd>evil") pass validation while extraction still wrote outside extract_path. _validate_extracted_paths only walks extract_path, so it did not catch the escape either. This is the shared utility every v3 extraction path routes through (model unpack, local/image.py, serve TGI and DJL prepare, pipeline repack). An already-correct implementation of the same logic exists in sagemaker-mlops/src/sagemaker/mlops/workflow/_repack_model.py; this brings common_utils.py in line with it. Adds regression tests covering the end-to-end escape, the sibling-prefix bypass, absolute members, and that validation is anchored to extract_path.
1 parent 44410e2 commit 4f2cdfd

2 files changed

Lines changed: 205 additions & 9 deletions

File tree

‎sagemaker-core/src/sagemaker/core/common_utils.py‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
import abc
3333
import uuid
3434
from datetime import datetime
35-
from os.path import abspath, realpath, dirname, normpath, join as joinpath
35+
from os.path import abspath, realpath, dirname, isabs, normpath, join as joinpath
3636

3737
from importlib import import_module
3838

@@ -1734,6 +1734,28 @@ def validate_path_within_directory(file_path, target_directory, source_descripti
17341734
)
17351735

17361736

1737+
def _is_within_base(resolved_path, base):
1738+
"""Checks if an already resolved absolute path is contained within a base directory.
1739+
1740+
Uses os.path.commonpath rather than a string prefix comparison, so that a sibling
1741+
directory which merely shares a textual prefix with the base directory (e.g. base
1742+
"/tmp/extract" and path "/tmp/extract-evil/f") is not treated as contained.
1743+
1744+
Args:
1745+
resolved_path (str): An absolute, normalized path.
1746+
base (str): An absolute, normalized base directory.
1747+
1748+
Returns:
1749+
bool: True if resolved_path is the base directory or nested under it.
1750+
"""
1751+
try:
1752+
return os.path.commonpath([resolved_path, base]) == base
1753+
except ValueError:
1754+
# Raised when the paths cannot be compared (e.g. different drives on Windows),
1755+
# in which case resolved_path cannot be inside base.
1756+
return False
1757+
1758+
17371759
def _is_bad_path(path, base):
17381760
"""Checks if the joined path (base directory + file path) is rooted under the base directory
17391761
@@ -1747,8 +1769,11 @@ def _is_bad_path(path, base):
17471769
Returns:
17481770
bool: True if the path is not rooted under the base directory, False otherwise.
17491771
"""
1750-
# joinpath will ignore base if path is absolute
1751-
return not _get_resolved_path(joinpath(base, path)).startswith(base)
1772+
# joinpath would silently discard base for an absolute path, and an archive member
1773+
# targeting an absolute location is never legitimate, so reject it outright.
1774+
if isabs(path):
1775+
return True
1776+
return not _is_within_base(_get_resolved_path(joinpath(base, path)), base)
17521777

17531778

17541779
def _is_bad_link(info, base):
@@ -1768,19 +1793,20 @@ def _is_bad_link(info, base):
17681793
return _is_bad_path(info.linkname, base=tip)
17691794

17701795

1771-
def _get_safe_members(members):
1796+
def _get_safe_members(members, base):
17721797
"""A generator that yields members that are safe to extract.
17731798
17741799
It filters out bad paths and bad links.
17751800
17761801
Args:
17771802
members (list): A list of members to check.
1803+
base (str): The resolved base directory that members must stay within. This must
1804+
be the directory the archive is extracted into, since that is what the member
1805+
paths are resolved against at extraction time.
17781806
17791807
Yields:
17801808
tarfile.TarInfo: The tar file info.
17811809
"""
1782-
base = _get_resolved_path("")
1783-
17841810
for file_info in members:
17851811
if _is_bad_path(file_info.name, base):
17861812
logger.error("%s is blocked (illegal path)", file_info.name)
@@ -1811,15 +1837,15 @@ def _validate_extracted_paths(extract_path):
18111837
for dir_name in dirs:
18121838
dir_path = os.path.join(root, dir_name)
18131839
resolved = _get_resolved_path(dir_path)
1814-
if not resolved.startswith(base):
1840+
if not _is_within_base(resolved, base):
18151841
logger.error("Extracted directory escaped extraction path: %s", dir_path)
18161842
raise ValueError(f"Extracted path outside expected directory: {dir_path}")
18171843

18181844
# Check files
18191845
for file_name in files:
18201846
file_path = os.path.join(root, file_name)
18211847
resolved = _get_resolved_path(file_path)
1822-
if not resolved.startswith(base):
1848+
if not _is_within_base(resolved, base):
18231849
logger.error("Extracted file escaped extraction path: %s", file_path)
18241850
raise ValueError(f"Extracted path outside expected directory: {file_path}")
18251851

@@ -1843,7 +1869,10 @@ def custom_extractall_tarfile(tar, extract_path):
18431869
if hasattr(tarfile, "data_filter"):
18441870
tar.extractall(path=extract_path, filter="data")
18451871
else:
1846-
tar.extractall(path=extract_path, members=_get_safe_members(tar))
1872+
# Members are resolved against the directory they are extracted into, so that is
1873+
# what containment has to be checked against.
1874+
base = _get_resolved_path(extract_path)
1875+
tar.extractall(path=extract_path, members=_get_safe_members(tar.getmembers(), base))
18471876
# Re-validate extracted paths to catch symlink race conditions
18481877
_validate_extracted_paths(extract_path)
18491878

‎sagemaker-core/tests/unit/test_common_utils.py‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,6 +1228,173 @@ def test_custom_extractall_tarfile_basic(self, tmp_path):
12281228
assert (extract_path / "file.txt").exists()
12291229

12301230

1231+
class TestTarExtractionPathTraversal:
1232+
"""Regression tests for path traversal in the pre-3.12 tar extraction fallback.
1233+
1234+
The fallback branch of custom_extractall_tarfile runs only when
1235+
tarfile.data_filter is unavailable (Python < 3.12, before the 3.9.17 / 3.10.12 /
1236+
3.11.4 backports). These tests force that branch so the member filtering is
1237+
exercised regardless of the interpreter the suite runs on.
1238+
"""
1239+
1240+
@staticmethod
1241+
def _no_data_filter():
1242+
"""Patch the module's tarfile reference with one that has no data_filter."""
1243+
from types import SimpleNamespace
1244+
1245+
return patch("sagemaker.core.common_utils.tarfile", SimpleNamespace())
1246+
1247+
@staticmethod
1248+
def _tar_with_member(tar_path, member_name, content=b"pwned"):
1249+
"""Write a tar archive containing a single member under an arbitrary name."""
1250+
import io
1251+
1252+
with tarfile.open(tar_path, "w:gz") as tar:
1253+
info = tarfile.TarInfo(name=member_name)
1254+
info.size = len(content)
1255+
tar.addfile(info, io.BytesIO(content))
1256+
1257+
def test_is_within_base_containment(self, tmp_path):
1258+
"""_is_within_base accepts the base and nested paths, rejects outside paths."""
1259+
from sagemaker.core.common_utils import _get_resolved_path, _is_within_base
1260+
1261+
base = _get_resolved_path(str(tmp_path / "extract"))
1262+
1263+
assert _is_within_base(base, base) is True
1264+
assert _is_within_base(_get_resolved_path(str(tmp_path / "extract" / "a")), base) is True
1265+
assert _is_within_base(_get_resolved_path(str(tmp_path / "other")), base) is False
1266+
1267+
def test_is_within_base_rejects_sibling_prefix(self, tmp_path):
1268+
"""A sibling directory sharing a textual prefix with base is not contained.
1269+
1270+
A plain startswith() comparison would accept "<base>-evil" because it is a
1271+
string prefix match.
1272+
"""
1273+
from sagemaker.core.common_utils import _get_resolved_path, _is_within_base
1274+
1275+
base = _get_resolved_path(str(tmp_path / "extract"))
1276+
sibling = _get_resolved_path(str(tmp_path / "extract-evil" / "f.txt"))
1277+
1278+
assert sibling.startswith(base) # the bug a prefix check would let through
1279+
assert _is_within_base(sibling, base) is False
1280+
1281+
def test_is_bad_path_rejects_sibling_prefix(self, tmp_path):
1282+
"""_is_bad_path blocks a member escaping into a prefix-sharing sibling dir."""
1283+
from sagemaker.core.common_utils import _get_resolved_path, _is_bad_path
1284+
1285+
base = _get_resolved_path(str(tmp_path / "extract"))
1286+
1287+
assert _is_bad_path("../extract-evil/f.txt", base) is True
1288+
1289+
def test_is_bad_path_rejects_absolute_member(self, tmp_path):
1290+
"""_is_bad_path blocks absolute member paths outright."""
1291+
from sagemaker.core.common_utils import _get_resolved_path, _is_bad_path
1292+
1293+
base = _get_resolved_path(str(tmp_path / "extract"))
1294+
1295+
assert _is_bad_path("/etc/passwd", base) is True
1296+
1297+
def test_is_bad_path_allows_nested_member(self, tmp_path):
1298+
"""_is_bad_path permits ordinary members nested under the base directory."""
1299+
from sagemaker.core.common_utils import _get_resolved_path, _is_bad_path
1300+
1301+
base = _get_resolved_path(str(tmp_path / "extract"))
1302+
1303+
assert _is_bad_path("code/inference.py", base) is False
1304+
1305+
def test_get_safe_members_filters_member_escaping_extract_path(self, tmp_path):
1306+
"""_get_safe_members blocks a member that escapes the base it is given."""
1307+
from sagemaker.core.common_utils import _get_resolved_path, _get_safe_members
1308+
1309+
base = _get_resolved_path(str(tmp_path / "target" / "extract"))
1310+
escaping = tarfile.TarInfo(name="../extract-evil/escaped.txt")
1311+
benign = tarfile.TarInfo(name="model.tar")
1312+
1313+
safe = list(_get_safe_members([escaping, benign], base))
1314+
1315+
assert [m.name for m in safe] == ["model.tar"]
1316+
1317+
def test_members_are_validated_against_extract_path(self, tmp_path):
1318+
"""Members must be validated against extract_path, not the working directory."""
1319+
from sagemaker.core.common_utils import _get_resolved_path, custom_extractall_tarfile
1320+
1321+
extract_path = tmp_path / "extract"
1322+
extract_path.mkdir()
1323+
mock_tar = Mock()
1324+
mock_tar.getmembers = Mock(return_value=[])
1325+
1326+
with self._no_data_filter():
1327+
with patch("sagemaker.core.common_utils._get_safe_members") as mock_safe:
1328+
mock_safe.return_value = []
1329+
custom_extractall_tarfile(mock_tar, str(extract_path))
1330+
1331+
assert mock_safe.call_args[0][1] == _get_resolved_path(str(extract_path))
1332+
1333+
def test_fallback_extraction_blocks_escape_outside_extract_path(self, tmp_path, monkeypatch):
1334+
"""End-to-end: a crafted member must not be written outside extract_path.
1335+
1336+
Mirrors the reported proof of concept. The member escapes into a directory whose
1337+
name shares a textual prefix with the working directory's path
1338+
("<tmp>/work" vs "<tmp>/workevil"), so a startswith() check anchored to the
1339+
working directory accepts it while extraction still writes it outside
1340+
extract_path. _validate_extracted_paths only walks extract_path, so it does not
1341+
catch the escape either.
1342+
"""
1343+
from sagemaker.core.common_utils import custom_extractall_tarfile
1344+
1345+
cwd = tmp_path / "work"
1346+
cwd.mkdir()
1347+
monkeypatch.chdir(cwd)
1348+
1349+
extract_path = tmp_path / "target" / "extract"
1350+
extract_path.mkdir(parents=True)
1351+
1352+
tar_path = tmp_path / "malicious.tar.gz"
1353+
self._tar_with_member(tar_path, "../workevil/escaped.txt")
1354+
1355+
escaped = tmp_path / "target" / "workevil" / "escaped.txt"
1356+
1357+
with tarfile.open(tar_path, "r:gz") as tar:
1358+
with self._no_data_filter():
1359+
custom_extractall_tarfile(tar, str(extract_path))
1360+
1361+
assert not escaped.exists(), "member escaped the extraction directory"
1362+
assert list(extract_path.rglob("*")) == []
1363+
1364+
def test_fallback_extraction_blocks_absolute_member(self, tmp_path):
1365+
"""An absolute member path must not be written to its absolute location."""
1366+
from sagemaker.core.common_utils import custom_extractall_tarfile
1367+
1368+
outside = tmp_path / "absolute_target.txt"
1369+
extract_path = tmp_path / "extract"
1370+
extract_path.mkdir()
1371+
1372+
tar_path = tmp_path / "absolute.tar.gz"
1373+
self._tar_with_member(tar_path, str(outside))
1374+
1375+
with tarfile.open(tar_path, "r:gz") as tar:
1376+
with self._no_data_filter():
1377+
custom_extractall_tarfile(tar, str(extract_path))
1378+
1379+
assert not outside.exists()
1380+
1381+
def test_fallback_extraction_allows_benign_archive(self, tmp_path):
1382+
"""The fallback branch still extracts legitimate nested members."""
1383+
from sagemaker.core.common_utils import custom_extractall_tarfile
1384+
1385+
extract_path = tmp_path / "extract"
1386+
extract_path.mkdir()
1387+
1388+
tar_path = tmp_path / "benign.tar.gz"
1389+
self._tar_with_member(tar_path, "code/inference.py", content=b"print('hi')")
1390+
1391+
with tarfile.open(tar_path, "r:gz") as tar:
1392+
with self._no_data_filter():
1393+
custom_extractall_tarfile(tar, str(extract_path))
1394+
1395+
assert (extract_path / "code" / "inference.py").read_bytes() == b"print('hi')"
1396+
1397+
12311398
class TestCanModelPackageSourceUriAutopopulate:
12321399
"""Test can_model_package_source_uri_autopopulate function."""
12331400

0 commit comments

Comments
 (0)