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 {