Inspect archive before extraction (#433)

utils/archiveutils.go

@@ -32,7 +32,7 @@ func ExtractArchive(localPath, localFileName, originFileName, logMsgPrefix strin
 	if err != nil {
 		return err
 	}
-	err = os.MkdirAll(extractionPath, 0777)
+	err = os.MkdirAll(extractionPath, 0755)
 	if errorutils.CheckError(err) != nil {
 		return err
 	}

utils/io/fileutils/archive.go

@@ -1,8 +1,13 @@
 package fileutils
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io/ioutil"
+	"os"
+	"path"
+	"strings"
 
 	"github.com/jfrog/jfrog-client-go/utils/errorutils"
 	"github.com/mholt/archiver/v3"
@@ -18,18 +23,24 @@ func IsSupportedArchive(filePath string) bool {
 }
 
 // The 'archiver' dependency includes an API called 'Unarchive' to extract archive files. This API uses the archive file
-// extension to determine the archive type.// the local file path to extract the archive.
+// extension to determine the archive type.
 // We therefore need to use the file name as it was in Artifactory, and not the file name which was downloaded. To achieve this,
 // we added a new implementation of the 'Unarchive' func and use it instead of the default one.
+// localArchivePath - The local file path to extract the archive
+// originArchiveName - The archive file name
+// destinationPath - The extraction destination directory
 func Unarchive(localArchivePath, originArchiveName, destinationPath string) error {
-	uaIface, err := byExtension(originArchiveName)
+	archive, err := byExtension(originArchiveName)
 	if err != nil {
 		return err
 	}
-	u, ok := uaIface.(archiver.Unarchiver)
+	u, ok := archive.(archiver.Unarchiver)
 	if !ok {
 		return errorutils.CheckError(errors.New("format specified by source filename is not an archive format: " + originArchiveName))
 	}
+	if err = inspectArchive(archive, localArchivePath, destinationPath); err != nil {
+		return err
+	}
 	return u.Unarchive(localArchivePath, destinationPath)
 }
 
@@ -118,3 +129,71 @@ var extCheckers = []archiver.ExtensionChecker{
 	&archiver.Xz{},
 	&archiver.Zstd{},
 }
+
+// Make sure the archive is free from Zip Slip and Zip symlinks attacks
+func inspectArchive(archive interface{}, localArchivePath, destinationDir string) error {
+	walker, ok := archive.(archiver.Walker)
+	if !ok {
+		return errorutils.CheckError(errors.New("couldn't inspect archive: " + localArchivePath))
+	}
+	return walker.Walk(localArchivePath, func(archiveEntry archiver.File) error {
+		header, err := extractArchiveEntryHeader(archiveEntry)
+		if err != nil {
+			return err
+		}
+		if !isEntryInDestination(destinationDir, header.EntryPath) {
+			return errorutils.CheckError(errors.New("illegal path in archive: " + header.EntryPath))
+		}
+
+		if (archiveEntry.Mode() & os.ModeSymlink) != 0 {
+			err = checkSymlinkEntry(header, archiveEntry, destinationDir)
+		}
+		return err
+	})
+}
+
+// Make sure the extraction path of the symlink entry target is under the destination dir
+func checkSymlinkEntry(header *archiveHeader, archiveEntry archiver.File, destinationDir string) error {
+	targetLinkPath := header.TargetLink
+	if targetLinkPath == "" {
+		// The link destination path is not always in the archive header
+		// In that case, we will look at the link content to get the link destination path
+		content, err := ioutil.ReadAll(archiveEntry.ReadCloser)
+		if err != nil {
+			return errorutils.CheckError(err)
+		}
+		targetLinkPath = string(content)
+	}
+
+	if !isEntryInDestination(destinationDir, targetLinkPath) {
+		return errorutils.CheckError(errors.New("illegal link path in archive: " + targetLinkPath))
+	}
+	return nil
+}
+
+// Make sure the extraction path of the archive entry is under the destination dir
+func isEntryInDestination(destinationDir, pathInArchive string) bool {
+	// Since the entry in archive should be always represented as Unix path, the "path" module is used and not "filepath"
+	pathInArchive = path.Clean(pathInArchive)
+	if !path.IsAbs(pathInArchive) {
+		// If path is relative, concatenate it to the destination dir
+		pathInArchive = path.Join(destinationDir, pathInArchive)
+	}
+	return strings.HasPrefix(pathInArchive, destinationDir)
+}
+
+// Extract the header of the archive entry
+func extractArchiveEntryHeader(f archiver.File) (*archiveHeader, error) {
+	headerBytes, err := json.Marshal(f.Header)
+	if err != nil {
+		return nil, err
+	}
+	archiveHeader := &archiveHeader{}
+	err = json.Unmarshal(headerBytes, archiveHeader)
+	return archiveHeader, err
+}
+
+type archiveHeader struct {
+	EntryPath  string `json:"Name,omitempty"`
+	TargetLink string `json:"Linkname,omitempty"`
+}

utils/io/fileutils/archive_test.go

@@ -0,0 +1,64 @@
+package fileutils
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUnarchive(t *testing.T) {
+	tests := []string{"zip", "tar", "tar.gz"}
+	for _, extension := range tests {
+		t.Run(extension, func(t *testing.T) {
+			// Create temp directory
+			tmpDir, err := CreateTempDir()
+			assert.NoError(t, err)
+			defer RemoveTempDir(tmpDir)
+
+			// Run unarchive on archive created on Unix
+			err = runUnarchive(t, "unix."+extension, "archives", filepath.Join(tmpDir, "unix"))
+			assert.NoError(t, err)
+			assert.FileExists(t, filepath.Join(tmpDir, "unix", "link"))
+			assert.FileExists(t, filepath.Join(tmpDir, "unix", "dir", "file"))
+
+			// Run unarchive on archive created on Windows
+			err = runUnarchive(t, "win."+extension, "archives", filepath.Join(tmpDir, "win"))
+			assert.NoError(t, err)
+			assert.FileExists(t, filepath.Join(tmpDir, "win", "link.lnk"))
+			assert.FileExists(t, filepath.Join(tmpDir, "win", "dir", "file.txt"))
+		})
+	}
+}
+
+func TestUnarchiveZipSlip(t *testing.T) {
+	tests := []struct {
+		testType    string
+		archives    []string
+		errorSuffix string
+	}{
+		{"rel", []string{"zip", "tar", "tar.gz"}, "illegal path in archive: ../file"},
+		{"abs", []string{"tar", "tar.gz"}, "illegal path in archive: /tmp/bla/file"},
+		{"softlink-abs", []string{"zip", "tar", "tar.gz"}, "illegal link path in archive: /tmp/bla/file"},
+		{"softlink-rel", []string{"zip", "tar", "tar.gz"}, "illegal link path in archive: ../file"},
+	}
+	for _, test := range tests {
+		t.Run(test.testType, func(t *testing.T) {
+			// Create temp directory
+			tmpDir, err := CreateTempDir()
+			assert.NoError(t, err)
+			defer RemoveTempDir(tmpDir)
+
+			for _, archive := range test.archives {
+				// Unarchive and make sure an error returns
+				err = runUnarchive(t, test.testType+"."+archive, "zipslip", tmpDir)
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), test.errorSuffix)
+			}
+		})
+	}
+}
+
+func runUnarchive(t *testing.T, archiveFileName, sourceDir, targetDir string) error {
+	return Unarchive(filepath.Join("testdata", sourceDir, archiveFileName), archiveFileName, targetDir)
+}