Prevent Zip Slip by validating unzip targets

operations/zipper.go

@@ -47,6 +47,15 @@ func slashed(text string) string {
 	return strings.Replace(text, backslash, slash, -1)
 }
 
+func safeExtractTarget(directory, entry string) (string, error) {
+	cleanDirectory := filepath.Clean(directory)
+	cleanTarget := filepath.Clean(filepath.Join(cleanDirectory, entry))
+	if cleanTarget != cleanDirectory && !strings.HasPrefix(cleanTarget, cleanDirectory+string(os.PathSeparator)) {
+		return "", fmt.Errorf("zip entry %q escapes destination directory", entry)
+	}
+	return cleanTarget, nil
+}
+
 func HololibZipShape(file *zip.File) error {
 	library := libraryPattern.MatchString(file.Name)
 	catalog := catalogPattern.MatchString(file.Name)
@@ -246,12 +255,15 @@ func (it *unzipper) Extract(directory string) error {
 		if entry.FileInfo().IsDir() {
 			continue
 		}
-		target := filepath.Join(directory, slashed(entry.Name)[limit:])
+		target, err := safeExtractTarget(directory, slashed(entry.Name)[limit:])
+		if err != nil {
+			return err
+		}
 		todo := WriteTarget{
 			Source: entry,
 			Target: target,
 		}
-		err := todo.execute()
+		err = todo.execute()
 		if err != nil {
 			return fmt.Errorf("Problem while extracting zip, reason: %v", err)
 		}

operations/zipper_test.go

@@ -1,20 +1,64 @@
 package operations
 
 import (
+	"archive/zip"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/joshyorko/rcc/hamlet"
 )
 
-const (
-	wintestpath = `a\b`
-	nixtestpath = `a/b`
-)
+func writeTestZip(t *testing.T, path string, entries map[string]string) {
+	t.Helper()
+	file, err := os.Create(path)
+	if err != nil {
+		t.Fatalf("unable to create zip: %v", err)
+	}
+	defer file.Close()
+	writer := zip.NewWriter(file)
+	for name, content := range entries {
+		entry, err := writer.Create(name)
+		if err != nil {
+			t.Fatalf("unable to create zip entry: %v", err)
+		}
+		if _, err = entry.Write([]byte(content)); err != nil {
+			t.Fatalf("unable to write zip entry: %v", err)
+		}
+	}
+	if err := writer.Close(); err != nil {
+		t.Fatalf("unable to close zip writer: %v", err)
+	}
+}
+
+func TestUnzipRejectsPathTraversal(t *testing.T) {
+	must, _ := hamlet.Specifications(t)
+	tmpDir := t.TempDir()
+	zipPath := filepath.Join(tmpDir, "malicious.zip")
+	writeTestZip(t, zipPath, map[string]string{
+		"../../outside.txt": "owned",
+	})
+
+	dest := filepath.Join(tmpDir, "dest")
+	err := Unzip(dest, zipPath, true, true, false)
+	must.NotEqual(nil, err)
+	must.Equal(false, filepath.IsAbs("../../outside.txt"))
+	_, statErr := os.Stat(filepath.Join(tmpDir, "outside.txt"))
+	must.Equal(true, os.IsNotExist(statErr))
+}
 
-func TestCanConvertSlashes(t *testing.T) {
-	must, wont := hamlet.Specifications(t)
+func TestUnzipExtractsRegularFile(t *testing.T) {
+	must, _ := hamlet.Specifications(t)
+	tmpDir := t.TempDir()
+	zipPath := filepath.Join(tmpDir, "good.zip")
+	writeTestZip(t, zipPath, map[string]string{
+		"safe/file.txt": "ok",
+	})
 
-	wont.Equal(wintestpath, nixtestpath)
-	must.Equal(3, len(wintestpath))
-	must.Equal(slashed(wintestpath), nixtestpath)
+	dest := filepath.Join(tmpDir, "dest")
+	err := Unzip(dest, zipPath, true, true, false)
+	must.Equal(nil, err)
+	content, err := os.ReadFile(filepath.Join(dest, "safe", "file.txt"))
+	must.Equal(nil, err)
+	must.Equal("ok", string(content))
 }