From 9ea52db219e89f79ba6e4a4f39ad11679224923e Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 2 Sep 2026 11:36:58 -0700 Subject: [PATCH] s3: validate the version-id header used as a filer path segment (#11097) * s3: reject a version-id header that is not a valid path segment putToFiler stored the client-supplied Seaweed-X-Amz-Version-Id header verbatim into object metadata. That value is later read back and used as a filer path component when building the .versions/v_ path, so a value containing "/", "\" or ".." could steer retention/legal-hold writes and remote-cache reads outside the object's own bucket tree. Validate the header with isValidVersionID before storing it, the same check the versioned read paths already apply, and reject the request otherwise. Server-set version ids ("null" and generated hex) pass. Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY * s3: validate a stored version-id before using it as a path The retention and legal-hold sinks build a .versions/v_ path from a version id read back out of object metadata, and the remote-cache path builder does the same from either the request or the stored id, without the isValidVersionID check the other version-id consumers apply. Guard these so a value that is not a valid path segment falls back to the regular / unversioned path instead of steering the write or read out of the bucket tree. Claude-Session: https://claude.ai/code/session_011QqNaxZwnpHgMoAZNp3RkY --- weed/s3api/s3api_object_handlers.go | 4 ++- weed/s3api/s3api_object_handlers_put.go | 8 ++++- weed/s3api/s3api_object_retention.go | 8 +++-- .../s3api/s3api_remote_versioned_path_test.go | 29 +++++++++++++++++++ 4 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 weed/s3api/s3api_remote_versioned_path_test.go diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 2d5542515..99b80d652 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -3381,7 +3381,9 @@ func (s3a *S3ApiServer) doCacheRemoteObject(ctx context.Context, dir, name strin } func (s3a *S3ApiServer) buildVersionedRemoteObjectPath(bucket, object, versionId string) (dir, name string) { - if versionId != "" && versionId != "null" { + // versionId names a .versions file, so a value that is not a valid path + // segment falls back to the unversioned remote path rather than escaping it. + if versionId != "" && versionId != "null" && isValidVersionID(versionId) { normalizedObject := s3_constants.NormalizeObjectKey(object) return s3a.bucketDir(bucket) + "/" + normalizedObject + s3_constants.VersionsFolder, s3a.getVersionFileName(versionId) } diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 53a857272..f8990de43 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -788,8 +788,14 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader // Set object owner according to bucket ownership settings. s3a.setObjectOwnerFromRequest(r, bucket, entry) - // Set version ID if present + // Set version ID if present. It is later used as a filer path segment, so a + // value carrying "/", "\\" or ".." must never be stored. if versionIdHeader := r.Header.Get(s3_constants.ExtVersionIdKey); versionIdHeader != "" { + if !isValidVersionID(versionIdHeader) { + glog.Warningf("putToFiler: rejecting invalid version ID %q for object %s", versionIdHeader, filePath) + s3a.deleteOrphanedChunks(chunkResult.FileChunks) + return "", s3err.ErrInvalidRequest, SSEResponseMetadata{} + } entry.Extended[s3_constants.ExtVersionIdKey] = []byte(versionIdHeader) glog.V(3).Infof("putToFiler: setting version ID %s for object %s", versionIdHeader, filePath) } diff --git a/weed/s3api/s3api_object_retention.go b/weed/s3api/s3api_object_retention.go index 4cd1b039f..66b82cade 100644 --- a/weed/s3api/s3api_object_retention.go +++ b/weed/s3api/s3api_object_retention.go @@ -282,7 +282,9 @@ func (s3a *S3ApiServer) setObjectRetention(bucket, object, versionId string, ret if entry.Extended != nil { if versionIdBytes, exists := entry.Extended[s3_constants.ExtVersionIdKey]; exists { versionId = string(versionIdBytes) - if versionId != "null" { + // A stored version id must be a valid path segment before it can + // name a .versions file; otherwise fall back to the regular object. + if versionId != "null" && isValidVersionID(versionId) { entryPath = object + ".versions/" + s3a.getVersionFileName(versionId) } } @@ -425,7 +427,9 @@ func (s3a *S3ApiServer) setObjectLegalHold(bucket, object, versionId string, leg if entry.Extended != nil { if versionIdBytes, exists := entry.Extended[s3_constants.ExtVersionIdKey]; exists { versionId = string(versionIdBytes) - if versionId != "null" { + // A stored version id must be a valid path segment before it can + // name a .versions file; otherwise fall back to the regular object. + if versionId != "null" && isValidVersionID(versionId) { entryPath = object + ".versions/" + s3a.getVersionFileName(versionId) } } diff --git a/weed/s3api/s3api_remote_versioned_path_test.go b/weed/s3api/s3api_remote_versioned_path_test.go new file mode 100644 index 000000000..75b7d7d31 --- /dev/null +++ b/weed/s3api/s3api_remote_versioned_path_test.go @@ -0,0 +1,29 @@ +package s3api + +import "testing" + +func TestBuildVersionedRemoteObjectPathRejectsTraversal(t *testing.T) { + s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}} + + tests := []struct { + name string + versionId string + wantDir string + wantName string + }{ + {"valid version", "opaque_123", "/buckets/mybkt/obj.versions", "v_opaque_123"}, + {"traversal drops to unversioned", "v1/../../../buckets/victim/pwn", "/buckets/mybkt", "obj"}, + {"backslash drops to unversioned", `v1\..\victim`, "/buckets/mybkt", "obj"}, + {"empty is unversioned", "", "/buckets/mybkt", "obj"}, + {"null is unversioned", "null", "/buckets/mybkt", "obj"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir, name := s3a.buildVersionedRemoteObjectPath("mybkt", "obj", tt.versionId) + if dir != tt.wantDir || name != tt.wantName { + t.Errorf("buildVersionedRemoteObjectPath(mybkt, obj, %q) = (%q, %q), want (%q, %q)", + tt.versionId, dir, name, tt.wantDir, tt.wantName) + } + }) + } +}