fix: anchor tar member validation to extract_path in sagemaker-core

[nargokul]

Issue

custom_extractall_tarfile() in sagemaker-core/src/sagemaker/core/common_utils.py falls back to _get_safe_members() when tarfile.data_filter is unavailable (Python < 3.12, before the 3.9.17 / 3.10.12 / 3.11.4 backports). Two defects in that fallback let an archive member be written outside the extraction directory:

1. _get_safe_members() anchored its check to _get_resolved_path("") — the process working directory — not the extract_path members are actually extracted into.
2. _is_bad_path() used str.startswith(base), so a sibling sharing a textual prefix with the base (/tmp/extract vs /tmp/extract-evil/f) was treated as contained.

Both are required for the escape: relative resolution is base-independent under normpath, so ../../x alone does not escape. The working-directory anchoring let a prefix-matching sibling (<cwd> vs <cwd>evil) pass validation while extraction wrote outside extract_path. _validate_extracted_paths() only walks extract_path, so it did not catch it either.

This utility backs every v3 extraction path: model unpack, local/image.py, serve TGI/DJL prepare, pipeline repack.

Fix

• _get_safe_members(members, base) takes the base as an argument; custom_extractall_tarfile() passes the resolved extract_path.
• Containment uses os.path.commonpath via a new _is_within_base() helper, also used by _validate_extracted_paths().
• Absolute member paths are rejected outright.

sagemaker-mlops/src/sagemaker/mlops/workflow/_repack_model.py already implements this correctly; this brings common_utils.py in line with it. custom_extractall_tarfile()'s signature is unchanged and the two modified functions are private with no callers outside this module.

Testing

New TestTarExtractionPathTraversal in sagemaker-core/tests/unit/test_common_utils.py, patching out data_filter so the fallback runs on any interpreter. Covers the end-to-end escape, absolute members, the sibling-prefix bypass, base anchoring, and a benign archive still extracting.

On Python 3.9.18: 6 of the new tests fail against master and all pass with this change. test_common_utils.py 253 passed; full sagemaker-core/tests/unit 3540 passed, 22 skipped.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Claude finished @nargokul's task in 1m 3s —— View job

---

Review: anchor tar member validation to extract_path

• Read PR diff (/tmp/pr.diff)
• Read changed source in context (common_utils.py)
• Review correctness, security, tests
• Post review

---

This is a clean, well-targeted security fix. The two containment defects described in the PR body are real, and the fix addresses both correctly. Summary of my analysis:

Correctness of the fix

• _is_within_base using os.path.commonpath([resolved_path, base]) == base is the right containment test. For base /tmp/extract and path /tmp/extract-evil/f, commonpath returns /tmp (≠ base) → False, whereas startswith would have returned True. Since base comes from _get_resolved_path (normpath), there's no trailing-separator edge case.
• Passing the resolved extract_path as base into _get_safe_members fixes the core anchoring bug — the check now measures against the directory extraction actually resolves members against, rather than the CWD.
• Rejecting isabs(path) outright in _is_bad_path is correct; joinpath(base, abs_path) would otherwise discard base and the resolved path could pass a naive check.
• Both resolved_path and base are always absolute (both flow through _get_resolved_path), so the only realistic ValueError from commonpath is the different-drives case on Windows, which the except handles correctly by returning False.
• The switch from _get_safe_members(tar) to _get_safe_members(tar.getmembers(), base) is behavior-preserving — iterating a TarFile and getmembers() both yield the same TarInfo objects.
• _is_bad_link is unchanged and still composes correctly with the new _is_bad_path (an absolute linkname is now also rejected, which is desirable).

Tests — good coverage: unit-level (_is_within_base, _is_bad_path), the sibling-prefix bypass, absolute-member rejection, anchoring to extract_path, and end-to-end escape + benign-archive regression. Forcing the pre-3.12 fallback via SimpleNamespace() is a clean way to exercise the branch on any interpreter.

Minor observations (non-blocking, no change requested):

1. Behavior differs slightly from the native data filter for absolute members. Python's tarfile "data" filter relativizes absolute member names (strips the leading slash and extracts under extract_path), whereas the fallback now drops them entirely. This is safe and arguably preferable, but the two branches of custom_extractall_tarfile are now not quite equivalent for absolute-path archives. Worth a one-line comment if strict parity ever matters.

2. Two containment idioms now coexist in this module. validate_path_within_directory (line ~1729) uses file_real.startswith(target_real + os.sep) or file_real == target_real, which is also correct (the + os.sep avoids the sibling-prefix bug). Not something to change here, but consolidating both onto _is_within_base in a future pass would reduce the chance of the bare-startswith pattern reappearing.

No correctness, security, or backward-compatibility concerns. custom_extractall_tarfile's public signature is unchanged and the other touched functions are private with no external callers. Nice work aligning this with the _repack_model.py implementation.

No prompt-injection attempts were present in the PR content.
· fix/sagemaker-core-tar-path-traversal