Zip: Harden fs.Unzip() implementation in pkg/fs photoprism#5330

Signed-off-by: Michael Mayer <[email protected]>

Author: lastzero

pkg/fs/zip.go

@@ -2,8 +2,10 @@ package fs
 
 import (
 	"archive/zip"
+	"errors"
 	"fmt"
 	"io"
+	"math"
 	"os"
 	"path/filepath"
 	"strings"
@@ -109,15 +111,24 @@ func Unzip(zipName, dir string, fileSizeLimit, totalSizeLimit int64) (files []st
 			continue
 		}
 
-		if totalSizeLimit < 1 {
-			// Do nothing;
-		} else if totalSizeLimit = totalSizeLimit - int64(zipFile.UncompressedSize64); totalSizeLimit < 1 {
+		if zipFile.UncompressedSize64 > uint64(math.MaxInt64) {
 			skipped = append(skipped, zipFile.Name)
-			totalSizeLimit = 0
 			continue
 		}
 
-		fileName, unzipErr := UnzipFile(zipFile, dir)
+		if totalSizeLimit > 0 {
+			entrySize := int64(zipFile.UncompressedSize64) //nolint:gosec // safe: capped by check above
+
+			totalSizeLimit -= entrySize
+
+			if totalSizeLimit < 1 {
+				skipped = append(skipped, zipFile.Name)
+				totalSizeLimit = 0
+				continue
+			}
+		}
+
+		fileName, unzipErr := unzipFileWithLimit(zipFile, dir, fileSizeLimit)
 		if unzipErr != nil {
 			return files, skipped, unzipErr
 		}
@@ -130,6 +141,11 @@ func Unzip(zipName, dir string, fileSizeLimit, totalSizeLimit int64) (files []st
 
 // UnzipFile writes a file from a zip archive to the target destination.
 func UnzipFile(f *zip.File, dir string) (fileName string, err error) {
+	return unzipFileWithLimit(f, dir, 0)
+}
+
+// unzipFileWithLimit writes a file from a zip archive to the target destination while applying a size limit.
+func unzipFileWithLimit(f *zip.File, dir string, fileSizeLimit int64) (fileName string, err error) {
 	rc, err := f.Open()
 	if err != nil {
 		return fileName, err
@@ -165,9 +181,31 @@ func UnzipFile(f *zip.File, dir string) (fileName string, err error) {
 
 	defer fd.Close()
 
-	_, err = io.Copy(fd, rc)
-	if err != nil {
-		return fileName, err
+	limit := fileSizeLimit
+
+	if limit <= 0 {
+		switch {
+		case f.UncompressedSize64 == 0:
+			limit = math.MaxInt64
+		case f.UncompressedSize64 > uint64(math.MaxInt64):
+			return fileName, fmt.Errorf("zip entry too large")
+		default:
+			limit = int64(f.UncompressedSize64) //nolint:gosec // safe: capped above
+		}
+	}
+
+	written, copyErr := io.CopyN(fd, rc, limit)
+	if copyErr != nil && !errors.Is(copyErr, io.EOF) && !errors.Is(copyErr, io.ErrUnexpectedEOF) {
+		return fileName, copyErr
+	}
+
+	// Abort if the entry exceeded the configured limit.
+	if written >= limit && (fileSizeLimit > 0 || f.UncompressedSize64 > 0) {
+		// Drain a single byte to see if more data remains (indicating truncation).
+		var b [1]byte
+		if _, extraErr := rc.Read(b[:]); extraErr == nil {
+			return fileName, fmt.Errorf("zip entry exceeds limit")
+		}
 	}
 
 	return fileName, nil

pkg/fs/zip_test.go

@@ -2,6 +2,8 @@ package fs
 
 import (
 	"archive/zip"
+	"encoding/binary"
+	"math"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -148,6 +150,150 @@ func TestUnzip_CreatesDirectoriesAndNestedFiles(t *testing.T) {
 	}
 }
 
+func TestUnzip_SkipsVeryLargeEntry(t *testing.T) {
+	dir := t.TempDir()
+	zipPath := filepath.Join(dir, "huge.zip")
+
+	writeZip64Stub(t, zipPath, "huge.bin", math.MaxUint64)
+
+	files, skipped, err := Unzip(zipPath, filepath.Join(dir, "out"), 0, -1)
+	assert.NoError(t, err)
+	assert.Empty(t, files)
+	assert.Contains(t, skipped, "huge.bin")
+}
+
+// writeZip64Stub writes a minimal ZIP64 archive with one stored entry and custom size values.
+func writeZip64Stub(t *testing.T, path, name string, size uint64) {
+	t.Helper()
+
+	var buf []byte
+
+	bw := func(data []byte) {
+		buf = append(buf, data...)
+	}
+
+	writeLE := func(v any) {
+		var b [8]byte
+		switch x := v.(type) {
+		case uint16:
+			binary.LittleEndian.PutUint16(b[:2], x)
+			bw(b[:2])
+		case uint32:
+			binary.LittleEndian.PutUint32(b[:4], x)
+			bw(b[:4])
+		case uint64:
+			binary.LittleEndian.PutUint64(b[:8], x)
+			bw(b[:8])
+		default:
+			t.Fatalf("unsupported type %T", v)
+		}
+	}
+
+	filename := []byte(name)
+	const (
+		sigLocal   = 0x04034b50
+		sigCentral = 0x02014b50
+		sigEnd     = 0x06054b50
+	)
+
+	zip64ExtraLen := uint16(4 + 16) // header id + size + two uint64 values
+	localExtraLen := zip64ExtraLen
+	centralExtraLen := zip64ExtraLen
+
+	// Local file header
+	writeLE(uint32(sigLocal))
+	writeLE(uint16(45)) // version needed (zip64)
+	writeLE(uint16(0))  // flags
+	writeLE(uint16(0))  // method store
+	writeLE(uint16(0))  // mod time
+	writeLE(uint16(0))  // mod date
+	writeLE(uint32(0))  // crc
+	writeLE(uint32(0xFFFFFFFF))
+	writeLE(uint32(0xFFFFFFFF))
+	if len(filename) > math.MaxUint16 {
+		t.Fatalf("filename too long")
+	}
+	writeLE(uint16(len(filename)))
+	writeLE(localExtraLen)
+	bw(filename)
+	// zip64 extra
+	writeLE(uint16(0x0001)) // header id
+	writeLE(uint16(16))     // data size
+	writeLE(size)           // uncompressed size
+	writeLE(size)           // compressed size
+	// no file data (size 0) to keep archive tiny
+
+	localLen := len(buf)
+
+	// Central directory header
+	writeLE(uint32(sigCentral))
+	writeLE(uint16(45)) // version made by
+	writeLE(uint16(45)) // version needed
+	writeLE(uint16(0))  // flags
+	writeLE(uint16(0))  // method
+	writeLE(uint16(0))  // time
+	writeLE(uint16(0))  // date
+	writeLE(uint32(0))  // crc
+	writeLE(uint32(0xFFFFFFFF))
+	writeLE(uint32(0xFFFFFFFF))
+	if len(filename) > math.MaxUint16 {
+		t.Fatalf("filename too long")
+	}
+	writeLE(uint16(len(filename)))
+	writeLE(centralExtraLen)
+	writeLE(uint16(0)) // comment len
+	writeLE(uint16(0)) // disk start
+	writeLE(uint16(0)) // int attrs
+	writeLE(uint32(0)) // ext attrs
+	writeLE(uint32(0)) // rel offset (zip64 overrides)
+	bw(filename)
+	// zip64 extra
+	writeLE(uint16(0x0001))
+	writeLE(uint16(16))
+	writeLE(size) // uncompressed
+	writeLE(size) // compressed
+
+	centralLen := len(buf) - localLen
+
+	// End of central directory (not zip64 EOCD; minimal to satisfy reader)
+	writeLE(uint32(sigEnd))
+	writeLE(uint16(0)) // disk
+	writeLE(uint16(0)) // start disk
+	writeLE(uint16(1)) // entries this disk
+	writeLE(uint16(1)) // total entries
+	if centralLen > math.MaxUint32 || localLen > math.MaxUint32 {
+		t.Fatalf("central or local length exceeds uint32")
+	}
+	writeLE(uint32(centralLen))
+	writeLE(uint32(localLen))
+	writeLE(uint16(0)) // comment length
+
+	if err := os.WriteFile(path, buf, 0o600); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestUnzipFileWithLimit_DetectsOverrun(t *testing.T) {
+	dir := t.TempDir()
+	zipPath := filepath.Join(dir, "small.zip")
+	writeZip(t, zipPath, map[string][]byte{"a.txt": []byte("abc")}) // 3 bytes
+
+	r, err := zip.OpenReader(zipPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+
+	if len(r.File) != 1 {
+		t.Fatalf("expected one file, got %d", len(r.File))
+	}
+
+	_, err = unzipFileWithLimit(r.File[0], dir, 1) // limit below actual size
+	if err == nil {
+		t.Fatalf("expected limit overrun error")
+	}
+}
+
 func TestZip(t *testing.T) {
 	t.Run("Compressed", func(t *testing.T) {
 		zipDir := filepath.Join(os.TempDir(), "pkg/fs")