s3: reject a request that names two operations (#10987)

The router matches bucket subresource routes in registration order while
the IAM action resolver matches its own list in a different order, so a
request carrying two operation subresources is authorized as one
operation and served as another. `PUT /bucket?policy&tagging` resolves to
s3:PutBucketTagging and runs PutBucketPolicy, letting an identity
delegated bucket tagging install an arbitrary bucket policy. The same
mismatch reaches PutBucketCors, PutBucketLifecycle, PutBucketVersioning,
PutObjectLockConfiguration, PutBucketRequestPayment and the policy and
cors deletes.

Reject the ambiguity where the other pre-routing checks live, so neither
list has to stay in step with the other. Keys that modify an operation
rather than select one -- versionId, partNumber, prefix -- still combine
freely.

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

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