Merge branch 'use-safe-join' into test-rc

Author: umbynos

conn.go

@@ -133,7 +133,11 @@ func uploadHandler(c *gin.Context) {
 	}
 
 	for _, extraFile := range data.ExtraFiles {
-		path := filepath.Join(tmpdir, extraFile.Filename)
+		path, err := utilities.SafeJoin(tmpdir, extraFile.Filename)
+		if err != nil {
+			c.String(http.StatusBadRequest, err.Error())
+			return
+		}
 		filePaths = append(filePaths, path)
 		log.Printf("Saving %s on %s", extraFile.Filename, path)
 
@@ -143,7 +147,7 @@ func uploadHandler(c *gin.Context) {
 			return
 		}
 
-		err := os.WriteFile(path, extraFile.Hex, 0644)
+		err = os.WriteFile(path, extraFile.Hex, 0644)
 		if err != nil {
 			c.String(http.StatusBadRequest, err.Error())
 			return

main_test.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/arduino/arduino-create-agent/config"
 	"github.com/arduino/arduino-create-agent/gen/tools"
+	"github.com/arduino/arduino-create-agent/upload"
 	v2 "github.com/arduino/arduino-create-agent/v2"
 	"github.com/gin-gonic/gin"
 	"github.com/stretchr/testify/require"
@@ -114,3 +115,38 @@ func TestInstallToolV2(t *testing.T) {
 		})
 	}
 }
+func TestUploadHandlerAgainstEvilFileNames(t *testing.T) {
+	r := gin.New()
+	r.POST("/", uploadHandler)
+	ts := httptest.NewServer(r)
+
+	uploadEvilFileName := Upload{
+		Port:       "/dev/ttyACM0",
+		Board:      "arduino:avr:uno",
+		Extra:      upload.Extra{Network: true},
+		Hex:        []byte("test"),
+		Filename:   "../evil.txt",
+		ExtraFiles: []additionalFile{{Hex: []byte("test"), Filename: "../evil.txt"}},
+	}
+	uploadEvilExtraFile := Upload{
+		Port:       "/dev/ttyACM0",
+		Board:      "arduino:avr:uno",
+		Extra:      upload.Extra{Network: true},
+		Hex:        []byte("test"),
+		Filename:   "file.txt",
+		ExtraFiles: []additionalFile{{Hex: []byte("test"), Filename: "../evil.txt"}},
+	}
+
+	for _, request := range []Upload{uploadEvilFileName, uploadEvilExtraFile} {
+		payload, err := json.Marshal(request)
+		require.NoError(t, err)
+
+		resp, err := http.Post(ts.URL, "encoding/json", bytes.NewBuffer(payload))
+		require.NoError(t, err)
+		require.Equal(t, http.StatusBadRequest, resp.StatusCode)
+
+		body, err := io.ReadAll(resp.Body)
+		require.NoError(t, err)
+		require.Contains(t, string(body), "unsafe path join")
+	}
+}

utilities/utilities.go

@@ -25,12 +25,15 @@ import (
 	"encoding/hex"
 	"encoding/pem"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"path"
 	"path/filepath"
 
+	"strings"
+
 	"github.com/arduino/arduino-create-agent/globals"
 )
 
@@ -40,15 +43,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
 	}
@@ -168,3 +177,16 @@ func VerifyInput(input string, signature string) error {
 	d := h.Sum(nil)
 	return rsa.VerifyPKCS1v15(rsaKey, crypto.SHA256, d, sign)
 }
+
+// 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")
+	}
+}

v2/pkgs/tools.go

@@ -223,8 +223,12 @@ func (c *Tools) install(ctx context.Context, path, url, checksum string) (*tools
 // Remove deletes the tool folder from Tools Folder
 func (c *Tools) Remove(ctx context.Context, payload *tools.ToolPayload) (*tools.Operation, error) {
 	path := filepath.Join(payload.Packager, payload.Name, payload.Version)
+	pathToRemove, err := utilities.SafeJoin(c.Folder, path)
+	if err != nil {
+		return nil, err
+	}
 
-	err := os.RemoveAll(filepath.Join(c.Folder, path))
+	err = os.RemoveAll(pathToRemove)
 	if err != nil {
 		return nil, err
 	}

v2/pkgs/tools_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/arduino/arduino-create-agent/gen/indexes"
 	"github.com/arduino/arduino-create-agent/gen/tools"
 	"github.com/arduino/arduino-create-agent/v2/pkgs"
+	"github.com/stretchr/testify/require"
 )
 
 // TestTools performs a series of operations about tools, ensuring it behaves as expected.
@@ -150,6 +151,33 @@ func TestTools(t *testing.T) {
 	if len(installed) != 1 {
 		t.Fatalf("expected %d == %d (%s)", len(installed), 1, "len(installed)")
 	}
+
+	t.Run("payload containing evil names", func(t *testing.T) {
+		evilFileNames := []string{
+			"/",
+			"..",
+			"../",
+			"../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",
+			}
+		}
+		for _, evilFileName := range evilFileNames {
+			// Here we could inject malicious name also in the Packager and Version field.
+			// Since the path is made by joining all of these 3 fields, we're using only the Name,
+			// as it won't change the result and let us keep the test small and readable.
+			_, err := service.Remove(ctx, &tools.ToolPayload{Name: evilFileName})
+			require.Error(t, err, evilFileName)
+			require.ErrorContains(t, err, "unsafe path join")
+		}
+	})
 }
 
 func strpoint(s string) *string {