fix: use filepath.EvalSymlinks to detect symlink chain escapes in archive extraction#565
Open
mwentzell-block wants to merge 2 commits intocashapp:masterfrom
Open
Conversation
…itizeSymlinkTarget PR cashapp#540 introduced sanitizeSymlinkTarget() using filepath.Clean to validate symlink targets during archive extraction. filepath.Clean is purely lexical and cannot detect a chain of symlinks where each individual hop appears safe but the full on-disk resolution escapes the extraction root. For example, a tarball containing: hop1 -> hop2 (lexically within dest — passes the Clean check) hop2 -> ../../escape (lexically: dest/hop2/../../escape — also passes) ...allows an attacker to write files outside the destination directory because the OS follows the full chain at write time. Fix: after the existing lexical check, resolve the parent directory of destFile using filepath.EvalSymlinks (which follows actual on-disk symlinks) and re-check the target against the extraction root. If EvalSymlinks fails (e.g. the parent doesn't exist yet), the lexical check alone applies. Adds TestSymlinkChainEscape to archive_test.go covering the two-hop tarball case.
Extract() returns "destination already exists" when dest is pre-created, causing the test to fail before reaching the symlink validation. The other traversal tests (TestLinkTraversal, TestLinkTraversalWithStrip) don't pre-create dest — bring TestSymlinkChainEscape in line with that pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #540 introduced
sanitizeSymlinkTarget()to prevent symlinks from escaping the extraction root during archive extraction. That check usesfilepath.Clean, which is purely lexical — it normalizes path strings but does not follow symlinks on disk.This leaves a chain escape bypass: a tarball can contain multiple symlinks where each individual hop looks safe under
filepath.Clean, but the full on-disk resolution escapes the extraction root.Example chain:
When
hop1is on disk and the OS follows both hops, a subsequent write throughhop1escapesdest.Fix
After the existing lexical check, resolve the parent directory of
destFileusingfilepath.EvalSymlinks(which follows actual on-disk symlinks) and re-validate the target against the extraction root. IfEvalSymlinksfails (parent doesn't exist yet during extraction), the lexical check alone applies — no regression for valid archives.Testing
Adds
TestSymlinkChainEscapetoarchive_test.go: builds a two-hop tarball in-memory and asserts that extraction fails with anillegal symlink targeterror and that no file is written outside the destination directory.Existing
TestLinkTraversalandTestLinkTraversalWithStriptests continue to pass unchanged.Reported via Block's vulnerability management program (VULN-78098). Bypass of #540.