Fix zipslip vulnerability

evil_generator/main.go

@@ -0,0 +1,106 @@
+// This utility is used to generate the archives used as testdata for zipslip vulnerability
+package main
+
+//go:generate go run main.go ../testdata/zipslip
+
+import (
+	"archive/tar"
+	"archive/zip"
+	"bytes"
+	"log"
+	"os"
+
+	"github.com/arduino/go-paths-helper"
+)
+
+func main() {
+	if len(os.Args) != 2 {
+		log.Fatal("Missing output directory")
+	}
+	outputDir := paths.New(os.Args[1])
+	if outputDir.IsNotDir() {
+		log.Fatalf("Output path %s is not a directory", outputDir)
+	}
+
+	evilPathTraversalFiles := []string{
+		"..",
+		"../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"/some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+	}
+	winSpecificPathTraversalFiles := []string{
+		"..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+		"some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+		"\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+		"\\some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+	}
+	winSpecificPathTraversalFiles = append(winSpecificPathTraversalFiles, evilPathTraversalFiles...)
+
+	// Generate evil zip
+	{
+		buf := new(bytes.Buffer)
+		w := zip.NewWriter(buf)
+		for _, file := range winSpecificPathTraversalFiles {
+			if f, err := w.Create(file); err != nil {
+				log.Fatal(err)
+			} else if _, err = f.Write([]byte("TEST")); err != nil {
+				log.Fatal(err)
+			}
+		}
+		if err := w.Close(); err != nil {
+			log.Fatal(err)
+		}
+		if err := outputDir.Join("evil.zip").WriteFile(buf.Bytes()); err != nil {
+			log.Fatal(err)
+		}
+	}
+
+	// Generate evil tar
+	{
+		buf := new(bytes.Buffer)
+		w := tar.NewWriter(buf)
+		for _, file := range evilPathTraversalFiles {
+			if err := w.WriteHeader(&tar.Header{
+				Name: file,
+				Size: 4,
+				Mode: 0666,
+			}); err != nil {
+				log.Fatal(err)
+			}
+			if _, err := w.Write([]byte("TEST")); err != nil {
+				log.Fatal(err)
+			}
+		}
+		if err := w.Close(); err != nil {
+			log.Fatal(err)
+		}
+		if err := outputDir.Join("evil.tar").WriteFile(buf.Bytes()); err != nil {
+			log.Fatal(err)
+		}
+	}
+
+	// Generate evil tar for windows
+	{
+		buf := new(bytes.Buffer)
+		w := tar.NewWriter(buf)
+		for _, file := range winSpecificPathTraversalFiles {
+			if err := w.WriteHeader(&tar.Header{
+				Name: file,
+				Size: 4,
+				Mode: 0666,
+			}); err != nil {
+				log.Fatal(err)
+			}
+			if _, err := w.Write([]byte("TEST")); err != nil {
+				log.Fatal(err)
+			}
+		}
+		if err := w.Close(); err != nil {
+			log.Fatal(err)
+		}
+		if err := outputDir.Join("evil-win.tar").WriteFile(buf.Bytes()); err != nil {
+			log.Fatal(err)
+		}
+	}
+}

extractor.go

@@ -132,7 +132,10 @@ func (e *Extractor) Tar(ctx context.Context, body io.Reader, location string, re
 			continue
 		}
 
-		path = filepath.Join(location, path)
+		if path, err = safeJoin(location, path); err != nil {
+			continue
+		}
+
 		info := header.FileInfo()
 
 		switch header.Typeflag {
@@ -232,7 +235,10 @@ func (e *Extractor) Zip(ctx context.Context, body io.Reader, location string, re
 			continue
 		}
 
-		path = filepath.Join(location, path)
+		if path, err = safeJoin(location, path); err != nil {
+			continue
+		}
+
 		info := header.FileInfo()
 
 		switch {
@@ -316,3 +322,16 @@ func match(r io.Reader) (io.Reader, types.Type, error) {
 
 	return r, typ, err
 }
+
+// safeJoin performs a filepath.Join of 'parent' and 'subdir' but returns an error
+// if the resulting path points outside of 'parent'.
+func safeJoin(parent, subdir string) (string, error) {
+	res := filepath.Join(parent, subdir)
+	if !strings.HasSuffix(parent, string(os.PathSeparator)) {
+		parent += string(os.PathSeparator)
+	}
+	if !strings.HasPrefix(res, parent) {
+		return res, errors.Errorf("unsafe path join: '%s' with '%s'", parent, subdir)
+	}
+	return res, nil
+}

extractor_test.go

@@ -3,12 +3,15 @@ package extract_test
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/codeclysm/extract/v3"
+	"github.com/stretchr/testify/require"
 )
 
 func TestExtractor_Tar(t *testing.T) {
@@ -77,6 +80,40 @@ func TestExtractor_Zip(t *testing.T) {
 	testWalk(t, tmp, files)
 }
 
+func TestZipSlipHardening(t *testing.T) {
+	{
+		logger := &LoggingFS{}
+		extractor := extract.Extractor{FS: logger}
+		data, err := os.Open("testdata/zipslip/evil.zip")
+		require.NoError(t, err)
+		require.NoError(t, extractor.Zip(context.Background(), data, "/tmp/test", nil))
+		require.NoError(t, data.Close())
+		fmt.Print(logger)
+		require.Empty(t, logger.Journal)
+	}
+	{
+		logger := &LoggingFS{}
+		extractor := extract.Extractor{FS: logger}
+		data, err := os.Open("testdata/zipslip/evil.tar")
+		require.NoError(t, err)
+		require.NoError(t, extractor.Tar(context.Background(), data, "/tmp/test", nil))
+		require.NoError(t, data.Close())
+		fmt.Print(logger)
+		require.Empty(t, logger.Journal)
+	}
+
+	if runtime.GOOS == "windows" {
+		logger := &LoggingFS{}
+		extractor := extract.Extractor{FS: logger}
+		data, err := os.Open("testdata/zipslip/evil-win.tar")
+		require.NoError(t, err)
+		require.NoError(t, extractor.Tar(context.Background(), data, "/tmp/test", nil))
+		require.NoError(t, data.Close())
+		fmt.Print(logger)
+		require.Empty(t, logger.Journal)
+	}
+}
+
 // MockDisk is a disk that chroots to a directory
 type MockDisk struct {
 	Base string

go.mod

@@ -3,6 +3,7 @@ module github.com/codeclysm/extract/v3
 go 1.14
 
 require (
+	github.com/arduino/go-paths-helper v1.2.0
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/h2non/filetype v1.0.6
 	github.com/juju/errors v0.0.0-20181118221551-089d3ea4e4d5

go.sum

@@ -1,3 +1,5 @@
+github.com/arduino/go-paths-helper v1.2.0 h1:qDW93PR5IZUN/jzO4rCtexiwF8P4OIcOmcSgAYLZfY4=
+github.com/arduino/go-paths-helper v1.2.0/go.mod h1:HpxtKph+g238EJHq4geEPv9p+gl3v5YYu35Yb+w31Ck=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=

loggingfs_test.go

@@ -0,0 +1,80 @@
+package extract_test
+
+import (
+	"fmt"
+	"os"
+)
+
+// LoggingFS is a disk that logs every operation, useful for unit-testing.
+type LoggingFS struct {
+	Journal []*LoggedOp
+}
+
+// LoggedOp is an operation logged in a LoggingFS journal.
+type LoggedOp struct {
+	Op      string
+	Path    string
+	OldPath string
+	Mode    os.FileMode
+	Flags   int
+}
+
+func (op *LoggedOp) String() string {
+	switch op.Op {
+	case "link":
+		return fmt.Sprintf("link     %s -> %s", op.Path, op.OldPath)
+	case "symlink":
+		return fmt.Sprintf("symlink  %s -> %s", op.Path, op.OldPath)
+	case "mkdirall":
+		return fmt.Sprintf("mkdirall %v %s", op.Mode, op.Path)
+	case "open":
+		return fmt.Sprintf("open     %v %s (flags=%04x)", op.Mode, op.Path, op.Flags)
+	}
+	panic("unknown LoggedOP " + op.Op)
+}
+
+func (m *LoggingFS) Link(oldname, newname string) error {
+	m.Journal = append(m.Journal, &LoggedOp{
+		Op:      "link",
+		OldPath: oldname,
+		Path:    newname,
+	})
+	return nil
+}
+
+func (m *LoggingFS) MkdirAll(path string, perm os.FileMode) error {
+	m.Journal = append(m.Journal, &LoggedOp{
+		Op:   "mkdirall",
+		Path: path,
+		Mode: perm,
+	})
+	return nil
+}
+
+func (m *LoggingFS) Symlink(oldname, newname string) error {
+	m.Journal = append(m.Journal, &LoggedOp{
+		Op:      "symlink",
+		OldPath: oldname,
+		Path:    newname,
+	})
+	return nil
+}
+
+func (m *LoggingFS) OpenFile(name string, flags int, perm os.FileMode) (*os.File, error) {
+	m.Journal = append(m.Journal, &LoggedOp{
+		Op:    "open",
+		Path:  name,
+		Mode:  perm,
+		Flags: flags,
+	})
+	return os.OpenFile(os.DevNull, flags, perm)
+}
+
+func (m *LoggingFS) String() string {
+	res := ""
+	for _, op := range m.Journal {
+		res += op.String()
+		res += "\n"
+	}
+	return res
+}

safejoin_test.go

@@ -0,0 +1,35 @@
+package extract
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSafeJoin(t *testing.T) {
+	ok := func(parent, subdir string) {
+		_, err := safeJoin(parent, subdir)
+		require.NoError(t, err, "joining '%s' and '%s'", parent, subdir)
+	}
+	ko := func(parent, subdir string) {
+		_, err := safeJoin(parent, subdir)
+		require.Error(t, err, "joining '%s' and '%s'", parent, subdir)
+	}
+	ok("/", "more/path")
+	ok("/path", "more/path")
+	ok("/path/", "more/path")
+	ok("/path/subdir", "more/path")
+	ok("/path/subdir/", "more/path")
+
+	ok("/", "..") // ! since we are extracting to / is ok-ish to accept ".."?
+	ko("/path", "..")
+	ko("/path/", "..")
+	ko("/path/subdir", "..")
+	ko("/path/subdir/", "..")
+
+	ok("/", "../pathpath") // ! since we are extracting to / is ok-ish to accept "../pathpath"?
+	ko("/path", "../pathpath")
+	ko("/path/", "../pathpath")
+	ko("/path/subdir", "../pathpath")
+	ko("/path/subdir/", "../pathpath")
+}