Repository navigation
pip 26.0.1.post1 follow-up: let a symlink resolve to an explicit archive root - #9
Conversation
_tar_member_paths never recorded ".", so is_symlink_target_in_tar refused a
symlink that resolves to the archive root even when the archive has an
explicit "." member. A "tar -C dir ." archive containing
"./sub/root -> .." extracted before this review round (tar.getmember(".")
found the member) and on upstream 26.2.2, but failed with InstallationError
after the member-name normalization change.
Record "." only when a member normalizes to it, never implicitly, which
matches the previous behaviour and upstream: without an explicit "." member
the link is still refused. No security impact: this only removes a false
rejection, and the realpath containment checks are unchanged (a link to the
root resolves to the extract directory itself, which is inside it).
Adds tests for the explicit-root (accepted) and implicit-root (refused)
cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ezequielp-activestate
left a comment
ezequielp-activestate
left a comment
There was a problem hiding this comment.
Follow-up review (40f50f2): 0 blockers. Approving.
The new commit fixes the nit from my last review. An explicit . / ./ member is now recorded as the archive root in _tar_member_paths (src/pip/_internal/utils/unpacking.py), so a symlink that resolves to it is accepted. The root is still never added implicitly. The source change is the same 6 lines on all five lines (pip PRs 4, 5, 6, 8 and 9).
Allowing . doesn't reopen the tar escape. I ran the exploit and legit-archive probe on this commit under Python 3.9, with tarfile.data_filter removed:
- All 7 escape shapes are still refused and nothing is written outside. That includes Case B (
.,redir -> .,redir/up -> ..,redir/up/outside.txt), which the realpath check now stops atredir/up. - Three extra variants that combine an explicit
.member with symlink chains (root -> .+root/up -> ..;sub/r -> ..+sub/r/../outside.txt;a -> .+a/a/a/b -> ..) are refused, with nothing written outside. - Legit archives L1–L4 still extract, and the new case (
.,./sub,./sub/root -> ..) now extracts.
Tests: tests/unit/test_utils_unpacking.py gives 68 passed, 1 skipped on Python 3.9. test_symlink_to_explicit_archive_root_is_in_tar fails on the previous code (assert '.' in {'sub', 'sub/root'}), checked on the 25.0.1 and 26.0.1 lines.
tests/unit suite (this commit only touches unpacking.py, whose tests were run).
Follow-up to #7, which merged before this review nit was addressed. It is not a new release:
26.0.1.post1is not tagged yet, so this lands in the same version.The nit:
_tar_member_pathsnever recorded".", sois_symlink_target_in_tarrefused a symlink resolving to the archive root even when the archive has an explicit.member. Atar -C dir .archive containing./sub/root -> ..extracted before the review round and on upstream 26.2.2, but failed withInstallationErrorafter the member-name normalization change.The fix: record
"."only when a member normalizes to it, never implicitly. That matches the earlier behaviour and upstream: without an explicit.member, the link is still refused. There is no security impact, since this only removes a false rejection and the realpath containment checks are unchanged.Verified:
./+./sub/root -> .../member,sub/root -> ..escaped=False)../-prefixed, implied-directory and ordinary-symlink archives still unpack.test_utils_unpacking.pypasses (68 passed, Python 3.9), and the new explicit-root test fails with the fix reverted.The same commit has gone onto the three open PRs (#4, #5, #6).
🤖 Generated with Claude Code