Prevent arbitrary file overwrite via path traversal [CVE-2019-10743]

archiver.go

@@ -72,6 +72,11 @@ type ExtensionChecker interface {
 	CheckExt(name string) error
 }
 
+// FilenameChecker validates filenames to prevent path traversal attacks
+type FilenameChecker interface {
+	CheckPath(to, filename string) error
+}
+
 // Unarchiver is a type that can extract archive files
 // into a folder.
 type Unarchiver interface {

archiver_test.go

@@ -451,6 +451,57 @@ func symmetricTest(t *testing.T, formatName, dest string, testSymlinks, testMode
 	}
 }
 
+// test at runtime if the CheckFilename function is behaving properly for the archive formats
+func TestSafeExtraction(t *testing.T) {
+
+	testArchives := []string{
+		"testdata/testarchives/evilarchives/evil.zip",
+		"testdata/testarchives/evilarchives/evil.tar",
+		"testdata/testarchives/evilarchives/evil.tar.gz",
+		"testdata/testarchives/evilarchives/evil.tar.bz2",
+	}
+
+	for _, archiveName := range testArchives {
+
+		expected := true // 'evilfile' should not be extracted outside of destination directory and 'safefile' should be extracted anyway in the destination folder anyway 
+
+		if _, err := os.Stat(archiveName); os.IsNotExist(err) {
+			t.Errorf("archive not found")
+		}
+
+		actual := CheckFilenames(archiveName)
+
+		if actual != expected {
+			t.Errorf("CheckFilename is misbehaving for archive format type %s", filepath.Ext(archiveName))
+		}
+	}
+}
+
+func CheckFilenames(archiveName string) bool {
+
+	evilNotExtracted := false // by default we cannot assume that the path traversal filename is mitigated by CheckFilename
+	safeExtracted := false    // by default we cannot assume that a benign file can be extracted successfully
+
+	// clean the destination folder after this test
+	defer os.RemoveAll("testdata/testarchives/destarchives/")
+
+	err := Unarchive(archiveName, "testdata/testarchives/destarchives/")
+	if err != nil {
+		fmt.Println(err)
+	}
+
+	// is 'evilfile' prevented to be extracted outside of the destination folder?
+	if _, err := os.Stat("testdata/testarchives/evilfile"); os.IsNotExist(err) {
+		evilNotExtracted = true
+	}
+	// is 'safefile' safely extracted without errors inside the destination path?
+	if _, err := os.Stat("testdata/testarchives/destarchives/safedir/safefile"); !os.IsNotExist(err) {
+		safeExtracted = true
+	}
+
+	return evilNotExtracted && safeExtracted
+}
+
 var archiveFormats = []interface{}{
 	DefaultZip,
 	DefaultTar,
@@ -481,3 +532,4 @@ func (ffi fakeFileInfo) Mode() os.FileMode  { return ffi.mode }
 func (ffi fakeFileInfo) ModTime() time.Time { return ffi.modTime }
 func (ffi fakeFileInfo) IsDir() bool        { return ffi.isDir }
 func (ffi fakeFileInfo) Sys() interface{}   { return ffi.sys }
+

rar.go

@@ -60,6 +60,17 @@ func (*Rar) CheckExt(filename string) error {
 	return nil
 }
 
+// CheckPath ensures that the filename has not been crafted to perform path traversal attacks
+func (*Rar) CheckPath(to, filename string) error {
+	to, _ = filepath.Abs(to) //explicit the destination folder to prevent that 'string.HasPrefix' check can be 'bypassed' when no destination folder is supplied in input
+	dest := filepath.Join(to, filename) 
+	//prevent path traversal attacks
+	if !strings.HasPrefix(dest, to) {
+		return fmt.Errorf("illegal file path: %s", filename)
+	}
+	return nil
+}
+
 // Unarchive unpacks the .rar file at source to destination.
 // Destination will be treated as a folder name. It supports
 // multi-volume archives.
@@ -94,7 +105,7 @@ func (r *Rar) Unarchive(source, destination string) error {
 			break
 		}
 		if err != nil {
-			if r.ContinueOnError {
+			if r.ContinueOnError || strings.Contains(err.Error(), "illegal file path") {
 				log.Printf("[ERROR] Reading file in rar archive: %v", err)
 				continue
 			}
@@ -145,10 +156,18 @@ func (r *Rar) unrarNext(to string) error {
 	if err != nil {
 		return err // don't wrap error; calling loop must break on io.EOF
 	}
+	defer f.Close()
+
 	header, ok := f.Header.(*rardecode.FileHeader)
 	if !ok {
 		return fmt.Errorf("expected header to be *rardecode.FileHeader but was %T", f.Header)
 	}
+
+	errPath := r.CheckPath(to, header.Name)
+	if errPath != nil {
+		return fmt.Errorf("checking path traversal attempt: %v", errPath)
+	}	
+
 	return r.unrarFile(f, filepath.Join(to, header.Name))
 }
 
@@ -402,6 +421,7 @@ var (
 	_ = Extractor(new(Rar))
 	_ = Matcher(new(Rar))
 	_ = ExtensionChecker(new(Rar))
+	_ = FilenameChecker(new(Rar))
 	_ = os.FileInfo(rarFileInfo{})
 )
 

tar.go

@@ -61,6 +61,17 @@ func (*Tar) CheckExt(filename string) error {
 	return nil
 }
 
+// CheckPath ensures that the filename has not been crafted to perform path traversal attacks
+func (*Tar) CheckPath(to, filename string) error {
+	to, _ = filepath.Abs(to) //explicit the destination folder to prevent that 'string.HasPrefix' check can be 'bypassed' when no destination folder is supplied in input
+	dest := filepath.Join(to, filename) 
+	//prevent path traversal attacks
+	if !strings.HasPrefix(dest, to) {
+		return fmt.Errorf("illegal file path: %s", filename)
+	}
+	return nil
+}
+
 // Archive creates a tarball file at destination containing
 // the files listed in sources. The destination must end with
 // ".tar". File paths can be those of regular files or
@@ -150,7 +161,7 @@ func (t *Tar) Unarchive(source, destination string) error {
 			break
 		}
 		if err != nil {
-			if t.ContinueOnError {
+			if t.ContinueOnError || strings.Contains(err.Error(), "illegal file path") {
 				log.Printf("[ERROR] Reading file in tar archive: %v", err)
 				continue
 			}
@@ -211,10 +222,17 @@ func (t *Tar) untarNext(to string) error {
 	if err != nil {
 		return err // don't wrap error; calling loop must break on io.EOF
 	}
+	defer f.Close()
+
 	header, ok := f.Header.(*tar.Header)
 	if !ok {
 		return fmt.Errorf("expected header to be *tar.Header but was %T", f.Header)
 	}
+
+	errPath := t.CheckPath(to, header.Name)
+	if errPath != nil {
+		return fmt.Errorf("checking path traversal attempt: %v", errPath)
+	}
 	return t.untarFile(f, filepath.Join(to, header.Name))
 }
 
@@ -609,7 +627,8 @@ var (
 	_ = Walker(new(Tar))
 	_ = Extractor(new(Tar))
 	_ = Matcher(new(Tar))
-	_ = ExtensionChecker(new(Rar))
+	_ = ExtensionChecker(new(Tar))
+	_ = FilenameChecker(new(Tar))
 )
 
 // DefaultTar is a default instance that is conveniently ready to use.

zip.go

@@ -68,6 +68,17 @@ func (*Zip) CheckExt(filename string) error {
 	return nil
 }
 
+// CheckPath ensures the file extension matches the format.
+func (*Zip) CheckPath(to, filename string) error {
+	to, _ = filepath.Abs(to) //explicit the destination folder to prevent that 'string.HasPrefix' check can be 'bypassed' when no destination folder is supplied in input
+	dest := filepath.Join(to, filename) 
+	//prevent path traversal attacks
+	if !strings.HasPrefix(dest, to) {
+		return fmt.Errorf("illegal file path: %s", filename)
+	}
+	return nil
+}
+
 // Archive creates a .zip file at destination containing
 // the files listed in sources. The destination must end
 // with ".zip". File paths can be those of regular files
@@ -165,7 +176,7 @@ func (z *Zip) Unarchive(source, destination string) error {
 			break
 		}
 		if err != nil {
-			if z.ContinueOnError {
+			if z.ContinueOnError || strings.Contains(err.Error(), "illegal file path") {
 				log.Printf("[ERROR] Reading file in zip archive: %v", err)
 				continue
 			}
@@ -182,9 +193,15 @@ func (z *Zip) extractNext(to string) error {
 		return err // don't wrap error; calling loop must break on io.EOF
 	}
 	defer f.Close()
+
+	errPath := z.CheckPath(to, f.Header.(zip.FileHeader).Name)
+	if errPath != nil {
+		return fmt.Errorf("checking path traversal attempt: %v", errPath)
+	}
 	return z.extractFile(f, to)
 }
 
+
 func (z *Zip) extractFile(f File, to string) error {
 	header, ok := f.Header.(zip.FileHeader)
 	if !ok {
@@ -560,6 +577,7 @@ var (
 	_ = Extractor(new(Zip))
 	_ = Matcher(new(Zip))
 	_ = ExtensionChecker(new(Zip))
+	_ = FilenameChecker(new(Zip))
 )
 
 // compressedFormats is a (non-exhaustive) set of lowercased