🔒 [Unzip] security risk mitigation (#135)

* Added new test cases

* Add file limit when unzipping

* Add file count limit when unzipping files

* Add file count limit when unzipping files

* Improve "unzip file count limit" tests readability

* Add file count limit

* Add file count limit

* Fix whitespace issue

* fix formatting

* Fix pull request issues

* Fix formatting issue

* Update mock

Co-authored-by: Adrien CABARBAYE <[email protected]>

changes/20220913.feature

@@ -0,0 +1 @@
+[Filesystem] Added a file number limit when unzipping to protect against attacks with zip files

utils/filesystem/files.go

@@ -1223,8 +1223,10 @@ func (fs *VFS) unzip(ctx context.Context, source string, destination string, lim
 	if err != nil {
 		return
 	}
-	// List of file paths to return
 
+	fileCounter := atomic.NewUint64(0)
+
+	// List of file paths to return
 	totalSizeOnDisk := atomic.NewUint64(0)
 
 	info, err := fs.Lstat(source)
@@ -1259,6 +1261,8 @@ func (fs *VFS) unzip(ctx context.Context, source string, destination string, lim
 
 	// For each file in the zip file
 	for i := range zipReader.File {
+		fileCounter.Inc()
+
 		zippedFile := zipReader.File[i]
 		subErr := parallelisation.DetermineContextError(ctx)
 		if subErr != nil {
@@ -1302,6 +1306,9 @@ func (fs *VFS) unzip(ctx context.Context, source string, destination string, lim
 		if limits.Apply() && totalSizeOnDisk.Load() > limits.GetMaxTotalSize() {
 			return fileList, fmt.Errorf("%w: more than %v B of disk space was used while unzipping %v (%v B used already)", commonerrors.ErrTooLarge, limits.GetMaxTotalSize(), source, totalSizeOnDisk.Load())
 		}
+		if limits.Apply() && int64(fileCounter.Load()) > limits.GetMaxFileCount() {
+			return fileList, fmt.Errorf("%w: more than %v files were created while unzipping %v (%v files created already)", commonerrors.ErrTooLarge, limits.GetMaxFileCount(), source, fileCounter)
+		}
 	}
 
 	// Ensuring directory timestamps are preserved (this needs to be done after all the files have been created).

utils/filesystem/files_test.go

@@ -1032,7 +1032,7 @@ func TestUnzip_Limits(t *testing.T) {
 	destPath, err := fs.TempDirInTempDir("unzip-limits-")
 	require.NoError(t, err)
 	defer func() { _ = fs.Rm(destPath) }()
-	limits := NewLimits(1<<30, 1<<10) // Total size limited to 10 Kb
+	limits := NewLimits(1<<30, 1<<10, 1000000) // Total size limited to 10 Kb
 
 	empty, err := fs.IsEmpty(destPath)
 	assert.NoError(t, err)
@@ -1085,7 +1085,7 @@ func TestUnzip_ZipBomb(t *testing.T) {
 	destPath, err := fs.TempDirInTempDir("unzip-limits-")
 	require.NoError(t, err)
 	defer func() { _ = fs.Rm(destPath) }()
-	limits := NewLimits(1<<30, 1<<20) // Total size limited to 1 Mb
+	limits := NewLimits(1<<30, 1<<20, 1000000) // Total size limited to 1 Mb
 
 	empty, err := fs.IsEmpty(destPath)
 	assert.NoError(t, err)
@@ -1306,6 +1306,44 @@ func TestFilepathStem(t *testing.T) {
 	})
 }
 
+func TestUnzipFileCountLimit(t *testing.T) {
+	fs := NewFs(StandardFS)
+
+	testInDir := "testdata"
+	limits := NewLimits(1<<30, 10<<30, 10)
+
+	t.Run("unzip file above file count limit", func(t *testing.T) {
+		testFile := "abovefilecountlimitzip"
+		srcPath := filepath.Join(testInDir, testFile+".zip")
+
+		destPath, err := fs.TempDirInTempDir("unzip-limits-")
+		assert.NoError(t, err)
+		defer func() {
+			_ = fs.Rm(destPath)
+		}()
+
+		_, err = fs.UnzipWithContextAndLimits(context.TODO(), srcPath, destPath, limits)
+		assert.True(t, commonerrors.Any(err, commonerrors.ErrTooLarge))
+	})
+
+	t.Run("unzip file below file count limit", func(t *testing.T) {
+		testFile := "belowfilecountlimitzip"
+		srcPath := filepath.Join(testInDir, testFile+".zip")
+
+		destPath, err := fs.TempDirInTempDir("unzip-limits-")
+		assert.NoError(t, err)
+
+		defer func() {
+			if tempErr := fs.Rm(destPath); tempErr != nil {
+				err = tempErr
+			}
+		}()
+
+		_, err = fs.UnzipWithContextAndLimits(context.TODO(), srcPath, destPath, limits)
+		assert.NoError(t, err)
+	})
+}
+
 func checkCopyDir(t *testing.T, fs FS, src string, dest string) {
 	assert.True(t, fs.Exists(src))
 	assert.False(t, fs.Exists(dest))

utils/filesystem/interfaces.go

@@ -74,6 +74,8 @@ type ILimits interface {
 	GetMaxFileSize() int64
 	// GetMaxTotalSize returns the maximum size in byte a location can have on a file system (whether it is a file or a folder)
 	GetMaxTotalSize() uint64
+	// GetMaxFileCount returns the maximum files in byte a file can have on a file system
+	GetMaxFileCount() int64
 }
 
 // ILock defines a generic lock using the file system.

utils/filesystem/limits.go

@@ -21,6 +21,10 @@ func (n *noLimits) GetMaxTotalSize() uint64 {
 	return 0
 }
 
+func (n *noLimits) GetMaxFileCount() int64 {
+	return 0
+}
+
 func (n *noLimits) Validate() error {
 	return nil
 }
@@ -29,6 +33,7 @@ func (n *noLimits) Validate() error {
 type Limits struct {
 	MaxFileSize  int64  `mapstructure:"max_file_size"`
 	MaxTotalSize uint64 `mapstructure:"max_total_size"`
+	MaxFileCount int64  `mapstructure:"max_file_count"`
 }
 
 func (l *Limits) Apply() bool {
@@ -43,6 +48,10 @@ func (l *Limits) GetMaxTotalSize() uint64 {
 	return l.MaxTotalSize
 }
 
+func (l *Limits) GetMaxFileCount() int64 {
+	return l.MaxFileCount
+}
+
 func (l *Limits) Validate() error {
 	validation.ErrorTag = "mapstructure"
 
@@ -54,6 +63,7 @@ func (l *Limits) Validate() error {
 	return validation.ValidateStruct(l,
 		validation.Field(&l.MaxFileSize, validation.Required.When(l.Apply())),
 		validation.Field(&l.MaxTotalSize, validation.Required.When(l.Apply())),
+		validation.Field(&l.MaxFileCount, validation.Required.When(l.Apply())),
 	)
 }
 
@@ -63,14 +73,15 @@ func NoLimits() ILimits {
 }
 
 // NewLimits defines file system FileSystemLimits.
-func NewLimits(maxFileSize int64, maxTotalSize uint64) ILimits {
+func NewLimits(maxFileSize int64, maxTotalSize uint64, maxFileCount int64) ILimits {
 	return &Limits{
 		MaxFileSize:  maxFileSize,
 		MaxTotalSize: maxTotalSize,
+		MaxFileCount: maxFileCount,
 	}
 }
 
 // DefaultLimits defines default file system FileSystemLimits
 func DefaultLimits() ILimits {
-	return NewLimits(1<<30, 10<<30)
+	return NewLimits(1<<30, 10<<30, 1000000)
 }