Fix tar path traversal through symlinks

[wagoodman]

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:

1. the first entry, with path ./x, is a symlink that points relatively outside of the unarchive destination (e.g. ../../../here)
2. 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

[mholt]

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)

[wagoodman]

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.

[mholt]

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.

[wagoodman]

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)

[mholt]

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)

[mholt]

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.

[gnuletik]

Hello,
Is there anything blocking this PR?
Could that be released in a new version (v3.5.2) ?
Thanks!

[Integralist]

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?

[ericpang777]

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.

[wagoodman]

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).

[McTonderski]

Hey @mholt could you take a look at this PR ?

[MAGNUSGAO]

Hey @mholt would you mind taking a look at this PR? Thank you!

[McTonderski]

Any chance @mholt to look at it ?

[k4leung4]

FYI, a workaround in the mean time, github.com/anchore/archiver/[email protected]

[earl-warren]

#404 is related?

[devhaozi]

Why this PR not been merged? @mholt can you take a look at it?

[magnusbaeck]

I don't think this is correct. We're supposed to be checking if the symlink target escapes from destination, but we're not considering that header.Linkname is relative to its parent directory. Thus we're currently not only ignoring a symlink.txt entry that links to ../foo but also a subdir/symlink.txt entry with the same link target. Such a symlink resolves to foo which is inside destination.

The following patch illustrates this. The tests are still passing, i.e. the target file is ignored even though it's supposed to be kept.

diff --git a/tar_test.go b/tar_test.go
index 683308e..f527295 100644
--- a/tar_test.go
+++ b/tar_test.go
@@ -69,8 +69,8 @@ func TestDefaultTar_Unarchive_SymlinkPathTraversal(t *testing.T) {
                t.Fatalf("unarchiving '%s' to '%s': %v", source, destination, err)
        }
 
-       requireDoesNotExist(t, filepath.Join(tmp, "target"))
-       requireRegularFile(t, filepath.Join(tmp, "destination", "duplicatedentry.txt"))
+       requireDoesNotExist(t, filepath.Join(tmp, "destination", "target"))
+       requireRegularFile(t, filepath.Join(tmp, "destination", "foo", "duplicatedentry.txt"))
 }
 
 func TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination(t *testing.T) {
@@ -91,7 +91,7 @@ func TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination(t *testing
        }
 
        requireDoesNotExist(t, "/tmp/thing")
-       requireRegularFile(t, filepath.Join(tmp, "destination", "duplicatedentry.txt"))
+       requireRegularFile(t, filepath.Join(tmp, "destination", "foo", "duplicatedentry.txt"))
 }
 
 func createSymlinkPathTraversalSample(t *testing.T, archivePath string, linkPath string) {
@@ -105,8 +105,8 @@ func createSymlinkPathTraversalSample(t *testing.T, archivePath string, linkPath
        }
 
        var infos = []tarinfo{
-               {"duplicatedentry.txt", linkPath, "", tar.TypeSymlink},
-               {"duplicatedentry.txt", "", "content modified!", tar.TypeReg},
+               {"foo/duplicatedentry.txt", linkPath, "", tar.TypeSymlink},
+               {"foo/duplicatedentry.txt", "", "content modified!", tar.TypeReg},
        }
 
        var buf bytes.Buffer

[ericpang777]

@wagoodman What do you think?

[jsirianni]

Running into issues with govulncheck, if this PR is stale perhaps someone else can take over? @wagoodman

[mholt]

Closing due to inactivity (OP has not responded), and this repo is being archived in favor of mholt/archives.