From a846133a8f7bc8ff33f1cd5eb5ec16cadb9052d7 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Tue, 30 Jan 2024 12:44:34 -0500 Subject: [PATCH 1/2] fix tar path traversal through symlinks Signed-off-by: Alex Goodman --- tar.go | 12 +++++++++ tar_test.go | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/tar.go b/tar.go index be898665..45be6ca0 100644 --- a/tar.go +++ b/tar.go @@ -238,6 +238,18 @@ func (t *Tar) untarNext(destination string) error { return fmt.Errorf("checking path traversal attempt: %v", errPath) } + switch header.Typeflag { + case tar.TypeSymlink, tar.TypeLink: + // though we've already checked the name for possible path traversals, it is possible + // to write content though a symlink to a path outside of the destination folder + // with multiple header entries. We should consider any symlink or hardlink that points + // to outside of the destination folder to be a possible path traversal attack. + errPath = t.CheckPath(destination, header.Linkname) + if errPath != nil { + return fmt.Errorf("checking path traversal attempt in symlink: %v", errPath) + } + } + if t.StripComponents > 0 { if strings.Count(header.Name, "/") < t.StripComponents { return nil // skip path with fewer components diff --git a/tar_test.go b/tar_test.go index 7a9d3541..7ad43d29 100644 --- a/tar_test.go +++ b/tar_test.go @@ -1,14 +1,24 @@ package archiver_test import ( + "archive/tar" + "bytes" "io/ioutil" "os" "path" + "path/filepath" "testing" "github.com/mholt/archiver/v3" ) +func requireDoesNotExist(t *testing.T, path string) { + _, err := os.Stat(path) + if err == nil { + t.Fatalf("'%s' expected to not exist", path) + } +} + func requireRegularFile(t *testing.T, path string) os.FileInfo { fileInfo, err := os.Stat(path) if err != nil { @@ -47,6 +57,68 @@ func TestDefaultTar_Unarchive_HardlinkSuccess(t *testing.T) { assertSameFile(t, fileaInfo, filebInfo) } +func TestDefaultTar_Unarchive_SymlinkPathTraversal(t *testing.T) { + tmp := t.TempDir() + source := filepath.Join(tmp, "source.tar") + createSymlinkPathTraversalSample(t, source) + destination := filepath.Join(tmp, "destination") + + err := archiver.DefaultTar.Unarchive(source, destination) + if err != nil { + t.Fatalf("unarchiving '%s' to '%s': %v", source, destination, err) + } + + requireDoesNotExist(t, filepath.Join(tmp, "target")) + requireRegularFile(t, filepath.Join(tmp, "destination", "symlinkvehicle.txt")) +} + +func createSymlinkPathTraversalSample(t *testing.T, archivePath string) { + t.Helper() + + type tarinfo struct { + Name string + Link string + Body string + Type byte + } + + var infos = []tarinfo{ + {"symlinkvehicle.txt", "./../target", "", tar.TypeSymlink}, + {"symlinkvehicle.txt", "", "content modified!", tar.TypeReg}, + } + + var buf bytes.Buffer + var tw = tar.NewWriter(&buf) + for _, ti := range infos { + hdr := &tar.Header{ + Name: ti.Name, + Mode: 0600, + Linkname: ti.Link, + Typeflag: ti.Type, + Size: int64(len(ti.Body)), + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal("Writing header: ", err) + } + if _, err := tw.Write([]byte(ti.Body)); err != nil { + t.Fatal("Writing body: ", err) + } + } + + f, err := os.Create(archivePath) + if err != nil { + t.Fatal(err) + } + _, err = f.Write(buf.Bytes()) + if err != nil { + t.Fatal(err) + } + + if err := f.Close(); err != nil { + t.Fatal(err) + } +} + func TestDefaultTar_Extract_HardlinkSuccess(t *testing.T) { source := "testdata/gnu-hardlinks.tar" From b27fbecc26119d67af8283f90ca3262e0f4d2c8c Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Wed, 31 Jan 2024 10:25:30 -0500 Subject: [PATCH 2/2] address absolute symlink destinations Signed-off-by: Alex Goodman --- tar.go | 8 +++++++- tar_test.go | 29 ++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/tar.go b/tar.go index 45be6ca0..d5820114 100644 --- a/tar.go +++ b/tar.go @@ -240,13 +240,19 @@ func (t *Tar) untarNext(destination string) error { switch header.Typeflag { case tar.TypeSymlink, tar.TypeLink: + // this covers cases when the link is an absolute path to a file outside the destination folder + if filepath.IsAbs(header.Linkname) { + errPath := &IllegalPathError{AbsolutePath: "", Filename: header.Linkname} + return fmt.Errorf("absolute path for symlink destination not allowed: %w", errPath) + } + // though we've already checked the name for possible path traversals, it is possible // to write content though a symlink to a path outside of the destination folder // with multiple header entries. We should consider any symlink or hardlink that points // to outside of the destination folder to be a possible path traversal attack. errPath = t.CheckPath(destination, header.Linkname) if errPath != nil { - return fmt.Errorf("checking path traversal attempt in symlink: %v", errPath) + return fmt.Errorf("checking path traversal attempt in symlink: %w", errPath) } } diff --git a/tar_test.go b/tar_test.go index 7ad43d29..c365a6d6 100644 --- a/tar_test.go +++ b/tar_test.go @@ -13,14 +13,14 @@ import ( ) func requireDoesNotExist(t *testing.T, path string) { - _, err := os.Stat(path) + _, err := os.Lstat(path) if err == nil { t.Fatalf("'%s' expected to not exist", path) } } func requireRegularFile(t *testing.T, path string) os.FileInfo { - fileInfo, err := os.Stat(path) + fileInfo, err := os.Lstat(path) if err != nil { t.Fatalf("fileInfo on '%s': %v", path, err) } @@ -60,7 +60,7 @@ func TestDefaultTar_Unarchive_HardlinkSuccess(t *testing.T) { func TestDefaultTar_Unarchive_SymlinkPathTraversal(t *testing.T) { tmp := t.TempDir() source := filepath.Join(tmp, "source.tar") - createSymlinkPathTraversalSample(t, source) + createSymlinkPathTraversalSample(t, source, "./../target") destination := filepath.Join(tmp, "destination") err := archiver.DefaultTar.Unarchive(source, destination) @@ -69,10 +69,25 @@ func TestDefaultTar_Unarchive_SymlinkPathTraversal(t *testing.T) { } requireDoesNotExist(t, filepath.Join(tmp, "target")) - requireRegularFile(t, filepath.Join(tmp, "destination", "symlinkvehicle.txt")) + requireRegularFile(t, filepath.Join(tmp, "destination", "duplicatedentry.txt")) } -func createSymlinkPathTraversalSample(t *testing.T, archivePath string) { +func TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination(t *testing.T) { + tmp := t.TempDir() + source := filepath.Join(tmp, "source.tar") + createSymlinkPathTraversalSample(t, source, "/tmp/thing") + destination := filepath.Join(tmp, "destination") + + err := archiver.DefaultTar.Unarchive(source, destination) + if err != nil { + t.Fatalf("unarchiving '%s' to '%s': %v", source, destination, err) + } + + requireDoesNotExist(t, "/tmp/thing") + requireRegularFile(t, filepath.Join(tmp, "destination", "duplicatedentry.txt")) +} + +func createSymlinkPathTraversalSample(t *testing.T, archivePath string, linkPath string) { t.Helper() type tarinfo struct { @@ -83,8 +98,8 @@ func createSymlinkPathTraversalSample(t *testing.T, archivePath string) { } var infos = []tarinfo{ - {"symlinkvehicle.txt", "./../target", "", tar.TypeSymlink}, - {"symlinkvehicle.txt", "", "content modified!", tar.TypeReg}, + {"duplicatedentry.txt", linkPath, "", tar.TypeSymlink}, + {"duplicatedentry.txt", "", "content modified!", tar.TypeReg}, } var buf bytes.Buffer