use SafeJoin function inside SavefileonTempDir

Author: alessio-perugini

utilities/utilities.go

@@ -19,11 +19,13 @@ import (
 	"archive/zip"
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"path"
 	"path/filepath"
+	"strings"
 )
 
 // SaveFileonTempDir creates a temp directory and saves the file data as the
@@ -32,15 +34,21 @@ import (
 // Returns an error if the filename doesn't form a valid path.
 //
 // Note that path could be defined and still there could be an error.
-func SaveFileonTempDir(filename string, data io.Reader) (path string, err error) {
-	// Create Temp Directory
+func SaveFileonTempDir(filename string, data io.Reader) (string, error) {
 	tmpdir, err := os.MkdirTemp("", "arduino-create-agent")
 	if err != nil {
 		return "", errors.New("Could not create temp directory to store downloaded file. Do you have permissions?")
 	}
+	return saveFileonTempDir(tmpdir, filename, data)
+}
 
+func saveFileonTempDir(tmpDir, filename string, data io.Reader) (string, error) {
+	path, err := SafeJoin(tmpDir, filename)
+	if err != nil {
+		return "", err
+	}
 	// Determine filename
-	filename, err = filepath.Abs(tmpdir + "/" + filename)
+	filename, err = filepath.Abs(path)
 	if err != nil {
 		return "", err
 	}
@@ -141,3 +149,16 @@ func Unzip(zippath string, destination string) (err error) {
 	}
 	return
 }
+
+// SafeJoin performs a filepath.Join of 'parent' and 'subdir' but returns an error
+// if the resulting path points outside of 'parent'.
+func SafeJoin(parent, subdir string) (string, error) {
+	res := filepath.Join(parent, subdir)
+	if !strings.HasSuffix(parent, string(os.PathSeparator)) {
+		parent += string(os.PathSeparator)
+	}
+	if !strings.HasPrefix(res, parent) {
+		return res, fmt.Errorf("unsafe path join: '%s' with '%s'", parent, subdir)
+	}
+	return res, nil
+}

utilities/utilities_test.go

@@ -0,0 +1,48 @@
+package utilities
+
+import (
+	"bytes"
+	"fmt"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSaveFileonTemp(t *testing.T) {
+	filename := "file"
+	tmpDir := t.TempDir()
+
+	path, err := saveFileonTempDir(tmpDir, filename, bytes.NewBufferString("TEST"))
+	require.NoError(t, err)
+	require.Equal(t, filepath.Join(tmpDir, filename), path)
+}
+
+func TestSaveFileonTempDirWithEvilName(t *testing.T) {
+	evilFileNames := []string{
+		"/",
+		"..",
+		"../",
+		"../evil.txt",
+		"../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+		"/some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
+	}
+	if runtime.GOOS == "windows" {
+		evilFileNames = []string{
+			"..\\",
+			"..\\evil.txt",
+			"..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+			"some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+			"\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+			"\\some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
+		}
+	}
+	for _, evilFileName := range evilFileNames {
+		_, err := saveFileonTempDir(t.TempDir(), evilFileName, bytes.NewBufferString("TEST"))
+		require.Error(t, err, fmt.Sprintf("with filename: '%s'", evilFileName))
+		require.ErrorContains(t, err, "unsafe path join")
+	}
+}