From 1081cfc733291b51f2095e21de79cac3ac5b1dc3 Mon Sep 17 00:00:00 2001 From: Pierre-Emmanuel Jacquier <15922119+pierre-emmanuelJ@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:14:24 +0000 Subject: [PATCH 1/3] storage: keep recursive downloads inside the destination folder An object such as "public/../file" is a valid in-bucket path, but once the "public/" prefix is stripped by a recursive download it was written one level above the destination. Check the final local path instead of the object key, and skip the objects that would leave the destination. AI-assisted: true Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + pkg/storage/sos/object.go | 29 ++++- pkg/storage/sos/object_test.go | 110 ++++++++++++++++++ .../storage_download_path_traversal.txtar | 40 +++++++ 4 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 tests/e2e/scenarios/with-api/storage/storage_download_path_traversal.txtar diff --git a/CHANGELOG.md b/CHANGELOG.md index df59eb49c..f382400aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Bug fixes - NLB: `--description ""` now empties the description on `load-balancer update` and `load-balancer service update` (#919) +- storage: `download -r` now skips objects whose name would be written outside of the destination folder once the prefix is stripped (e.g. `public/../file` downloaded from `public/`) ### Improvements diff --git a/pkg/storage/sos/object.go b/pkg/storage/sos/object.go index 72d20d413..56634b573 100644 --- a/pkg/storage/sos/object.go +++ b/pkg/storage/sos/object.go @@ -243,8 +243,11 @@ func (c *Client) DownloadFiles( for _, object := range objects { key := aws.ToString(object.Key) - subpath := strings.TrimPrefix(key, prefix) - dst := filepath.Join(dst, subpath) // new local-scope dst variable! + dst, err := DownloadDestination(dst, prefix, key) // new local-scope dst variable! + if err != nil { + fmt.Fprintf(os.Stderr, "warning: Skipping file %s. File references a parent directory\n", key) + continue + } if !dryRun { err := os.MkdirAll(filepath.Dir(dst), 0o755) @@ -253,7 +256,7 @@ func (c *Client) DownloadFiles( } } - err := c.DownloadFile(ctx, bucket, dst, object, overwrite, dryRun) + err = c.DownloadFile(ctx, bucket, dst, object, overwrite, dryRun) if err != nil { // We might have downloaded files succesfuly before this error, // to quit with error now does not make much sense. @@ -848,6 +851,26 @@ func (o *ShowObjectOutput) ToTable() { }()}) } +// DownloadDestination returns the local path an object is written to when +// downloading the objects under prefix into the dst folder. It returns an error +// if that path is outside of dst, which happens if what is left of the key once +// the prefix is stripped references a parent directory (e.g. "public/../file" +// downloaded from the "public/" prefix). +func DownloadDestination(dst, prefix, key string) (string, error) { + if dst == "" { + dst = "." + } + + file := filepath.Join(dst, strings.TrimPrefix(key, prefix)) + + rel, err := filepath.Rel(dst, file) + if err != nil || !filepath.IsLocal(rel) { + return "", fmt.Errorf("object %q would be written outside of %q", key, dst) + } + + return file, nil +} + func IsTraversalPath(key string) bool { cleaned := path.Clean(key) return strings.HasPrefix(cleaned, "..") diff --git a/pkg/storage/sos/object_test.go b/pkg/storage/sos/object_test.go index 59531caf1..020e2cfe9 100644 --- a/pkg/storage/sos/object_test.go +++ b/pkg/storage/sos/object_test.go @@ -613,6 +613,116 @@ func Test_IsTraversalPath(t *testing.T) { } } +func TestDownloadDestination(t *testing.T) { + tests := []struct { + name string + dst string + prefix string + key string + expect string + wantErr bool + }{ + { + name: "object under prefix", + dst: "/tmp/victim", + prefix: "public/", + key: "public/a/file.txt", + expect: "/tmp/victim/a/file.txt", + }, + { + name: "bucket root prefix", + dst: "/tmp/victim", + prefix: "/", + key: "public/file.txt", + expect: "/tmp/victim/public/file.txt", + }, + { + name: "prefix without trailing separator", + dst: "/tmp/victim", + prefix: "public", + key: "public/file.txt", + expect: "/tmp/victim/file.txt", + }, + { + name: "parent reference staying in destination", + dst: "/tmp/victim", + prefix: "public/", + key: "public/a/../file.txt", + expect: "/tmp/victim/file.txt", + }, + { + name: "no destination", + prefix: "public/", + key: "public/a/file.txt", + expect: "a/file.txt", + }, + { + name: "parent reference in bucket escaping destination", + dst: "/tmp/victim", + prefix: "public/", + key: "public/../pwned.txt", + wantErr: true, + }, + { + name: "nested parent references escaping destination", + dst: "/tmp/victim", + prefix: "public/", + key: "public/a/../../pwned.txt", + wantErr: true, + }, + { + name: "parent reference with relative destination", + dst: "downloads", + prefix: "public/", + key: "public/../pwned.txt", + wantErr: true, + }, + { + name: "parent reference without destination", + prefix: "public/", + key: "public/../pwned.txt", + wantErr: true, + }, + { + name: "absolute path without destination", + prefix: "public", + key: "public/etc/pwned.txt", + expect: "etc/pwned.txt", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := sos.DownloadDestination(tt.dst, tt.prefix, tt.key) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, filepath.FromSlash(tt.expect), got) + }) + } +} + +func TestDownloadFiles_SkipsObjectsEscapingDestination(t *testing.T) { + parent := t.TempDir() + dst := filepath.Join(parent, "victim") + assert.NoError(t, os.Mkdir(dst, 0o755)) + + // The object is skipped before reaching the API, so no S3 client is needed. + client := &sos.Client{} + err := client.DownloadFiles( + context.Background(), + "bucket", "public/", "bucket/public/", dst, + []*types.Object{ + {Key: aws.String("public/../pwned/file.txt"), Size: aws.Int64(1)}, + }, + true, false, + ) + assert.NoError(t, err) + assert.NoDirExists(t, filepath.Join(parent, "pwned")) +} + func drainDeleteObjectVersions(deletedChan <-chan types.DeletedObject, errChan <-chan error) ([]types.DeletedObject, []error) { var deleted []types.DeletedObject var errs []error diff --git a/tests/e2e/scenarios/with-api/storage/storage_download_path_traversal.txtar b/tests/e2e/scenarios/with-api/storage/storage_download_path_traversal.txtar new file mode 100644 index 000000000..0a1d92065 --- /dev/null +++ b/tests/e2e/scenarios/with-api/storage/storage_download_path_traversal.txtar @@ -0,0 +1,40 @@ +# Recursive download must not write outside of the destination folder. +# An object key such as "public/../pwned.txt" is a valid in-bucket path, but once +# the "public/" prefix is stripped it would land one level above the destination. + +# Setup: create a bucket for this test run +exec exo --zone $TEST_ZONE --output-format json storage mb sos://dl-$TEST_RUN_ID +stdout '"name"' + +exec exo storage upload payload.txt sos://dl-$TEST_RUN_ID/public/../pwned.txt +exec exo storage upload hello.txt sos://dl-$TEST_RUN_ID/public/hello.txt +exec exo storage upload hello.txt sos://dl-$TEST_RUN_ID/public/sub/hello.txt + +# The escaping object is skipped with a warning, the others are downloaded +mkdir victim +exec exo storage download --recursive sos://dl-$TEST_RUN_ID/public/ victim +stderr 'Skipping file public/../pwned.txt' +! exists pwned.txt +exists victim/hello.txt +exists victim/sub/hello.txt + +# Dry-run skips it as well +exec exo storage download --recursive --dry-run sos://dl-$TEST_RUN_ID/public/ victim +stderr 'Skipping file public/../pwned.txt' +! stdout 'pwned.txt' + +# Downloading from the bucket root keeps the object inside the destination +mkdir root +exec exo storage download --recursive sos://dl-$TEST_RUN_ID/ root +exists root/pwned.txt +exists root/public/hello.txt +! exists pwned.txt + +# Teardown +exec exo storage delete --force --recursive sos://dl-$TEST_RUN_ID/ +exec exo storage rb -f sos://dl-$TEST_RUN_ID + +-- payload.txt -- +escaped +-- hello.txt -- +hello from e2e test From ea38b4a720d1fd651a58b64956f727b9d00ee4ec Mon Sep 17 00:00:00 2001 From: Pierre-Emmanuel Jacquier <15922119+pierre-emmanuelJ@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:14:33 +0000 Subject: [PATCH 2/3] Reference the PR in the changelog entry AI-assisted: true Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f382400aa..11e9c82ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ ### Bug fixes - NLB: `--description ""` now empties the description on `load-balancer update` and `load-balancer service update` (#919) -- storage: `download -r` now skips objects whose name would be written outside of the destination folder once the prefix is stripped (e.g. `public/../file` downloaded from `public/`) +- storage: `download -r` now skips objects whose name would be written outside of the destination folder once the prefix is stripped (e.g. `public/../file` downloaded from `public/`) (#920) ### Improvements From 176a15010ec3e7ff84b15d157f510556999143b0 Mon Sep 17 00:00:00 2001 From: Pierre-Emmanuel Jacquier <15922119+pierre-emmanuelJ@users.noreply.github.com> Date: Mon, 5 Oct 2026 13:02:09 +0000 Subject: [PATCH 3/3] storage: drop the object key traversal check The check on the local path the object is written to covers every case the key check did, and the key check missed some. Keeping both was confusing. AI-assisted: true Co-Authored-By: Claude Opus 5.5 --- cmd/storage/storage_download.go | 7 +----- pkg/storage/sos/object.go | 5 ---- pkg/storage/sos/object_test.go | 41 ++++++--------------------------- 3 files changed, 8 insertions(+), 45 deletions(-) diff --git a/cmd/storage/storage_download.go b/cmd/storage/storage_download.go index 039329607..4b7252286 100644 --- a/cmd/storage/storage_download.go +++ b/cmd/storage/storage_download.go @@ -95,12 +95,7 @@ Examples: objects := make([]*s3types.Object, 0) if err := storage.ForEachObject(exocmd.GContext, bucket, prefix, recursive, func(o *s3types.Object) error { - - if o.Key != nil && !sos.IsTraversalPath(*o.Key) { - objects = append(objects, o) - } else if o.Key != nil { - fmt.Printf("warning: Skipping file %s. File references a parent directory\n", *o.Key) - } + objects = append(objects, o) return nil }); err != nil { return fmt.Errorf("error listing objects: %s", err) diff --git a/pkg/storage/sos/object.go b/pkg/storage/sos/object.go index 56634b573..d2bd3ef0f 100644 --- a/pkg/storage/sos/object.go +++ b/pkg/storage/sos/object.go @@ -870,8 +870,3 @@ func DownloadDestination(dst, prefix, key string) (string, error) { return file, nil } - -func IsTraversalPath(key string) bool { - cleaned := path.Clean(key) - return strings.HasPrefix(cleaned, "..") -} diff --git a/pkg/storage/sos/object_test.go b/pkg/storage/sos/object_test.go index 020e2cfe9..1fe550c02 100644 --- a/pkg/storage/sos/object_test.go +++ b/pkg/storage/sos/object_test.go @@ -579,40 +579,6 @@ func TestUploadFiles(t *testing.T) { } } -func Test_IsTraversalPath(t *testing.T) { - tests := []struct { - path string - expect bool - }{ - { - path: "test.txt", - expect: false, - }, - { - path: "../test.txt", - expect: true, - }, - { - path: "a/b/../../../test.text", - expect: true, - }, - { - path: "a/b/../../test.txt", - expect: false, - }, - { - path: "../a/b/test.txt", - expect: true, - }, - } - - for _, ut := range tests { - t.Run(ut.path, func(t *testing.T) { - assert.Equal(t, ut.expect, sos.IsTraversalPath(ut.path)) - }) - } -} - func TestDownloadDestination(t *testing.T) { tests := []struct { name string @@ -670,6 +636,13 @@ func TestDownloadDestination(t *testing.T) { key: "public/a/../../pwned.txt", wantErr: true, }, + { + name: "parent reference from the bucket root", + dst: "/tmp/victim", + prefix: "/", + key: "../pwned.txt", + wantErr: true, + }, { name: "parent reference with relative destination", dst: "downloads",