From c89b766cf417fddbe7eace40d31262ecb85bfa93 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 29 Jun 2026 16:09:56 +0100 Subject: [PATCH] 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 83d1e62aa9e0dbd10a5d7eb34c117ae997268cdf) --- cmd/serve/s3/backend.go | 25 ++++-- cmd/serve/s3/backend_test.go | 167 +++++++++++++++++++++++++++++++++++ cmd/serve/s3/list.go | 10 ++- cmd/serve/s3/serve_s3.md | 11 +++ cmd/serve/s3/utils.go | 44 +++++++++ 5 files changed, 249 insertions(+), 8 deletions(-) create mode 100644 cmd/serve/s3/backend_test.go diff --git a/cmd/serve/s3/backend.go b/cmd/serve/s3/backend.go index dea7720f7..7ff6941ed 100644 --- a/cmd/serve/s3/backend.go +++ b/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) diff --git a/cmd/serve/s3/backend_test.go b/cmd/serve/s3/backend_test.go new file mode 100644 index 000000000..eb792c89c --- /dev/null +++ b/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) + } + } +} diff --git a/cmd/serve/s3/list.go b/cmd/serve/s3/list.go index 559fa14a4..3f87f7ccb 100644 --- a/cmd/serve/s3/list.go +++ b/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 } diff --git a/cmd/serve/s3/serve_s3.md b/cmd/serve/s3/serve_s3.md index a1281b515..e7d35f539 100644 --- a/cmd/serve/s3/serve_s3.md +++ b/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. diff --git a/cmd/serve/s3/utils.go b/cmd/serve/s3/utils.go index cb6a926a3..a84857766 100644 --- a/cmd/serve/s3/utils.go +++ b/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 {