diff --git a/weed/s3api/s3api_ambiguous_subresource_test.go b/weed/s3api/s3api_ambiguous_subresource_test.go new file mode 100644 index 000000000..e74153471 --- /dev/null +++ b/weed/s3api/s3api_ambiguous_subresource_test.go @@ -0,0 +1,75 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAmbiguousSubresource pins the rule that a request may name only one +// operation. The router picks a handler by registration order and the IAM action +// resolver picks an action by its own order, so a request carrying two operation +// subresources gets authorized as one and served as the other. +func TestAmbiguousSubresource(t *testing.T) { + for _, query := range []string{ + "", + "policy=", + "tagging=", + "acl=&versionId=abc", + "tagging=&versionId=abc", + "retention=&versionId=abc", + "uploadId=xyz&partNumber=3", + "attributes=&partNumber=3&versionId=abc", + "versions=&prefix=a&delimiter=/", + "uploads=&prefix=a&x-id=CreateMultipartUpload", + "list-type=2&prefix=a&continuation-token=x", + "acl=&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=deadbeef", + } { + req, _ := http.NewRequest("GET", "http://localhost/bucket/key?"+query, nil) + assert.False(t, hasAmbiguousSubresource(req.URL.Query()), "%q names one operation", query) + } + + for _, query := range []string{ + "policy=&tagging=", + "tagging=&policy=", + "cors=&tagging=", + "lifecycle=&tagging=", + "versioning=&tagging=", + "object-lock=&tagging=", + "requestPayment=&tagging=", + "acl=&policy=", + "policy=&cors=", + "delete=&policy=", + "uploads=&uploadId=xyz", + "policy=&tagging=&cors=", + } { + req, _ := http.NewRequest("PUT", "http://localhost/bucket?"+query, nil) + assert.True(t, hasAmbiguousSubresource(req.URL.Query()), "%q names two operations", query) + } +} + +// The bucket tagger's escalation: PUT /bucket?policy&tagging routes to the +// bucket-policy handler while resolving as s3:PutBucketTagging. The guard has to +// reject it before either the handler or the IAM check runs. +func TestAmbiguousSubresourceRejectedBeforeHandler(t *testing.T) { + served := false + handler := validateRequestPath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + served = true + })) + + req, _ := http.NewRequest("PUT", "http://localhost/bucket?policy=&tagging=", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.False(t, served, "an ambiguous request must not reach a handler") + assert.Equal(t, http.StatusBadRequest, rec.Code) + + served = false + req, _ = http.NewRequest("PUT", "http://localhost/bucket?policy=", nil) + rec = httptest.NewRecorder() + handler.ServeHTTP(rec, req) + assert.True(t, served, "an unambiguous request must still be served") +} diff --git a/weed/s3api/s3api_path_validation.go b/weed/s3api/s3api_path_validation.go index e95140291..692115b8f 100644 --- a/weed/s3api/s3api_path_validation.go +++ b/weed/s3api/s3api_path_validation.go @@ -39,6 +39,37 @@ func hasPathSegmentQuery(rawQuery string) bool { return false } +// operationSubresources are the query keys that select which operation a request +// is, so at most one may appear. The router matches them in registration order +// and the IAM action resolver in its own, so a request carrying two is +// authorized as one operation and served as another: `PUT /bucket?policy&tagging` +// authorizes as PutBucketTagging and runs PutBucketPolicy. Keys left out here +// (versionId, partNumber, prefix, ...) modify an operation instead of selecting +// one and may accompany any of these. +var operationSubresources = map[string]bool{ + "accelerate": true, "acl": true, "analytics": true, "attributes": true, + "cors": true, "delete": true, "encryption": true, "intelligent-tiering": true, + "inventory": true, "legal-hold": true, "lifecycle": true, "location": true, + "logging": true, "metrics": true, "notification": true, "object-lock": true, + "ownershipControls": true, "policy": true, "policyStatus": true, + "publicAccessBlock": true, "renameObject": true, "replication": true, + "requestPayment": true, "retention": true, "tagging": true, "uploadId": true, + "uploads": true, "versioning": true, "versions": true, "website": true, +} + +func hasAmbiguousSubresource(query url.Values) bool { + seen := 0 + for key := range query { + if !operationSubresources[key] { + continue + } + if seen++; seen > 1 { + return true + } + } + return false +} + func hasInvalidPathSegment(values []string) bool { for _, value := range values { if value != "" && !s3_constants.IsValidPathSegment(value) { @@ -73,14 +104,20 @@ 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) { + if r.URL.RawQuery != "" { query := r.URL.Query() - if hasInvalidPathSegment(query["versionId"]) || hasInvalidPathSegment(query["uploadId"]) { + if hasAmbiguousSubresource(query) { s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) return } + // versionId and uploadId are later used as filer entry names, and + // the encoded spelling of either still names one. + if hasPathSegmentQuery(r.URL.RawQuery) { + if hasInvalidPathSegment(query["versionId"]) || hasInvalidPathSegment(query["uploadId"]) { + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest) + return + } + } } next.ServeHTTP(w, r) })