diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd0a7dc1..d706fc66a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - NLB: `--description ""` now empties the description on `load-balancer update` and `load-balancer service update` (#919) - dbaas: fix `dbaas update` crashing on the `--opensearch-dashboard-*` and `--opensearch-index-template-*` flags, and `--opensearch-dashboard-max-old-space-size` being ignored on `dbaas create` and `dbaas update` (#921) - storage: `headers add/delete`, `metadata add/delete`, `copy` and `move` no longer report a 404 on an existing object whose key contains `+` or a percent-encoded sequence such as `%26` (#922) +- 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 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 72d20d413..d2bd3ef0f 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,7 +851,22 @@ func (o *ShowObjectOutput) ToTable() { }()}) } -func IsTraversalPath(key string) bool { - cleaned := path.Clean(key) - return strings.HasPrefix(cleaned, "..") +// 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 } diff --git a/pkg/storage/sos/object_test.go b/pkg/storage/sos/object_test.go index 59531caf1..1fe550c02 100644 --- a/pkg/storage/sos/object_test.go +++ b/pkg/storage/sos/object_test.go @@ -579,40 +579,123 @@ func TestUploadFiles(t *testing.T) { } } -func Test_IsTraversalPath(t *testing.T) { +func TestDownloadDestination(t *testing.T) { tests := []struct { - path string - expect bool + name string + dst string + prefix string + key string + expect string + wantErr bool }{ { - path: "test.txt", - expect: false, + name: "object under prefix", + dst: "/tmp/victim", + prefix: "public/", + key: "public/a/file.txt", + expect: "/tmp/victim/a/file.txt", }, { - path: "../test.txt", - expect: true, + name: "bucket root prefix", + dst: "/tmp/victim", + prefix: "/", + key: "public/file.txt", + expect: "/tmp/victim/public/file.txt", }, { - path: "a/b/../../../test.text", - expect: true, + name: "prefix without trailing separator", + dst: "/tmp/victim", + prefix: "public", + key: "public/file.txt", + expect: "/tmp/victim/file.txt", }, { - path: "a/b/../../test.txt", - expect: false, + name: "parent reference staying in destination", + dst: "/tmp/victim", + prefix: "public/", + key: "public/a/../file.txt", + expect: "/tmp/victim/file.txt", }, { - path: "../a/b/test.txt", - expect: true, + 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 from the bucket root", + dst: "/tmp/victim", + prefix: "/", + key: "../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 _, ut := range tests { - t.Run(ut.path, func(t *testing.T) { - assert.Equal(t, ut.expect, sos.IsTraversalPath(ut.path)) + 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