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) + } + }) + } +}