serve s3: fix path traversal letting clients see files in the root GHSA-8v25-v8p6-qf7v

S3 object keys are opaque names that may legally contain `..` segments. `serve
s3` built backend paths with `path.Join(bucket, key)`, which normalised the key
so a request such as `GET /bucket/../root-secret.txt` resolved to a file outside
the selected bucket elsewhere under the serve root. Listing prefixes and
multipart uploads were affected also.

This did not allow reading of files outside the root, but did allow reading of
files in the root which normally aren't visible; only directories are visible as
buckets normally.

Because `serve s3` maps keys to file paths it cannot represent every opaque S3
key, so rather than normalising keys (which would alias distinct keys onto one
file as well as allow traversal) it now rejects any key that is not already in
canonical path form - containing `..`, `.`, `//` or a leading or trailing slash
- with a 400 Bad Request, as MinIO does. Directory listing prefixes are
validated the same way but allow the empty bucket-root prefix and an optional
trailing slash.

Fixes: GHSA-8v25-v8p6-qf7v
(cherry picked from commit 83d1e62aa9)

cmd/serve/s3/backend.go

@@ -117,7 +117,10 @@ func (b *s3Backend) HeadObject(ctx context.Context, bucketName, objectName strin
 		return nil, gofakes3.BucketNotFound(bucketName)
 	}
 
-	fp := path.Join(bucketName, objectName)
+	fp, err := bucketObjectPath(bucketName, objectName)
+	if err != nil {
+		return nil, err
+	}
 	node, err := _vfs.Stat(fp)
 	if err != nil {
 		return nil, gofakes3.KeyNotFound(objectName)
@@ -172,7 +175,10 @@ func (b *s3Backend) GetObject(ctx context.Context, bucketName, objectName string
 		return nil, gofakes3.BucketNotFound(bucketName)
 	}
 
-	fp := path.Join(bucketName, objectName)
+	fp, err := bucketObjectPath(bucketName, objectName)
+	if err != nil {
+		return nil, err
+	}
 	node, err := _vfs.Stat(fp)
 	if err != nil {
 		return nil, gofakes3.KeyNotFound(objectName)
@@ -310,7 +316,10 @@ func (b *s3Backend) PutObject(
 		return result, gofakes3.BucketNotFound(bucketName)
 	}
 
-	fp := path.Join(bucketName, objectName)
+	fp, err := bucketObjectPath(bucketName, objectName)
+	if err != nil {
+		return result, err
+	}
 	objectDir := path.Dir(fp)
 	// _, err = db.fs.Stat(objectDir)
 	// if err == vfs.ENOENT {
@@ -403,7 +412,10 @@ func (b *s3Backend) deleteObject(ctx context.Context, bucketName, objectName str
 		return gofakes3.BucketNotFound(bucketName)
 	}
 
-	fp := path.Join(bucketName, objectName)
+	fp, err := bucketObjectPath(bucketName, objectName)
+	if err != nil {
+		return err
+	}
 	// S3 does not report an error when attempting to delete a key that does not exist, so
 	// we need to skip IsNotExist errors.
 	if err := _vfs.Remove(fp); err != nil && !os.IsNotExist(err) {
@@ -474,7 +486,10 @@ func (b *s3Backend) CopyObject(ctx context.Context, srcBucket, srcKey, dstBucket
 	if err != nil {
 		return result, err
 	}
-	fp := path.Join(srcBucket, srcKey)
+	fp, err := bucketObjectPath(srcBucket, srcKey)
+	if err != nil {
+		return result, err
+	}
 	if srcBucket == dstBucket && srcKey == dstKey {
 		b.meta.Store(fp, meta)
 

cmd/serve/s3/backend_test.go

@@ -0,0 +1,167 @@
+package s3
+
+import (
+	"context"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/rclone/gofakes3"
+	_ "github.com/rclone/rclone/backend/local"
+	"github.com/rclone/rclone/cmd/serve/proxy"
+	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fstest"
+	"github.com/rclone/rclone/vfs/vfscommon"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// newTestBackend serves a root directory containing a bucket directory, an
+// object inside that bucket and a root-level file that is not part of any
+// bucket. It returns the backend and the serve root path.
+func newTestBackend(t *testing.T) (*s3Backend, string) {
+	fstest.Initialise()
+	ctx := context.Background()
+	root := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(root, "bucket"), 0777))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "root-secret.txt"), []byte(rootSecret), 0666))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "bucket", "object.txt"), []byte("normal object"), 0666))
+
+	f, err := fs.NewFs(ctx, root)
+	require.NoError(t, err)
+
+	opt := Opt
+	opt.HTTP.ListenAddr = []string{endpoint}
+	w, err := newServer(ctx, f, &opt, &vfscommon.Opt, &proxy.Opt)
+	require.NoError(t, err)
+
+	return newBackend(w).(*s3Backend), root
+}
+
+const rootSecret = "ROOT_LEVEL_SECRET_MARKER"
+
+// TestPathTraversal checks that dot-dot segments in an object key cannot
+// escape the bucket namespace to read, overwrite or create files outside the
+// selected bucket.
+func TestPathTraversal(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("Get", func(t *testing.T) {
+		b, _ := newTestBackend(t)
+		_, err := b.GetObject(ctx, "bucket", "../root-secret.txt", nil)
+		assert.Error(t, err, "GET with dot-dot key must not read the root-level file")
+	})
+
+	t.Run("Head", func(t *testing.T) {
+		b, _ := newTestBackend(t)
+		_, err := b.HeadObject(ctx, "bucket", "../root-secret.txt")
+		assert.Error(t, err, "HEAD with dot-dot key must not find the root-level file")
+	})
+
+	t.Run("Put", func(t *testing.T) {
+		b, root := newTestBackend(t)
+		const payload = "OVERWRITTEN_BY_DOTDOT"
+		_, err := b.PutObject(ctx, "bucket", "../root-secret.txt", map[string]string{}, strings.NewReader(payload), int64(len(payload)))
+		assert.Error(t, err, "PUT with dot-dot key must not overwrite the root-level file")
+		got, readErr := os.ReadFile(filepath.Join(root, "root-secret.txt"))
+		require.NoError(t, readErr)
+		assert.Equal(t, rootSecret, string(got), "root-level file must be unchanged")
+	})
+
+	// path.Dir of the joined path can re-introduce a traversal, so check that a
+	// rejected PUT does not create directories outside the bucket as a side
+	// effect of making the object's parent directory.
+	t.Run("PutNoSideEffectDir", func(t *testing.T) {
+		b, root := newTestBackend(t)
+		_, err := b.PutObject(ctx, "bucket", "../sneaky-dir/f.txt", map[string]string{}, strings.NewReader("x"), 1)
+		assert.Error(t, err, "PUT with dot-dot key must be rejected")
+		_, statErr := os.Stat(filepath.Join(root, "sneaky-dir"))
+		assert.True(t, os.IsNotExist(statErr), "no directory must be created outside the bucket")
+	})
+
+	// A legitimate object inside the bucket must still be readable.
+	t.Run("LegitimateObject", func(t *testing.T) {
+		b, _ := newTestBackend(t)
+		obj, err := b.GetObject(ctx, "bucket", "object.txt", nil)
+		require.NoError(t, err)
+		defer func() { _ = obj.Contents.Close() }()
+		contents, err := io.ReadAll(obj.Contents)
+		require.NoError(t, err)
+		assert.Equal(t, "normal object", string(contents))
+	})
+
+	// A legitimate nested object must still be writable, creating its parent
+	// directory inside the bucket.
+	t.Run("LegitimateNestedPut", func(t *testing.T) {
+		b, root := newTestBackend(t)
+		const payload = "nested"
+		_, err := b.PutObject(ctx, "bucket", "sub/dir/file.txt", map[string]string{}, strings.NewReader(payload), int64(len(payload)))
+		require.NoError(t, err)
+		got, readErr := os.ReadFile(filepath.Join(root, "bucket", "sub", "dir", "file.txt"))
+		require.NoError(t, readErr)
+		assert.Equal(t, payload, string(got))
+	})
+}
+
+// TestBucketObjectPath unit-tests the bucket/key join used by serve s3. Object
+// keys are opaque, so non-canonical keys (containing "..", ".", "//", or
+// leading/trailing slashes) are rejected rather than normalised.
+func TestBucketObjectPath(t *testing.T) {
+	for _, test := range []struct {
+		bucket, key string
+		want        string
+		wantErr     bool
+	}{
+		{"bucket", "object.txt", "bucket/object.txt", false},
+		{"bucket", "a/b/c.txt", "bucket/a/b/c.txt", false},
+		{"bucket", "a/../b.txt", "", true}, // distinct from b.txt, not normalised
+		{"bucket", "a/./b.txt", "", true},
+		{"bucket", "a//b.txt", "", true},
+		{"bucket", "/leading", "", true},
+		{"bucket", "trailing/", "", true},
+		{"bucket", "", "", true},
+		{"bucket", "../root-secret.txt", "", true},
+		{"bucket", "../../etc/passwd", "", true},
+		{"bucket", "../otherbucket/x", "", true},
+		{"bucket", "a/../../escape", "", true},
+		{"bucket", "..", "", true},
+	} {
+		got, err := bucketObjectPath(test.bucket, test.key)
+		if test.wantErr {
+			assert.Error(t, err, "bucket=%q key=%q", test.bucket, test.key)
+			assert.True(t, gofakes3.HasErrorCode(err, gofakes3.ErrInvalidArgument), "want 400 InvalidArgument for key=%q, got %v", test.key, err)
+		} else {
+			require.NoError(t, err, "bucket=%q key=%q", test.bucket, test.key)
+			assert.Equal(t, test.want, got, "bucket=%q key=%q", test.bucket, test.key)
+		}
+	}
+}
+
+// TestBucketDirPath unit-tests the directory-prefix join: the empty prefix
+// addresses the bucket root and a trailing slash is ignored, but traversal and
+// other non-canonical prefixes are still rejected.
+func TestBucketDirPath(t *testing.T) {
+	for _, test := range []struct {
+		bucket, dir string
+		want        string
+		wantErr     bool
+	}{
+		{"bucket", "", "bucket", false}, // empty prefix addresses the bucket root
+		{"bucket", "dir", "bucket/dir", false},
+		{"bucket", "dir/", "", true}, // trailing slash not normalised away
+		{"bucket", "a/b", "bucket/a/b", false},
+		{"bucket", "../x", "", true},
+		{"bucket", "a//b", "", true},
+		{"bucket", "..", "", true},
+	} {
+		got, err := bucketDirPath(test.bucket, test.dir)
+		if test.wantErr {
+			assert.Error(t, err, "bucket=%q dir=%q", test.bucket, test.dir)
+		} else {
+			require.NoError(t, err, "bucket=%q dir=%q", test.bucket, test.dir)
+			assert.Equal(t, test.want, got, "bucket=%q dir=%q", test.bucket, test.dir)
+		}
+	}
+}

cmd/serve/s3/list.go

@@ -8,8 +8,12 @@ import (
 	"github.com/rclone/rclone/vfs"
 )
 
-func (b *s3Backend) entryListR(_vfs *vfs.VFS, bucket, fdPath, name string, addPrefix bool, response *gofakes3.ObjectList) error {
-	fp := path.Join(bucket, fdPath)
+func (b *s3Backend) entryListR(_vfs *vfs.VFS, bucketName, fdPath, name string, addPrefix bool, response *gofakes3.ObjectList) error {
+	fp, err := bucketDirPath(bucketName, fdPath)
+	if err != nil {
+		// A listing prefix that can't be represented as a path matches nothing.
+		return gofakes3.ErrNoSuchKey
+	}
 
 	dirEntries, err := getDirEntries(fp, _vfs)
 	if err != nil {
@@ -32,7 +36,7 @@ func (b *s3Backend) entryListR(_vfs *vfs.VFS, bucket, fdPath, name string, addPr
 				response.AddPrefix(prefixWithTrailingSlash)
 				continue
 			}
-			err := b.entryListR(_vfs, bucket, path.Join(fdPath, object), "", false, response)
+			err := b.entryListR(_vfs, bucketName, path.Join(fdPath, object), "", false, response)
 			if err != nil {
 				return err
 			}

cmd/serve/s3/serve_s3.md

@@ -131,6 +131,17 @@ Versioning is not currently supported.
 Metadata will only be saved in memory other than the rclone `mtime`
 metadata which will be set as the modification time of the file.
 
+### Object names
+
+`serve s3` stores objects as files in the backend, so object keys are
+mapped to file paths rather than treated as the opaque strings AWS S3
+allows. Keys must be in canonical path form: keys that contain `..` or
+`.` path segments, repeated slashes (`//`), or a leading or trailing
+slash are rejected with a `400 Bad Request` (`InvalidArgument`)
+instead of being normalised, since normalising them could alias two
+distinct keys to the same file or resolve a key outside its bucket.
+This matches the behaviour of other S3 servers such as MinIO.
+
 ### Supported operations
 
 `serve s3` currently supports the following operations.

cmd/serve/s3/utils.go

@@ -86,6 +86,50 @@ func getFileHash(node any, hashType hash.Type) string {
 	return hash
 }
 
+// canonicalKey reports whether key is a non-empty path in canonical form: it
+// is equal to the path it cleans to, so it has no ".", ".." or empty
+// (repeated, leading or trailing slash) segments.
+//
+// The key is anchored with a leading "/" before cleaning so that a leading
+// ".." cannot silently escape upwards and still compare equal.
+func canonicalKey(key string) bool {
+	return key != "" && "/"+key == path.Clean("/"+key)
+}
+
+// errInvalidObjectName is returned for object keys that cannot be represented
+// as a backend path, for example because they contain "..", "." or "//"
+// segments. Like MinIO, this is reported as a 400 Bad Request rather than
+// silently resolving the key to a different object (or one outside the
+// bucket).
+func errInvalidObjectName(key string) error {
+	return gofakes3.ErrorMessagef(gofakes3.ErrInvalidArgument, "Object name contains unsupported characters: %q", key)
+}
+
+// bucketObjectPath joins the bucket name and object key into a backend path.
+//
+// S3 object keys are opaque, so rclone treats "dir/../file" and "file" as
+// distinct keys and refuses to normalise one into the other. Keys that are
+// not in canonical form (or that would escape the bucket) are rejected with
+// errInvalidObjectName rather than resolved.
+func bucketObjectPath(bucketName, objectName string) (string, error) {
+	if !canonicalKey(objectName) {
+		return "", errInvalidObjectName(objectName)
+	}
+	return path.Join(bucketName, objectName), nil
+}
+
+// bucketDirPath joins the bucket name and a directory path (a listing prefix)
+// into a backend path. It is like bucketObjectPath except that the empty path
+// is allowed and addresses the bucket root. Every other non-canonical path,
+// including one with a trailing slash, is rejected - it is not normalised, so
+// "dir" and "dir/" are not treated as the same directory.
+func bucketDirPath(bucketName, dirName string) (string, error) {
+	if dirName == "" {
+		return bucketName, nil
+	}
+	return bucketObjectPath(bucketName, dirName)
+}
+
 func prefixParser(p *gofakes3.Prefix) (path, remaining string) {
 	idx := strings.LastIndexByte(p.Prefix, '/')
 	if idx < 0 {