archiver
archiver copied to clipboard
mholt
Fix tar path traversal through symlinks
This PR addresses behavior of archiver.Tar.Unarchive() described in CVE-2024-0406, specifically in two cases:
Case 1
When a tar contains two header entries for the same file:
- the first entry, with path
./x, is a symlink that points relatively outside of the unarchive destination (e.g.../../../here) - the second entry, also with path
./x, is a regular file
This will result in the symlink being created in the first pass, then the second entry writing contents to a potentially new file (or overwrite an existing file) outside of the unarchive destination
Case 2
When a tar contains a link that points to an absolute path (e.g. /bin/here). In this case it is unlikely that this path is within the unarchive destination.
Changes
This PR changes the behavior by:
- not writing out symlinks with link destination names that are absolute paths
- not writing out symlinks with link destination names that are relative paths that resolve to outside of the unarchive directory
It seems your patch requires Go 1.15, can you please update the CI workflow to use Go 1.15 (multiple versions not necessary anymore at this point)
not a problem -- I left the test matrix in place in case anyone wants to test multiple versions again but restricted this to v1.15.
Thanks, that's exactly what I was thinking.
I'm not entirely sure why the tests are failing now, but it looks like one of the deps doesn't build on Mac -- that one is likely unrelated to this change.
I've bumped to go 1.21, but have low hopes. The windows test failure seems to be a separate issue:
=== RUN TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination
tar_test.go:83: unarchiving 'C:\Users\RUNNER~1\AppData\Local\Temp\TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination313487669\001\source.tar' to 'C:\Users\RUNNER~1\AppData\Local\Temp\TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination313487669\001\destination': reading file in tar archive: C:\Users\RUNNER~1\AppData\Local\Temp\TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination313487669\001\destination\duplicatedentry.txt: creating new file: open C:\Users\RUNNER~1\AppData\Local\Temp\TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination313487669\001\destination\duplicatedentry.txt: The system cannot find the path specified.
--- FAIL: TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination (0.00s)
Yeah, probably unrelated. I haven't touched this code in years, so it doesn't surprise me. I'll try to investigate when I get back from an errand.
(I imagine there's no need to require a Go version bump on code that is ~5 years old though)
Ok so one of the tests is failing on Windows because the path described in the unarchival operation doesn't exist in the CI environment.
Once that is fixed, we can probably merge this.
Also maybe we can downgrade Go again to 1.15 or so, I don't think ~5 year old code needs to be requiring the most recent Go.
Hello,
Is there anything blocking this PR?
Could that be released in a new version (v3.5.2) ?
Thanks!
Is there anything blocking this PR?
I just came here to see if anything was blocking a new release and it's the failing Windows test.
The PR author needs to address that issue first.
Looks like something to do with the line:
createSymlinkPathTraversalSample(t, source, "./../target")
I'm presuming that needs a path.Join() instead?
Hi @wagoodman, just wondering if you still have plans to keep working on this CVE fix. It seems like the Windows test is failing but someone suggested a solution above.
Sorry, I thought this was already merged 😳 I just pushed a test update @mholt (the issue was that the test was expressing an absolute path, but only for unix systems, not windows).
Running into issues with govulncheck, if this PR is stale perhaps someone else can take over? @wagoodman
