storage: keep recursive downloads inside the destination folder

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
 

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)

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
 }

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

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