Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tar path traversal through symlinks #396

Open
wants to merge 4 commits into
base: v3-deprecated
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/macos-latest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:

strategy:
matrix:
go-version: [1.13, 1.17]
go-version: [1.21]
runs-on: macos-latest
steps:
- name: Install Go
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ubuntu-latest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:

strategy:
matrix:
go-version: [1.13, 1.17]
go-version: [1.21]
runs-on: ubuntu-latest
steps:
- name: Install Go
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/windows-latest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:

strategy:
matrix:
go-version: [1.13, 1.17]
go-version: [1.21]
runs-on: windows-latest
steps:
- name: Install Go
Expand Down
18 changes: 18 additions & 0 deletions tar.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,24 @@ func (t *Tar) untarNext(destination string) error {
return fmt.Errorf("checking path traversal attempt: %v", errPath)
}

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wagoodman What do you think?

if errPath != nil {
return fmt.Errorf("checking path traversal attempt in symlink: %w", errPath)
}
}

if t.StripComponents > 0 {
if strings.Count(header.Name, "/") < t.StripComponents {
return nil // skip path with fewer components
Expand Down
96 changes: 95 additions & 1 deletion tar_test.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
package archiver_test

import (
"archive/tar"
"bytes"
"io/ioutil"
"os"
"path"
"path/filepath"
"runtime"
"testing"

"github.com/mholt/archiver/v3"
)

func requireDoesNotExist(t *testing.T, path string) {
_, 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)
}
Expand Down Expand Up @@ -47,6 +58,89 @@ 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, "./../target")
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", "duplicatedentry.txt"))
}

func TestDefaultTar_Unarchive_SymlinkPathTraversal_AbsLinkDestination(t *testing.T) {
tmp := t.TempDir()
source := filepath.Join(tmp, "source.tar")

linkPath := "/tmp/thing"
if runtime.GOOS == "windows" {
linkPath = "C:\\tmp\\thing"
}

createSymlinkPathTraversalSample(t, source, linkPath)
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 {
Name string
Link string
Body string
Type byte
}

var infos = []tarinfo{
{"duplicatedentry.txt", linkPath, "", tar.TypeSymlink},
{"duplicatedentry.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"

Expand Down