use safe functions to join file paths and open files

Use safe functions to join file paths and open files in places reported by Snyk.
`BuildPathChecked` checks for the ZipSlip and path traversal vulnerabilities,
while files are now opened using `os.OpenInRoot`.

Author: redbeam

pkg/crc/machine/bundle/repository.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -36,10 +37,14 @@ func (repo *Repository) Get(bundleName string) (*CrcBundleInfo, error) {
 	if _, err := os.Stat(path); err != nil {
 		return nil, errors.Wrapf(err, "could not find cached bundle info in %s", path)
 	}
-	jsonFilepath := filepath.Join(path, metadataFilename)
-	content, err := os.ReadFile(filepath.Clean(jsonFilepath))
+	metadataFile, err := os.OpenInRoot(path, metadataFilename)
 	if err != nil {
-		return nil, errors.Wrapf(err, "error reading %s file", jsonFilepath)
+		return nil, errors.Wrapf(err, "error opening metadata file")
+	}
+	defer metadataFile.Close()
+	content, err := io.ReadAll(metadataFile)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error reading %s file", filepath.Join(path, metadataFilename))
 	}
 	var bundleInfo CrcBundleInfo
 	if err := json.Unmarshal(content, &bundleInfo); err != nil {

pkg/extract/extract.go

@@ -107,7 +107,7 @@ func untar(ctx context.Context, reader io.Reader, targetDir string, fileFilter f
 		}
 
 		// the target location where the dir/file should be created
-		path, err := buildPath(targetDir, header.Name)
+		path, err := BuildPathChecked(targetDir, header.Name)
 		if err != nil {
 			return nil, err
 		}
@@ -120,13 +120,13 @@ func untar(ctx context.Context, reader io.Reader, targetDir string, fileFilter f
 		// check the file type
 		switch header.Typeflag {
 
-		// if its a dir and it doesn't exist create it
+		// if it's a dir, and it doesn't exist, create it
 		case tar.TypeDir:
 			if err := os.MkdirAll(path, header.FileInfo().Mode()); err != nil { // nolint:gosec // G703: paths from header.FileInfo()
 				return nil, err
 			}
 
-		// if it's a file create it
+		// if it's a file, create it
 		case tar.TypeReg, tar.TypeGNUSparse:
 			// tar.Next() will externally only iterate files, so we might have to create intermediate directories here
 			if err := uncompressFile(ctx, tarReader, header.FileInfo(), path, showProgress); err != nil {
@@ -159,17 +159,19 @@ func uncompressFile(ctx context.Context, tarReader io.Reader, fileInfo os.FileIn
 	return file.Close()
 }
 
-func buildPath(baseDir, filename string) (string, error) {
+// BuildPathChecked joins filename to baseDir and checks for ZipSlip and path traversal vulnerabilities.
+// More info: https://snyk.io/research/zip-slip-vulnerability, https://learn.snyk.io/lesson/directory-traversal/?ecosystem=golang
+func BuildPathChecked(baseDir, filename string) (string, error) {
 	path := filepath.Join(baseDir, filename) // #nosec G305
-
-	// Check for ZipSlip. More Info: https://snyk.io/research/zip-slip-vulnerability
 	baseDir = filepath.Clean(baseDir)
-	if path != baseDir && !strings.HasPrefix(path, baseDir+string(os.PathSeparator)) {
-		return "", fmt.Errorf("%s: illegal file path (expected prefix: %s)", path, baseDir+string(os.PathSeparator))
+	// clean prefix to ensure it isn't "//" when baseDir is "/"
+	prefix := filepath.Clean(baseDir + string(os.PathSeparator))
+	if path != baseDir && !strings.HasPrefix(path, prefix) {
+		return "", fmt.Errorf("%s: illegal file path (expected prefix: %s)", path, prefix)
 	}
-
 	return path, nil
 }
+
 func unzip(ctx context.Context, archive, target string, fileFilter func(string) bool, showProgress bool) ([]string, error) {
 	var extractedFiles []string
 	reader, err := zip.OpenReader(archive)
@@ -182,7 +184,7 @@ func unzip(ctx context.Context, archive, target string, fileFilter func(string)
 	}
 
 	for _, file := range reader.File {
-		path, err := buildPath(target, file.Name)
+		path, err := BuildPathChecked(target, file.Name)
 		if err != nil {
 			return nil, err
 		}

pkg/extract/extract_test.go

@@ -65,6 +65,19 @@ func TestZipSlip(t *testing.T) {
 	assert.ErrorContains(t, err, "illegal file path")
 }
 
+func TestBuildPathChecked(t *testing.T) {
+	baseDir := "/tmp/testDir"
+	validFilename := "file.txt"
+	invalidFilename := "../../etc/passwd"
+
+	joinedValid, err := BuildPathChecked(baseDir, validFilename)
+	assert.NoError(t, err)
+	assert.Equal(t, filepath.Join(baseDir, validFilename), joinedValid)
+
+	_, err = BuildPathChecked(baseDir, invalidFilename)
+	assert.ErrorContains(t, err, "illegal file path")
+}
+
 func copyFileMap(orig fileMap) fileMap {
 	copiedMap := fileMap{}
 	for key, value := range orig {

test/extended/util/prepare.go

@@ -96,8 +96,14 @@ func CleanTestRunDir() error {
 		return err
 	}
 
+	rootDir, err := os.OpenRoot(TestRunDir)
+	if err != nil {
+		return err
+	}
+	defer rootDir.Close()
+
 	for _, file := range files {
-		err := os.RemoveAll(filepath.Clean(filepath.Join(TestRunDir, file.Name())))
+		err = rootDir.RemoveAll(file.Name())
 		if err != nil {
 			return err
 		}

test/extended/util/util.go

@@ -68,11 +68,9 @@ func CopyResourcesFromPath(resourcesPath string) error {
 	}
 	destLoc, _ := os.Getwd()
 	for _, file := range files {
+		fmt.Printf("Copying %s to %s\n", filepath.Join(resourcesPath, file.Name()), destLoc)
 
-		sFileName := filepath.Join(resourcesPath, file.Name())
-		fmt.Printf("Copying %s to %s\n", sFileName, destLoc)
-
-		sFile, err := os.Open(filepath.Clean(sFileName))
+		sFile, err := os.OpenInRoot(resourcesPath, file.Name())
 		if err != nil {
 			fmt.Printf("Error occurred opening file: %s", err)
 			return err
@@ -112,7 +110,7 @@ func DownloadBundle(bundleLocation string, bundleDestination string, bundleName
 
 	if bundleLocation[:4] != "http" {
 
-		// copy the file locall
+		// copy the file locally
 
 		if bundleDestination == "." {
 			bundleDestination, _ = os.Getwd()
@@ -242,11 +240,11 @@ func SendCommandToVM(cmd string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	ssh, err := ssh.NewClient(connectionDetails.SSHUsername, connectionDetails.IP, connectionDetails.SSHPort, connectionDetails.SSHKeys...)
+	sshClient, err := ssh.NewClient(connectionDetails.SSHUsername, connectionDetails.IP, connectionDetails.SSHPort, connectionDetails.SSHKeys...)
 	if err != nil {
 		return "", err
 	}
-	out, _, err := ssh.Run(cmd)
+	out, _, err := sshClient.Run(cmd)
 	if err != nil {
 		return "", err
 	}