diff --git a/weed/s3api/copy_source_decode_test.go b/weed/s3api/copy_source_decode_test.go index 5d7713e744..9260884f65 100644 --- a/weed/s3api/copy_source_decode_test.go +++ b/weed/s3api/copy_source_decode_test.go @@ -158,6 +158,8 @@ func TestCopySourceRejectsTraversal(t *testing.T) { {"bare dot segment rejected", "bucket-a/./key", true}, {"dotdot bucket rejected", "../bucket-secret/flag", true}, {"dotdot with versionId escapes bucket", "bucket-a/../bucket-secret/flag?versionId=v1", true}, + {"versionId encoded slash rejected", "bucket-a/dir/key?versionId=v1%2F..%2Fsecret", true}, + {"versionId backslash rejected", "bucket-a/dir/key?versionId=v1%5C..%5Csecret", true}, } for _, tc := range tests { @@ -166,9 +168,9 @@ func TestCopySourceRejectsTraversal(t *testing.T) { if err != nil { cpSrcPath = tc.rawCopySource } - srcBucket, srcObject, _ := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath) + srcBucket, srcObject, srcVersionId := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath) - gotErr := ValidateCopySource(cpSrcPath, srcBucket, srcObject) != nil + gotErr := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId) != nil if gotErr != tc.wantErr { t.Errorf("ValidateCopySource(%q) error = %v, want %v (parsed bucket=%q object=%q)", tc.rawCopySource, gotErr, tc.wantErr, srcBucket, srcObject) diff --git a/weed/s3api/iceberg/handlers_table.go b/weed/s3api/iceberg/handlers_table.go index 88fca0b16c..334765b0a3 100644 --- a/weed/s3api/iceberg/handlers_table.go +++ b/weed/s3api/iceberg/handlers_table.go @@ -175,6 +175,18 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) { return } + // location/name are client-supplied; confine the metadata write to the + // authorized catalog bucket and reject traversal segments so path.Join in + // saveMetadataFile cannot escape into another bucket. + if metadataBucket != bucketName { + writeError(w, http.StatusBadRequest, "BadRequestException", "table location must be within bucket "+bucketName) + return + } + if !isValidTablePath(metadataPath) { + writeError(w, http.StatusBadRequest, "BadRequestException", "invalid table location path") + return + } + // Authoritative existence check: ask the catalog whether a table is registered // at this name. If it is, short-circuit with the existing table (idempotent // CreateTable). Any leftover objects at the target path from a previous diff --git a/weed/s3api/iceberg/path_validation.go b/weed/s3api/iceberg/path_validation.go index faf044881d..c1911ae110 100644 --- a/weed/s3api/iceberg/path_validation.go +++ b/weed/s3api/iceberg/path_validation.go @@ -56,6 +56,21 @@ func validateRequestPath(next http.Handler) http.Handler { }) } +// isValidTablePath reports whether a "/"-separated table path (the part below +// the bucket) is free of segments that path.Join would collapse to escape the +// bucket directory. Empty segments (from leading/duplicate slashes) are ignored. +func isValidTablePath(tablePath string) bool { + for _, segment := range strings.Split(tablePath, "/") { + if segment == "" { + continue + } + if !isValidNameSegment(segment) { + return false + } + } + return true +} + // isValidNameSegment rejects a single path-segment value (bucket prefix slot, // table name, or one namespace part) that would be unsafe to embed in a filer // path: `.`, `..`, embedded slash/backslash, or NUL. diff --git a/weed/s3api/iceberg/path_validation_test.go b/weed/s3api/iceberg/path_validation_test.go index eaeea07379..fe7a0912bf 100644 --- a/weed/s3api/iceberg/path_validation_test.go +++ b/weed/s3api/iceberg/path_validation_test.go @@ -8,6 +8,27 @@ import ( "github.com/gorilla/mux" ) +func TestIsValidTablePath(t *testing.T) { + tests := []struct { + tablePath string + want bool + }{ + {"sales.orders", true}, + {"ns/table", true}, + {"ns//table", true}, + {"", true}, + {"../other", false}, + {"ns/../../etc", false}, + {"ns/./x", false}, + {`ns\..\x`, false}, + } + for _, tt := range tests { + if got := isValidTablePath(tt.tablePath); got != tt.want { + t.Errorf("isValidTablePath(%q) = %v, want %v", tt.tablePath, got, tt.want) + } + } +} + func TestValidateRequestPath_RejectsTraversal(t *testing.T) { tests := []struct { name string diff --git a/weed/s3api/s3_constants/header.go b/weed/s3api/s3_constants/header.go index 97afcb3920..81314eca15 100644 --- a/weed/s3api/s3_constants/header.go +++ b/weed/s3api/s3_constants/header.go @@ -212,6 +212,16 @@ func IsValidBucketName(bucket string) bool { return !strings.ContainsAny(bucket, "/\\\x00") } +// IsValidPathSegment reports whether value is safe to use as one filer path +// component. It intentionally does not enforce any application-specific +// format; callers can layer stricter checks on top. +func IsValidPathSegment(value string) bool { + if value == "" || value == "." || value == ".." { + return false + } + return !strings.ContainsAny(value, "/\\\x00") +} + // NormalizeObjectKey normalizes object keys by removing duplicate slashes and converting backslashes. // This normalizes keys from various sources (URL path, form values, etc.) to a consistent format. // It also converts Windows-style backslashes to forward slashes for cross-platform compatibility. diff --git a/weed/s3api/s3_constants/header_test.go b/weed/s3api/s3_constants/header_test.go index dc001ec0a6..7e45882710 100644 --- a/weed/s3api/s3_constants/header_test.go +++ b/weed/s3api/s3_constants/header_test.go @@ -148,6 +148,29 @@ func TestIsValidBucketName(t *testing.T) { } } +func TestIsValidPathSegment(t *testing.T) { + tests := []struct { + input string + want bool + }{ + {"opaque-id_123", true}, + {"..hidden", true}, + {"", false}, + {".", false}, + {"..", false}, + {"dir/value", false}, + {"dir\\value", false}, + {"nul\x00value", false}, + } + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + if got := IsValidPathSegment(tt.input); got != tt.want { + t.Errorf("IsValidPathSegment(%q) = %v, want %v", tt.input, got, tt.want) + } + }) + } +} + func TestRemoveDuplicateSlashes(t *testing.T) { tests := []struct { name string diff --git a/weed/s3api/s3api_copy_validation.go b/weed/s3api/s3api_copy_validation.go index 15b456629f..ca4413b429 100644 --- a/weed/s3api/s3api_copy_validation.go +++ b/weed/s3api/s3api_copy_validation.go @@ -242,8 +242,12 @@ func validateEncryptionContext(contextHeader string) error { return nil } -// ValidateCopySource validates the copy source path and permissions +// ValidateCopySource validates the copy source path. func ValidateCopySource(copySource string, srcBucket, srcObject string) error { + return validateCopySource(copySource, srcBucket, srcObject, "") +} + +func validateCopySource(copySource string, srcBucket, srcObject, srcVersionId string) error { if copySource == "" { return &CopyValidationError{ Code: s3err.ErrInvalidCopySource, @@ -273,6 +277,12 @@ func ValidateCopySource(copySource string, srcBucket, srcObject string) error { Message: "Copy source contains invalid path segments", } } + if !isValidVersionID(srcVersionId) { + return &CopyValidationError{ + Code: s3err.ErrInvalidCopySource, + Message: "Copy source contains an invalid version ID", + } + } return nil } diff --git a/weed/s3api/s3api_multipart_path_validation_test.go b/weed/s3api/s3api_multipart_path_validation_test.go new file mode 100644 index 0000000000..a9013a68ec --- /dev/null +++ b/weed/s3api/s3api_multipart_path_validation_test.go @@ -0,0 +1,39 @@ +package s3api + +import ( + "strings" + "testing" +) + +func TestCheckUploadIDRequiresGeneratedFormat(t *testing.T) { + s3a := &S3ApiServer{} + const object = "dir/object" + hash := s3a.generateUploadID(object) + + tests := []struct { + name string + uploadID string + wantErr bool + }{ + {"legacy hash", hash, false}, + {"current format", hash + "_" + strings.Repeat("a", 32), false}, + {"wrong object hash", strings.Repeat("0", 40) + "_" + strings.Repeat("a", 32), true}, + {"arbitrary suffix", hash + "_suffix", true}, + {"uppercase suffix", hash + "_" + strings.Repeat("A", 32), true}, + {"short suffix", hash + "_" + strings.Repeat("a", 31), true}, + {"long suffix", hash + "_" + strings.Repeat("a", 33), true}, + {"slash traversal", hash + "/../../../victim-bucket", true}, + {"suffix traversal", hash + "_" + strings.Repeat("a", 32) + "/../../../victim-bucket", true}, + {"backslash traversal", hash + `\..\..\victim-bucket`, true}, + {"nul suffix", hash + "_abc\x00def", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotErr := s3a.checkUploadId(object, tt.uploadID) != nil + if gotErr != tt.wantErr { + t.Errorf("checkUploadId(%q) error = %v, want %v", tt.uploadID, gotErr, tt.wantErr) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 8b87d97794..79fe4f0633 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -101,7 +101,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request } // Validate copy source and destination - if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil { + if err := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId); err != nil { glog.V(2).Infof("CopyObjectHandler validation error: %v", err) errCode := MapCopyValidationError(err) s3err.WriteErrorResponse(w, r, errCode) @@ -765,7 +765,7 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req } // Validate the copy source as CopyObject does. - if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil { + if err := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId); err != nil { glog.V(2).Infof("CopyObjectPartHandler validation error: %v", err) s3err.WriteErrorResponse(w, r, MapCopyValidationError(err)) return diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index 7daaa24691..0363ea0157 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -34,6 +34,13 @@ func deleteErrorFromCode(code s3err.ErrorCode, key, versionId string) DeleteErro } } +func validateDeleteObjectIdentifier(object ObjectIdentifier) s3err.ErrorCode { + if !s3_constants.IsValidObjectKey(object.Key) || !isValidVersionID(object.VersionId) { + return s3err.ErrInvalidRequest + } + return s3err.ErrNone +} + // isMissingDeleteConditionTarget normalizes missing-target detection for conditional deletes. // Prefer errors.Is(err, filer_pb.ErrNotFound) and errors.Is(err, ErrDeleteMarker); keep the // string-based fallback only as a defensive bridge for filer paths that still return plain text. @@ -50,6 +57,9 @@ func isMissingDeleteConditionTarget(err error) bool { } func (s3a *S3ApiServer) resolveDeleteConditionalEntry(bucket, object, versionId, versioningState string) (*filer_pb.Entry, error) { + if !isValidVersionID(versionId) { + return nil, errInvalidVersionID + } normalizedObject := s3_constants.NormalizeObjectKey(object) bucketDir := s3a.bucketDir(bucket) @@ -173,6 +183,9 @@ func (s3a *S3ApiServer) deleteVersionedObject(r *http.Request, bucket, object, v // when the entry's Attributes.TtlSec > 0 so the volume is guaranteed to // drop the chunks on its own. func (s3a *S3ApiServer) deleteUnversionedObjectWithClient(client filer_pb.SeaweedFilerClient, bucket, object string, metadataOnly bool) error { + if !s3_constants.IsValidBucketName(bucket) || !s3_constants.IsValidObjectKey(object) { + return errors.New("invalid bucket or object path") + } target := util.NewFullPath(s3a.bucketDir(bucket), object) dir, name := target.DirAndName() return deleteObjectEntry(client, dir, name, !metadataOnly, false) @@ -423,6 +436,10 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h if object.Key == "" { continue } + if validationCode := validateDeleteObjectIdentifier(object); validationCode != s3err.ErrNone { + deleteErrors = append(deleteErrors, deleteErrorFromCode(validationCode, object.Key, object.VersionId)) + continue + } if err := s3a.validateTableBucketObjectPath(bucket, object.Key); err != nil { deleteErrors = append(deleteErrors, deleteErrorFromCode(s3err.ErrAccessDenied, object.Key, object.VersionId)) continue diff --git a/weed/s3api/s3api_object_handlers_delete_test.go b/weed/s3api/s3api_object_handlers_delete_test.go index 7ee8216cbf..b2104e257f 100644 --- a/weed/s3api/s3api_object_handlers_delete_test.go +++ b/weed/s3api/s3api_object_handlers_delete_test.go @@ -1,13 +1,45 @@ package s3api import ( + "errors" "testing" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestValidateDeleteObjectIdentifier(t *testing.T) { + tests := []struct { + name string + identifier ObjectIdentifier + want s3err.ErrorCode + }{ + {"clean key", ObjectIdentifier{Key: "dir/key"}, s3err.ErrNone}, + {"clean version", ObjectIdentifier{Key: "dir/key", VersionId: "opaque-version"}, s3err.ErrNone}, + {"key traversal", ObjectIdentifier{Key: "../victim/key"}, s3err.ErrInvalidRequest}, + {"encoded traversal already decoded", ObjectIdentifier{Key: "dir/../../victim/key"}, s3err.ErrInvalidRequest}, + {"backslash traversal", ObjectIdentifier{Key: `..\victim\key`}, s3err.ErrInvalidRequest}, + {"version traversal", ObjectIdentifier{Key: "key", VersionId: "v1/../../../victim"}, s3err.ErrInvalidRequest}, + {"version backslash", ObjectIdentifier{Key: "key", VersionId: `v1\..\victim`}, s3err.ErrInvalidRequest}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, validateDeleteObjectIdentifier(tt.identifier)) + }) + } +} + +func TestGetSpecificObjectVersionRejectsUnsafeVersionID(t *testing.T) { + s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}} + + _, err := s3a.getSpecificObjectVersion("bucket", "key", "v1/../../../victim") + + require.Error(t, err) + assert.True(t, errors.Is(err, errInvalidVersionID)) +} + func TestDeleteUnversionedObjectWithClient_MetadataOnlySkipsChunkDelete(t *testing.T) { // metadataOnly=true must reach the filer as IsDeleteData=false so the // volume server reclaims chunks via TTL instead of the filer enqueueing @@ -50,6 +82,16 @@ func TestDeleteUnversionedObjectWithClient_FullPathFromBucketsRoot(t *testing.T) assert.Equal(t, "c.txt", client.deleteReq.Name) } +func TestDeleteUnversionedObjectWithClientRejectsTraversal(t *testing.T) { + s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}} + client := &deleteObjectEntryTestClient{} + + err := s3a.deleteUnversionedObjectWithClient(client, "source-bucket", "../victim-bucket/secret", false) + + require.Error(t, err) + assert.Nil(t, client.deleteReq, "invalid path must be rejected before a filer delete RPC") +} + func TestDeleteUnversionedObjectWithClient_PropagatesEntryAttributesIrrelevant(t *testing.T) { // The metadataOnly decision is the caller's responsibility (the // lifecycle handler). This function is dumb plumbing — it must not diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index 43084be69b..7f2feca5de 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -521,13 +521,32 @@ func (s3a *S3ApiServer) checkUploadId(object string, id string) error { hash := s3a.generateUploadID(object) - if !strings.HasPrefix(id, hash) { + // uploadID becomes a filer directory name. Accept the historical hash-only + // form and the exact current hash_UUID form, not arbitrary hash-prefixed paths. + valid := id == hash // legacy upload IDs generated before the UUID suffix was added + if len(id) == len(hash)+1+32 && strings.HasPrefix(id, hash+"_") { + valid = isLowerHex(id[len(hash)+1:]) + } + if !valid { glog.Errorf("object %s and uploadID %s are not matched", object, id) return fmt.Errorf("object %s and uploadID %s are not matched", object, id) } return nil } +func isLowerHex(value string) bool { + if value == "" { + return false + } + for i := 0; i < len(value); i++ { + c := value[i] + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + // Parse bucket url queries for ?uploads func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int, encodingType string) { prefix = values.Get("prefix") diff --git a/weed/s3api/s3api_object_handlers_postpolicy.go b/weed/s3api/s3api_object_handlers_postpolicy.go index b3cab13b44..ce3d74d697 100644 --- a/weed/s3api/s3api_object_handlers_postpolicy.go +++ b/weed/s3api/s3api_object_handlers_postpolicy.go @@ -56,7 +56,12 @@ func (s3a *S3ApiServer) PostPolicyBucketHandler(w http.ResponseWriter, r *http.R if fileName != "" && strings.Contains(formValues.Get("Key"), "${filename}") { formValues.Set("Key", strings.Replace(formValues.Get("Key"), "${filename}", fileName, -1)) } - object := s3_constants.NormalizeObjectKey(formValues.Get("Key")) + rawObject := formValues.Get("Key") + if rawObject == "" || !s3_constants.IsValidObjectKey(rawObject) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + return + } + object := s3_constants.NormalizeObjectKey(rawObject) if err := s3a.validateTableBucketObjectPath(bucket, object); err != nil { s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) return diff --git a/weed/s3api/s3api_object_handlers_postpolicy_test.go b/weed/s3api/s3api_object_handlers_postpolicy_test.go index 8e566d55e8..cd6b62e0ef 100644 --- a/weed/s3api/s3api_object_handlers_postpolicy_test.go +++ b/weed/s3api/s3api_object_handlers_postpolicy_test.go @@ -555,6 +555,32 @@ func TestPostPolicyBucketHandlerKeyExtraction(t *testing.T) { } } +func TestPostPolicyBucketHandlerRejectsTraversalKey(t *testing.T) { + var buf bytes.Buffer + writer := multipart.NewWriter(&buf) + assert.NoError(t, writer.WriteField("key", "uploads/../../victim-bucket/secret")) + assert.NoError(t, writer.WriteField("Policy", "")) + part, err := writer.CreateFormFile("file", "payload.txt") + assert.NoError(t, err) + _, err = part.Write([]byte("secret")) + assert.NoError(t, err) + assert.NoError(t, writer.Close()) + + req := httptest.NewRequest(http.MethodPost, "/source-bucket", &buf) + req.Header.Set("Content-Type", writer.FormDataContentType()) + req = mux.SetURLVars(req, map[string]string{"bucket": "source-bucket"}) + rec := httptest.NewRecorder() + + s3a := &S3ApiServer{ + option: &S3ApiServerOption{BucketsPath: "/buckets"}, + iam: &IdentityAccessManagement{}, + } + s3a.PostPolicyBucketHandler(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "InvalidRequest") +} + // TestPostPolicyBucketHandler_PolicyViolationReturns403 drives the handler // end-to-end with a signed multipart POST whose policy conditions cannot be // satisfied by the form fields. It verifies the handler responds 403 diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index ca6b236966..45b18527e3 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -371,6 +371,9 @@ type putFinalize struct { // targets and a part write would otherwise bind a TTL clock starting // before CompleteMultipartUpload. func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader io.Reader, bucket string, object string, partNumber int, lifecycleTTLSec int32, finalize *putFinalize, uniqueWritePath bool) (etag string, code s3err.ErrorCode, sseMetadata SSEResponseMetadata) { + if !s3_constants.IsValidBucketName(bucket) || (object != "" && !s3_constants.IsValidObjectKey(object)) { + return "", s3err.ErrInvalidRequest, SSEResponseMetadata{} + } // NEW OPTIMIZATION: Write directly to volume servers, bypassing filer proxy // This eliminates the filer proxy overhead for PUT operations // Note: filePath is now passed directly instead of URL (no parsing needed) diff --git a/weed/s3api/s3api_object_versioned_finalize.go b/weed/s3api/s3api_object_versioned_finalize.go index ca1bd978dd..d377285ced 100644 --- a/weed/s3api/s3api_object_versioned_finalize.go +++ b/weed/s3api/s3api_object_versioned_finalize.go @@ -112,6 +112,9 @@ func wormDeleteCondition(worm, bypass bool) *filer_pb.WriteCondition { // object (serializing the pointer recompute); for object-lock buckets the // condition gates the delete on the version's WORM guards evaluated on the owner. func (s3a *S3ApiServer) routedDeleteSpecificVersion(owner pb.ServerAddress, bucket, object, versionId string, worm, bypass bool) s3err.ErrorCode { + if !isValidVersionID(versionId) { + return s3err.ErrInvalidRequest + } versionFileName := s3a.getVersionFileName(versionId) versionsPath := s3a.toFilerPath(bucket, object+s3_constants.VersionsFolder) cond := wormDeleteCondition(worm, bypass) diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index 14c5e47b44..fce90a8250 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -1033,6 +1033,9 @@ func (s3a *S3ApiServer) calculateETagFromChunks(chunks []*filer_pb.FileChunk) st // getSpecificObjectVersion retrieves a specific version of an object func (s3a *S3ApiServer) getSpecificObjectVersion(bucket, object, versionId string) (*filer_pb.Entry, error) { + if !isValidVersionID(versionId) { + return nil, errInvalidVersionID + } // Normalize object path to ensure consistency with toFilerPath behavior normalizedObject := s3_constants.NormalizeObjectKey(object) @@ -1068,6 +1071,9 @@ func (s3a *S3ApiServer) getSpecificObjectVersion(bucket, object, versionId strin // pass true when the live entry's Attributes.TtlSec > 0 so the volume // reclaims chunks on its own. func (s3a *S3ApiServer) deleteSpecificObjectVersion(ctx context.Context, bucket, object, versionId string, metadataOnly bool) error { + if !isValidVersionID(versionId) { + return errInvalidVersionID + } // Normalize object path to ensure consistency with toFilerPath behavior normalizedObject := s3_constants.NormalizeObjectKey(object) diff --git a/weed/s3api/s3api_path_validation.go b/weed/s3api/s3api_path_validation.go index 0ca22789ce..e95140291c 100644 --- a/weed/s3api/s3api_path_validation.go +++ b/weed/s3api/s3api_path_validation.go @@ -2,12 +2,52 @@ package s3api import ( "net/http" + "net/url" + "strings" "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" ) +func hasPathSegmentQuery(rawQuery string) bool { + if strings.Contains(rawQuery, "versionId") || strings.Contains(rawQuery, "uploadId") { + return true + } + if !strings.Contains(rawQuery, "%") { + return false + } + + for rawQuery != "" { + field := rawQuery + if i := strings.IndexByte(rawQuery, '&'); i >= 0 { + field, rawQuery = rawQuery[:i], rawQuery[i+1:] + } else { + rawQuery = "" + } + if i := strings.IndexByte(field, '='); i >= 0 { + field = field[:i] + } + if !strings.Contains(field, "%") { + continue + } + key, err := url.QueryUnescape(field) + if err == nil && (key == "versionId" || key == "uploadId") { + return true + } + } + return false +} + +func hasInvalidPathSegment(values []string) bool { + for _, value := range values { + if value != "" && !s3_constants.IsValidPathSegment(value) { + return true + } + } + return false +} + // validateRequestPath rejects requests whose captured {bucket}/{object} mux // vars would normalize to a parent-directory traversal once joined into a // filer path. The router runs with mux.NewRouter().SkipClean(true), so @@ -33,6 +73,15 @@ func validateRequestPath(next http.Handler) http.Handler { return } } + // versionId and uploadId are later used as filer entry names. Avoid + // parsing every request's query while still recognizing encoded names. + if hasPathSegmentQuery(r.URL.RawQuery) { + query := r.URL.Query() + if hasInvalidPathSegment(query["versionId"]) || hasInvalidPathSegment(query["uploadId"]) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + return + } + } next.ServeHTTP(w, r) }) } diff --git a/weed/s3api/s3api_path_validation_test.go b/weed/s3api/s3api_path_validation_test.go index feb9ccb16b..9229c359c6 100644 --- a/weed/s3api/s3api_path_validation_test.go +++ b/weed/s3api/s3api_path_validation_test.go @@ -85,3 +85,57 @@ func TestValidateRequestPath_RejectsEmptyCapturedVars(t *testing.T) { }) } } + +func TestValidateRequestPath_RejectsUnsafePathQueryValues(t *testing.T) { + tests := []struct { + name string + rawQuery string + wantCode int + }{ + {"clean version ID", "versionId=opaque-version", http.StatusOK}, + {"clean upload ID", "uploadId=opaque_upload", http.StatusOK}, + {"empty values", "versionId=&uploadId=", http.StatusOK}, + {"version ID encoded slash", "versionId=v1%2F..%2Fsecret", http.StatusBadRequest}, + {"version ID backslash", "versionId=v1%5C..%5Csecret", http.StatusBadRequest}, + {"upload ID traversal", "uploadId=hash%2F..%2F..%2Fvictim", http.StatusBadRequest}, + {"unsafe repeated value", "versionId=clean&versionId=bad%2Fvalue", http.StatusBadRequest}, + {"encoded version ID name", "version%49d=v1%2F..%2Fsecret", http.StatusBadRequest}, + {"encoded upload ID name", "upload%49d=hash%2F..%2Fvictim", http.StatusBadRequest}, + {"unrelated query", "prefix=folder&delimiter=%2F", http.StatusOK}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + called := false + h := validateRequestPath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + })) + req := mux.SetURLVars( + httptest.NewRequest(http.MethodGet, "/bucket-a/key?"+tt.rawQuery, nil), + map[string]string{"bucket": "bucket-a", "object": "key"}, + ) + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + + if rr.Code != tt.wantCode { + t.Fatalf("query %q: got status %d, want %d", tt.rawQuery, rr.Code, tt.wantCode) + } + if tt.wantCode != http.StatusOK && called { + t.Fatalf("query %q: inner handler reached despite rejection", tt.rawQuery) + } + }) + } +} + +func TestHasPathSegmentQuery_CommonPathDoesNotAllocate(t *testing.T) { + allocations := testing.AllocsPerRun(1000, func() { + if hasPathSegmentQuery("prefix=folder&delimiter=%2F") { + t.Fatal("unrelated query recognized as a path segment query") + } + }) + if allocations != 0 { + t.Fatalf("common query path allocated %v times per run, want 0", allocations) + } +} diff --git a/weed/s3api/s3api_version_id.go b/weed/s3api/s3api_version_id.go index 94b359c904..124db3cf07 100644 --- a/weed/s3api/s3api_version_id.go +++ b/weed/s3api/s3api_version_id.go @@ -3,6 +3,7 @@ package s3api import ( "crypto/rand" "encoding/hex" + "errors" "fmt" "math" "strconv" @@ -24,6 +25,13 @@ const ( versionIdFormatThreshold = 0x4000000000000000 ) +var errInvalidVersionID = errors.New("invalid version ID") + +// Version IDs are opaque to clients but become filenames under .versions. +func isValidVersionID(versionId string) bool { + return versionId == "" || s3_constants.IsValidPathSegment(versionId) +} + // generateVersionId creates a unique version ID // If useInvertedFormat is true, uses inverted timestamps so newer versions sort first // If false, uses raw timestamps (old format) for backward compatibility diff --git a/weed/s3api/s3api_version_id_test.go b/weed/s3api/s3api_version_id_test.go index fd52fd7737..7057aab981 100644 --- a/weed/s3api/s3api_version_id_test.go +++ b/weed/s3api/s3api_version_id_test.go @@ -63,6 +63,27 @@ func TestVersionIdFormatDetection(t *testing.T) { } } +func TestIsValidVersionID(t *testing.T) { + tests := []struct { + versionID string + want bool + }{ + {"", true}, + {"null", true}, + {"opaque-version_123", true}, + {"v1/../../../victim", false}, + {`v1\..\victim`, false}, + {"bad\x00version", false}, + } + for _, tt := range tests { + t.Run(tt.versionID, func(t *testing.T) { + if got := isValidVersionID(tt.versionID); got != tt.want { + t.Errorf("isValidVersionID(%q) = %v, want %v", tt.versionID, got, tt.want) + } + }) + } +} + // TestGenerateVersionIdFormats tests that generateVersionId produces correct format based on parameter func TestGenerateVersionIdFormats(t *testing.T) { // Generate old format version ID diff --git a/weed/server/filer_jwt_test.go b/weed/server/filer_jwt_test.go index b66c085dda..69a96cb500 100644 --- a/weed/server/filer_jwt_test.go +++ b/weed/server/filer_jwt_test.go @@ -185,6 +185,38 @@ func TestFilerServer_maybeCheckJwtAuthorization_Scoped(t *testing.T) { isWrite: false, expectAuthorized: false, }, + { + name: "copy source within subtree allowed", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1/loot?cp.from=/tenant1/src", + isWrite: true, + expectAuthorized: true, + }, + { + name: "copy source outside subtree denied", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1/loot?cp.from=/tenant2/secret", + isWrite: true, + expectAuthorized: false, + }, + { + name: "copy source dot-dot escape denied", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1/loot?cp.from=/tenant1/../tenant2/secret", + isWrite: true, + expectAuthorized: false, + }, + { + name: "move source outside subtree denied", + token: genToken([]string{"/tenant1"}, nil), + method: "POST", + path: "/tenant1/dst?mv.from=/tenant2/secret", + isWrite: true, + expectAuthorized: false, + }, } for _, tt := range tests { diff --git a/weed/server/filer_server_handlers.go b/weed/server/filer_server_handlers.go index b5c22d676d..2fa9dee37f 100644 --- a/weed/server/filer_server_handlers.go +++ b/weed/server/filer_server_handlers.go @@ -253,17 +253,15 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool) } if len(claims.AllowedPrefixes) > 0 { - hasPrefix := false - for _, prefix := range claims.AllowedPrefixes { - if pathHasComponentPrefix(r.URL.Path, prefix) { - hasPrefix = true - break + // Copy and move name their source via a query parameter, not r.URL.Path. + // Scope every path the request reads or relocates, or a prefix-restricted + // token could reach data outside its allowed subtree. + for _, p := range jwtScopedRequestPaths(r) { + if !anyComponentPrefixMatches(claims.AllowedPrefixes, p) { + glog.V(1).Infof("jwt path not allowed from %s: %v", r.RemoteAddr, p) + return false } } - if !hasPrefix { - glog.V(1).Infof("jwt path not allowed from %s: %v", r.RemoteAddr, r.URL.Path) - return false - } } if len(claims.AllowedMethods) > 0 { hasMethod := false @@ -282,6 +280,32 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool) return true } +// jwtScopedRequestPaths returns every filer path a request touches that must be +// covered by the JWT AllowedPrefixes: the write target (r.URL.Path) plus any +// copy/move source named by the cp.from / mv.from query parameters. +func jwtScopedRequestPaths(r *http.Request) []string { + paths := []string{r.URL.Path} + if query := r.URL.Query(); query.Has("cp.from") || query.Has("mv.from") { + if from := query.Get("cp.from"); from != "" { + paths = append(paths, from) + } + if from := query.Get("mv.from"); from != "" { + paths = append(paths, from) + } + } + return paths +} + +// anyComponentPrefixMatches reports whether p is within any of the prefixes. +func anyComponentPrefixMatches(prefixes []string, p string) bool { + for _, prefix := range prefixes { + if pathHasComponentPrefix(p, prefix) { + return true + } + } + return false +} + // pathHasComponentPrefix reports whether reqPath is contained within the // directory subtree denoted by prefix, treating both as "/"-separated // path components. Both inputs are normalised with path.Clean to neutralise diff --git a/weed/server/filer_server_handlers_write.go b/weed/server/filer_server_handlers_write.go index 267968c46f..f44f530045 100644 --- a/weed/server/filer_server_handlers_write.go +++ b/weed/server/filer_server_handlers_write.go @@ -81,8 +81,18 @@ func (fs *FilerServer) PostHandler(w http.ResponseWriter, r *http.Request, conte ctx := r.Context() destination := r.RequestURI - if finalDestination := r.Header.Get(s3_constants.SeaweedStorageDestinationHeader); finalDestination != "" { - destination = finalDestination + headerDestination := r.Header.Get(s3_constants.SeaweedStorageDestinationHeader) + if headerDestination != "" { + destination = headerDestination + } + + // The destination header picks storage rules for a logical destination, but + // the entry is written at r.URL.Path. Enforce the read-only/quota rule on the + // actual write path too, so the header cannot route a write into a read-only + // location. + if headerDestination != "" && fs.filer.FilerConf.MatchStorageRule(r.URL.Path).ReadOnly { + writeJsonError(w, r, http.StatusInsufficientStorage, ErrReadOnly) + return } query := r.URL.Query() diff --git a/weed/server/wrapped_webdav_fs.go b/weed/server/wrapped_webdav_fs.go index 4ef8d4e68c..86de33e0f3 100644 --- a/weed/server/wrapped_webdav_fs.go +++ b/weed/server/wrapped_webdav_fs.go @@ -4,6 +4,7 @@ import ( "context" "io/fs" "os" + "path" "strings" "golang.org/x/net/webdav" @@ -14,6 +15,14 @@ type wrappedFs struct { webdav.FileSystem } +// confine joins a client-supplied name under subFolder. The name is cleaned as +// a rooted path first so `..` segments cannot climb above subFolder; cleaning +// after the concat (as the underlying FileSystem does) would resolve them across +// the confinement boundary. +func (w wrappedFs) confine(name string) string { + return w.subFolder + path.Clean("/"+name) +} + // NewWrappedFs returns a webdav.FileSystem identical to fs, except it // provides access to a sub-folder of fs that is denominated by subFolder. // It transparently handles renaming paths and filenames so that the outer part of the wrapped filesystem @@ -26,12 +35,12 @@ func NewWrappedFs(fs webdav.FileSystem, subFolder string) webdav.FileSystem { } func (w wrappedFs) Mkdir(ctx context.Context, name string, perm os.FileMode) error { - name = w.subFolder + name + name = w.confine(name) return w.FileSystem.Mkdir(ctx, name, perm) } func (w wrappedFs) OpenFile(ctx context.Context, name string, flag int, perm os.FileMode) (webdav.File, error) { - name = w.subFolder + name + name = w.confine(name) file, err := w.FileSystem.OpenFile(ctx, name, flag, perm) file = wrappedFile{ File: file, @@ -42,18 +51,18 @@ func (w wrappedFs) OpenFile(ctx context.Context, name string, flag int, perm os. } func (w wrappedFs) RemoveAll(ctx context.Context, name string) error { - name = w.subFolder + name + name = w.confine(name) return w.FileSystem.RemoveAll(ctx, name) } func (w wrappedFs) Rename(ctx context.Context, oldName, newName string) error { - oldName = w.subFolder + oldName - newName = w.subFolder + newName + oldName = w.confine(oldName) + newName = w.confine(newName) return w.FileSystem.Rename(ctx, oldName, newName) } func (w wrappedFs) Stat(ctx context.Context, name string) (os.FileInfo, error) { - name = w.subFolder + name + name = w.confine(name) info, err := w.FileSystem.Stat(ctx, name) info = wrappedFileInfo{ subFolder: &w.subFolder, diff --git a/weed/server/wrapped_webdav_fs_test.go b/weed/server/wrapped_webdav_fs_test.go new file mode 100644 index 0000000000..a401423baa --- /dev/null +++ b/weed/server/wrapped_webdav_fs_test.go @@ -0,0 +1,24 @@ +package weed_server + +import "testing" + +func TestWrappedFsConfine(t *testing.T) { + w := wrappedFs{subFolder: "/confined"} + tests := []struct { + name string + want string + }{ + {"/a/b", "/confined/a/b"}, + {"a/b", "/confined/a/b"}, + {"/", "/confined/"}, + {"/../etc/passwd", "/confined/etc/passwd"}, + {"/a/../../etc", "/confined/etc"}, + {"/a/./b", "/confined/a/b"}, + {"/a//b", "/confined/a/b"}, + } + for _, tt := range tests { + if got := w.confine(tt.name); got != tt.want { + t.Errorf("confine(%q) = %q, want %q (must stay under subFolder)", tt.name, got, tt.want) + } + } +}