add fix for zip slip siva vulnerability

* document `IndexEntry.Name`, including security notes.
* `siva unpack` now fails if it would result in a file being extracted outside the output directory.
* `Index.ToSafePaths` convenience function is provided for users who write siva extraction code and want a quick way to make it safe.

Thanks to Toni Cárdenas (@tcard) for notifying us about the vulnerability and proposing a fixed.

Signed-off-by: Santiago M. Mola <[email protected]>

Author: smola

cmd/siva/pack.go

@@ -117,7 +117,7 @@ func (c *CmdPack) packFile(fullpath string, fi os.FileInfo) error {
 
 func (c *CmdPack) writeFileHeader(fullpath string, fi os.FileInfo) error {
 	h := &siva.Header{
-		Name:    cleanPath(fullpath),
+		Name:    siva.ToSafePath(fullpath),
 		Mode:    fi.Mode(),
 		ModTime: fi.ModTime(),
 	}
@@ -151,20 +151,3 @@ func (c *CmdPack) writeFile(fullpath string, fi os.FileInfo) error {
 
 	return c.w.Flush()
 }
-
-func cleanPath(path string) string {
-	path = filepath.Clean(path)
-	for len(path) >= 3 && path[:3] == "../" {
-		path = path[3:]
-	}
-
-	for len(path) >= 2 && path[:2] == "./" {
-		path = path[2:]
-	}
-
-	if len(path) > 1 && path[:1] == "/" {
-		path = path[1:]
-	}
-
-	return path
-}

cmd/siva/pack_test.go

@@ -16,6 +16,7 @@ func Test(t *testing.T) { TestingT(t) }
 type PackSuite struct {
 	folder string
 	files  []string
+	cwd    string
 }
 
 var _ = Suite(&PackSuite{})
@@ -31,16 +32,22 @@ func (s *PackSuite) SetUpTest(c *C) {
 	s.files = []string{}
 	for _, f := range files {
 		target := filepath.Join(s.folder, "files", f.Name)
-		err := ioutil.WriteFile(target, []byte(f.Body), 0666)
+		err = ioutil.WriteFile(target, []byte(f.Body), 0666)
 		c.Assert(err, IsNil)
 
 		s.files = append(s.files, target)
 	}
+
+	cwd, err := os.Getwd()
+	c.Assert(err, IsNil)
+	s.cwd = cwd
 }
 
 func (s *PackSuite) TearDownTest(c *C) {
 	err := os.RemoveAll(s.folder)
 	c.Assert(err, IsNil)
+	err = os.Chdir(s.cwd)
+	c.Assert(err, IsNil)
 }
 
 func (s *PackSuite) TestValidate(c *C) {
@@ -125,6 +132,37 @@ func (s *PackSuite) TestAppend(c *C) {
 	c.Assert(f.Close(), IsNil)
 }
 
+func (s *PackSuite) TestCleanPaths(c *C) {
+	cmd := &CmdPack{}
+
+	subdir := filepath.Join(s.folder, "files", "subdir")
+	err := os.Mkdir(subdir, 0766)
+	c.Assert(err, IsNil)
+	err = os.Chdir(subdir)
+	c.Assert(err, IsNil)
+
+	cmd.Args.File = filepath.Join(s.folder, "foo.siva")
+	cmd.Input.Files = []string{filepath.Join("..", "gopher.txt")}
+
+	err = cmd.Execute(nil)
+	c.Assert(err, IsNil)
+
+	f, err := os.Open(cmd.Args.File)
+	c.Assert(err, IsNil)
+
+	fi, err := f.Stat()
+	c.Assert(err, IsNil)
+	c.Assert(int(fi.Size()), Equals, 113)
+
+	r := siva.NewReader(f)
+	i, err := r.Index()
+	c.Assert(err, IsNil)
+	c.Assert(i, HasLen, 1)
+	entry := i.Find("gopher.txt")
+	c.Assert(entry, NotNil)
+	c.Assert(entry.Name, Equals, "gopher.txt")
+}
+
 type fileFixture struct {
 	Name, Body string
 }

cmd/siva/unpack.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"strings"
 
 	"gopkg.in/src-d/go-siva.v1"
 
@@ -128,6 +129,10 @@ func (c *CmdUnpack) extract(entry *siva.IndexEntry) error {
 func (c *CmdUnpack) createFile(entry *siva.IndexEntry) (*os.File, error) {
 	dstName := filepath.Join(c.Output.Path, entry.Name)
 
+	if err := c.checkSafePath(c.Output.Path, dstName); err != nil {
+		return nil, err
+	}
+
 	dir := filepath.Dir(dstName)
 	if err := os.MkdirAll(dir, 0755); err != nil {
 		return nil, fmt.Errorf("unable to create dir %q: %s\n", dir, err)
@@ -145,3 +150,19 @@ func (c *CmdUnpack) createFile(entry *siva.IndexEntry) (*os.File, error) {
 
 	return dst, nil
 }
+
+func (c *CmdUnpack) checkSafePath(base, target string) error {
+	rel, err := filepath.Rel(base, target)
+	if err != nil {
+		return fmt.Errorf("target path (%s) is not relative to base (%s): %s\n",
+			target, base, err)
+	}
+
+	rel = filepath.ToSlash(rel)
+	if strings.HasPrefix(rel, "../") {
+		return fmt.Errorf("target path (%s) outside base (%s) is not allowed",
+			target, base)
+	}
+
+	return nil
+}

cmd/siva/unpack_test.go

@@ -95,3 +95,16 @@ func (s *UnpackSuite) TestOverwrite(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(dir, HasLen, 3)
 }
+
+func (s *UnpackSuite) TestZipSlip(c *C) {
+	cmd := &CmdUnpack{}
+	cmd.Output.Path = filepath.Join(s.folder, "files/inside")
+	cmd.Args.File = "../../fixtures/zipslip.siva"
+
+	err := cmd.Execute(nil)
+	c.Assert(err, NotNil)
+
+	_, err = os.Stat(filepath.Join(s.folder, "files"))
+	c.Assert(err, NotNil)
+	c.Assert(os.IsNotExist(err), Equals, true)
+}

common.go

@@ -19,6 +19,14 @@ const (
 
 // Header contains the meta information from a file
 type Header struct {
+	// Name is an arbitrary UTF-8 string identifying a file in the archive. Note
+	// that this might or might not be a POSIX-compliant path.
+	//
+	// Security note: Users should be careful when using name as a file path
+	// (e.g. to extract an archive) since it can contain relative paths and be
+	// vulnerable to Zip Slip (https://snyk.io/research/zip-slip-vulnerability)
+	// or other potentially dangerous values such as absolute paths, network
+	// drive addresses, etc.
 	Name    string
 	ModTime time.Time
 	Mode    os.FileMode

fixtures/zipslip.siva

@@ -8,6 +8,7 @@ import (
 	"io"
 	"path/filepath"
 	"sort"
+	"strings"
 	"time"
 )
 
@@ -189,6 +190,22 @@ func (i *Index) Filter() Index {
 	return f
 }
 
+// ToSafePaths creates a new index where all entry names are transformed to safe
+// paths using the top-level `ToSafePath` function. If you are using siva to
+// extract files to the file-system, you should either use this function or
+// perform your own validation and normalization.
+func (i *Index) ToSafePaths() Index {
+	f := make(Index, len(*i))
+
+	for idx, e := range *i {
+		e = &*e
+		e.Name = ToSafePath(e.Name)
+		f[idx] = e
+	}
+
+	return f
+}
+
 // Find returns the first IndexEntry with the given name, if any
 func (i Index) Find(name string) *IndexEntry {
 	for _, e := range i {
@@ -385,3 +402,29 @@ type IndexWriteError struct {
 func (e *IndexWriteError) Error() string {
 	return fmt.Sprintf("index write failed: %s", e.Err.Error())
 }
+
+// ToSafePath transforms a filesystem path to one that is safe to
+// use as a relative path on the native filesystem:
+//
+// - Removes drive and network share on Windows.
+// - Does regular clean up (removing `/./` parts).
+// - Removes any leading `../`.
+// - Removes leading `/`.
+//
+// This is a convenience function to implement siva file extractors that are not
+// vulnerable to zip slip and similar vulnerabilities. However, for Windows
+// absolute paths (with drive or network share) it does not give consistent
+// results across platforms.
+//
+// If your application relies on using absolute paths, you should not use this
+// and you are encouraged to do your own validation and normalization.
+func ToSafePath(path string) string {
+	volume := filepath.VolumeName(path)
+	if volume != "" {
+		path = strings.Replace(path, volume, "", 1)
+	}
+
+	path = filepath.Join(string(filepath.Separator), path)
+	path = filepath.ToSlash(path)
+	return path[1:]
+}

index.go

@@ -2,6 +2,7 @@ package siva
 
 import (
 	"bytes"
+	"runtime"
 	"sort"
 	"time"
 
@@ -104,3 +105,51 @@ func (s *IndexSuite) TestFind(c *C) {
 	c.Assert(e, NotNil)
 	c.Assert(e.Start, Equals, uint64(2))
 }
+
+func (s *IndexSuite) TestToSafePathsWindows(c *C) {
+	if runtime.GOOS != "windows" {
+		c.Skip("windows only")
+	}
+
+	i := Index{
+		{Header: Header{Name: `C:\foo\bar`}, Start: 1},
+		{Header: Header{Name: `\\network\share\foo\bar`}, Start: 2},
+		{Header: Header{Name: `/foo/bar`}, Start: 3},
+		{Header: Header{Name: `../bar`}, Start: 4},
+		{Header: Header{Name: `foo/bar/../../baz`}, Start: 5},
+	}
+
+	f := i.ToSafePaths()
+	expected := Index{
+		{Header: Header{Name: `foo/bar`}, Start: 1},
+		{Header: Header{Name: `foo/bar`}, Start: 2},
+		{Header: Header{Name: `foo/bar`}, Start: 3},
+		{Header: Header{Name: `bar`}, Start: 4},
+		{Header: Header{Name: `baz`}, Start: 5},
+	}
+	c.Assert(f, DeepEquals, expected)
+}
+
+func (s *IndexSuite) TestToSafePathsNotWindows(c *C) {
+	if runtime.GOOS == "windows" {
+		c.Skip("posix only")
+	}
+
+	i := Index{
+		{Header: Header{Name: `C:\foo\bar`}, Start: 1},
+		{Header: Header{Name: `\\network\share\foo\bar`}, Start: 2},
+		{Header: Header{Name: `/foo/bar`}, Start: 3},
+		{Header: Header{Name: `../bar`}, Start: 4},
+		{Header: Header{Name: `foo/bar/../../baz`}, Start: 5},
+	}
+
+	f := i.ToSafePaths()
+	expected := Index{
+		{Header: Header{Name: `C:\foo\bar`}, Start: 1},
+		{Header: Header{Name: `\\network\share\foo\bar`}, Start: 2},
+		{Header: Header{Name: `foo/bar`}, Start: 3},
+		{Header: Header{Name: `bar`}, Start: 4},
+		{Header: Header{Name: `baz`}, Start: 5},
+	}
+	c.Assert(f, DeepEquals, expected)
+}