From 4c73aa65760c1968c9a5b5cf30b2bec24b3ef065 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Mon, 4 Sep 2023 14:43:08 +0200 Subject: [PATCH 1/3] use SafeJoin function inside SavefileonTempDir --- utilities/utilities.go | 27 ++++++++++++++++++--- utilities/utilities_test.go | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 utilities/utilities_test.go diff --git a/utilities/utilities.go b/utilities/utilities.go index 075310b03..fc4686e9d 100644 --- a/utilities/utilities.go +++ b/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 +} diff --git a/utilities/utilities_test.go b/utilities/utilities_test.go new file mode 100644 index 000000000..6511a1719 --- /dev/null +++ b/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") + } +} From 9926e58c0e73be9074377a97140643ab01a71e7a Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Mon, 4 Sep 2023 15:27:41 +0200 Subject: [PATCH 2/3] use SafeJoin in the uploadHandler --- conn.go | 8 ++++++-- main_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/conn.go b/conn.go index 5fcc4727c..19f7a400d 100644 --- a/conn.go +++ b/conn.go @@ -140,7 +140,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) @@ -150,7 +154,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 diff --git a/main_test.go b/main_test.go index 238ae2a2d..79ea87a19 100644 --- a/main_test.go +++ b/main_test.go @@ -28,6 +28,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" @@ -48,6 +49,42 @@ func TestValidSignatureKey(t *testing.T) { require.NotNil(t, key) } +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") + } +} + func TestInstallToolDifferentContentType(t *testing.T) { r := gin.New() goa := v2.Server(config.GetDataDir().String()) From 139ce6bfae091767e1f48928c65f999db9784c70 Mon Sep 17 00:00:00 2001 From: Alessio Perugini Date: Tue, 5 Sep 2023 11:50:43 +0200 Subject: [PATCH 3/3] use SafeJoin on tools Remove handler --- v2/pkgs/tools.go | 7 ++++++- v2/pkgs/tools_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/v2/pkgs/tools.go b/v2/pkgs/tools.go index 13189c9e4..975fde233 100644 --- a/v2/pkgs/tools.go +++ b/v2/pkgs/tools.go @@ -31,6 +31,7 @@ import ( "strings" "github.com/arduino/arduino-create-agent/gen/tools" + "github.com/arduino/arduino-create-agent/utilities" "github.com/codeclysm/extract/v3" ) @@ -216,8 +217,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 } diff --git a/v2/pkgs/tools_test.go b/v2/pkgs/tools_test.go index 581b30b1f..70236cff3 100644 --- a/v2/pkgs/tools_test.go +++ b/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 {