From 2c6edc64d928215043b0ef40fab8c90fda758e2e Mon Sep 17 00:00:00 2001
From: Yahav Itzhak <yahavi@users.noreply.github.com>
Date: Mon, 11 Mar 2024 09:19:16 +0200
Subject: [PATCH] Fix false positive ZipSlip when an archive starts with dot
 (#54)

---
 unarchive/archive.go                       |   7 ++++-
 unarchive/archive_test.go                  |  34 +++++++++++----------
 unarchive/testdata/archives/dot-dir.tar.gz | Bin 0 -> 110 bytes
 3 files changed, 24 insertions(+), 17 deletions(-)
 create mode 100644 unarchive/testdata/archives/dot-dir.tar.gz

diff --git a/unarchive/archive.go b/unarchive/archive.go
index c2522e9..091d691 100644
--- a/unarchive/archive.go
+++ b/unarchive/archive.go
@@ -3,12 +3,13 @@ package unarchive
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/jfrog/archiver/v3"
 	"io"
 	"os"
 	"path/filepath"
 	"strings"
 
+	"github.com/jfrog/archiver/v3"
+
 	"github.com/jfrog/gofrog/datastructures"
 )
 
@@ -135,6 +136,10 @@ func (u *Unarchiver) byExtension(filename string) (interface{}, error) {
 
 // Make sure the archive is free from Zip Slip and Zip symlinks attacks
 func inspectArchive(archive interface{}, localArchivePath, destinationDir string) error {
+	// If the destination directory ends with a slash, delete it. 
+	// This is necessary to handle a situation where the entry path might be at the root of the destination directory, 
+	// but in such a case "<destination-dir>/" is not a prefix of "<destination-dir>".
+	destinationDir = strings.TrimSuffix(destinationDir, string(os.PathSeparator))
 	walker, ok := archive.(archiver.Walker)
 	if !ok {
 		return fmt.Errorf("couldn't inspect archive: " + localArchivePath)
diff --git a/unarchive/archive_test.go b/unarchive/archive_test.go
index bc932c0..4d17eb9 100644
--- a/unarchive/archive_test.go
+++ b/unarchive/archive_test.go
@@ -14,8 +14,8 @@ func TestUnarchive(t *testing.T) {
 	for _, extension := range tests {
 		t.Run(extension, func(t *testing.T) {
 			// Create temp directory
-			tmpDir, createTempDirCallback := createTempDirWithCallbackAndAssert(t)
-			defer createTempDirCallback()
+			tmpDir := t.TempDir()
+
 			// Run unarchive on archive created on Unix
 			err := runUnarchive(t, uarchiver, "unix."+extension, "archives", filepath.Join(tmpDir, "unix"))
 			assert.NoError(t, err)
@@ -48,8 +48,7 @@ func TestUnarchiveSymlink(t *testing.T) {
 			for _, testCase := range unarchiveSymlinksCases {
 				t.Run(testCase.prefix, func(t *testing.T) {
 					// Create temp directory
-					tmpDir, createTempDirCallback := createTempDirWithCallbackAndAssert(t)
-					defer createTempDirCallback()
+					tmpDir := t.TempDir()
 
 					// Run unarchive
 					err := runUnarchive(t, uarchiver, testCase.prefix+"."+extension, "archives", tmpDir)
@@ -84,8 +83,8 @@ func TestUnarchiveZipSlip(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.testType, func(t *testing.T) {
 			// Create temp directory
-			tmpDir, createTempDirCallback := createTempDirWithCallbackAndAssert(t)
-			defer createTempDirCallback()
+			tmpDir := t.TempDir()
+
 			for _, archive := range test.archives {
 				// Unarchive and make sure an error returns
 				err := runUnarchive(t, uarchiver, test.testType+"."+archive, "zipslip", tmpDir)
@@ -103,8 +102,8 @@ func TestUnarchiveWithStripComponents(t *testing.T) {
 	for _, extension := range tests {
 		t.Run(extension, func(t *testing.T) {
 			// Create temp directory
-			tmpDir, createTempDirCallback := createTempDirWithCallbackAndAssert(t)
-			defer createTempDirCallback()
+			tmpDir := t.TempDir()
+
 			// Run unarchive on archive created on Unix
 			err := runUnarchive(t, uarchiver, "strip-components."+extension, "archives", filepath.Join(tmpDir, "unix"))
 			assert.NoError(t, err)
@@ -120,16 +119,19 @@ func TestUnarchiveWithStripComponents(t *testing.T) {
 	}
 }
 
+// Test unarchive file with a directory named "." in the root directory
+func TestUnarchiveDotDir(t *testing.T) {
+	// Create temp directory
+	tmpDir := t.TempDir()
+
+	// Run unarchive
+	err := runUnarchive(t, Unarchiver{}, "dot-dir.tar.gz", "archives", tmpDir+string(os.PathSeparator))
+	assert.NoError(t, err)
+	assert.DirExists(t, filepath.Join(tmpDir, "dir"))
+}
+
 func runUnarchive(t *testing.T, uarchiver Unarchiver, archiveFileName, sourceDir, targetDir string) error {
 	archivePath := filepath.Join("testdata", sourceDir, archiveFileName)
 	assert.True(t, uarchiver.IsSupportedArchive(archivePath))
 	return uarchiver.Unarchive(filepath.Join("testdata", sourceDir, archiveFileName), archiveFileName, targetDir)
 }
-
-func createTempDirWithCallbackAndAssert(t *testing.T) (string, func()) {
-	tempDirPath, err := os.MkdirTemp("", "archiver_test")
-	assert.NoError(t, err, "Couldn't create temp dir")
-	return tempDirPath, func() {
-		assert.NoError(t, os.RemoveAll(tempDirPath), "Couldn't remove temp dir")
-	}
-}
diff --git a/unarchive/testdata/archives/dot-dir.tar.gz b/unarchive/testdata/archives/dot-dir.tar.gz
new file mode 100644
index 0000000000000000000000000000000000000000..c8484d7dc56b8774fb3c37a88349eabcdce12006
GIT binary patch
literal 110
zcmV-!0FnP6iwFQ2zwKoJ1Jl!IpgAxwFfcbaR{+sKfKEd=h9;)w#>U3xCdP&eK;_2f
z<_rp^40Hgc#U+VFK&Mb@4!Y-t2+-3{$t<FA8XzJ5&CJXV6buXv49yHj<9`&4f>AIE
QXa^V=0PwY02>=KH0L8W{rT_o{

literal 0
HcmV?d00001