From f9289f057030f6cd1aa54c50273a3e3428f96f21 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 28 Sep 2026 07:17:58 +0800 Subject: [PATCH] s3: do not promote ?prefix into the object for non-List actions (#11494) * s3: do not promote ?prefix into the object for non-List actions authRequestWithAuthType mapped an empty object to the prefix parameter for every action, so PUT /bucket?versioning&prefix=x authorized as Write:bucket/x. An object-scoped grant (Write:bucket/*) could then change bucket versioning, lifecycle, cors, and object-lock configuration, and the promoted object also made ResolveS3Action report s3:PutObject to attached IAM policies. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: treat GET ?uploads as a bucket listing for authorization Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: resolve the listing action through the bucket-level object resolveS3AuthTarget fed the promoted prefix to ResolveS3Action, so a bucket-level ?uploads request resolved as s3:GetObject on the prefix ARN in the admin explicit-deny check. Resolve both action and resource against the object the bucket listing actually scopes. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: resolve the listing action through the bucket-level object in AuthorizeAction Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: drop the unreachable object-level uploads case from the resolver test Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/auth_credentials.go | 14 +-- .../auth_credentials_native_floor_test.go | 51 +++++++++ weed/s3api/auth_security_test.go | 106 ++++++++++++++++++ weed/s3api/s3_iam_middleware.go | 2 +- 4 files changed, 165 insertions(+), 8 deletions(-) diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 197f457ba..afbf303ba 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1780,13 +1780,13 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac bucket, object := s3_constants.GetBucketAndObject(r) prefix := s3_constants.GetPrefix(r) - // For List operations, use prefix for permission checking if available - if action == s3_constants.ACTION_LIST && object == "" && prefix != "" { - // List operation with prefix - check permission for the prefix path - object = prefix - } else if (object == "/" || object == "") && prefix != "" { - // Using the aws cli with s3, and s3api, and with boto3, the object is often set to "/" or empty - // but the prefix is set to the actual object key for permission checking + // For bucket listings, use prefix for permission checking if available: + // the aws cli, s3api, and boto3 carry the key scope in ?prefix= rather + // than the URL path. Other actions must not promote it — a bucket-level + // request has no object, and a promoted prefix would let an object-scoped + // grant (e.g. Write:bucket/*) authorize bucket-subresource operations + // like ?versioning or ?lifecycle as if they were object writes. + if isBucketListingRequest(r, action) && (object == "" || object == "/") && prefix != "" { object = prefix } diff --git a/weed/s3api/auth_credentials_native_floor_test.go b/weed/s3api/auth_credentials_native_floor_test.go index 7a01df976..e5a392b0b 100644 --- a/weed/s3api/auth_credentials_native_floor_test.go +++ b/weed/s3api/auth_credentials_native_floor_test.go @@ -124,3 +124,54 @@ func TestAttachedPolicyExplicitDenyOverridesNativeAdmin(t *testing.T) { assert.Equal(t, s3err.ErrAccessDenied, errCode, "explicit Deny in an attached policy must override native Admin") } + +// GET ?uploads lists multipart uploads at bucket level but routes under Read, +// so the prefix promoted into object must not change the evaluated action or +// resource: an explicit Deny on s3:ListBucketMultipartUploads must still +// constrain a native admin, and an Allow on the bucket ARN must satisfy a +// plain attached-policy identity. +func TestMultipartListingResolvesBucketAction(t *testing.T) { + mgr := newTestIAMManager(t) + iam := &IdentityAccessManagement{} + iam.SetIAMIntegration(NewS3IAMIntegration(mgr, "")) + + denyDoc, _ := json.Marshal(map[string]interface{}{ + "Version": "2012-10-17", + "Statement": []map[string]interface{}{ + {"Effect": "Deny", "Action": "s3:ListBucketMultipartUploads", "Resource": "arn:aws:s3:::mybucket"}, + }, + }) + require.NoError(t, iam.PutPolicy("DenyUploadsListing", string(denyDoc))) + allowDoc, _ := json.Marshal(map[string]interface{}{ + "Version": "2012-10-17", + "Statement": []map[string]interface{}{ + {"Effect": "Allow", "Action": "s3:ListBucketMultipartUploads", "Resource": "arn:aws:s3:::mybucket"}, + }, + }) + require.NoError(t, iam.PutPolicy("AllowUploadsListing", string(allowDoc))) + + uploadsReq := func() *http.Request { + return httptest.NewRequest(http.MethodGet, "/mybucket?uploads&prefix=x/", nil) + } + admin := &Identity{ + Name: "admin", + Account: &Account{DisplayName: "admin", Id: "admin"}, + Actions: []Action{s3_constants.ACTION_ADMIN}, + PolicyNames: []string{"DenyUploadsListing"}, + PrincipalArn: "arn:aws:iam::111122223333:user/admin", + } + reader := &Identity{ + Name: "reader", + Account: &Account{DisplayName: "reader", Id: "reader"}, + PolicyNames: []string{"AllowUploadsListing"}, + PrincipalArn: "arn:aws:iam::111122223333:user/reader", + } + + // object carries the promoted prefix, matching authRequestWithAuthType + assert.Equal(t, s3err.ErrAccessDenied, + iam.VerifyActionPermission(uploadsReq(), admin, s3_constants.ACTION_READ, "mybucket", "x/"), + "explicit Deny on the uploads listing must override native Admin") + assert.Equal(t, s3err.ErrNone, + iam.VerifyActionPermission(uploadsReq(), reader, s3_constants.ACTION_READ, "mybucket", "x/"), + "s3:ListBucketMultipartUploads on the bucket must allow the uploads listing") +} diff --git a/weed/s3api/auth_security_test.go b/weed/s3api/auth_security_test.go index 24b3c183c..d6b1440ee 100644 --- a/weed/s3api/auth_security_test.go +++ b/weed/s3api/auth_security_test.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" + "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/stretchr/testify/assert" @@ -557,3 +558,108 @@ func TestRealSDKSignerWithForwardedHeaders(t *testing.T) { }) } } + +// GHSA-8rrx-349w-6396: ?prefix= was promoted into the object for every action, +// so PUT /cache?versioning&prefix=x authorized as Write:cache/x. An identity +// holding only Write:cache/* could then change bucket versioning, lifecycle, +// cors, and object-lock configuration. +func TestPrefixParameterDoesNotEscalateBucketActions(t *testing.T) { + resetMemoryStore() + defer resetMemoryStore() + + configContent := `{ + "identities": [ + {"name":"admin","credentials":[{"accessKey":"ADMINKEY","secretKey":"adminsecret0000000000000000000001"}],"actions":["Admin"]}, + {"name":"writer","credentials":[{"accessKey":"WRITERKEY","secretKey":"writersecret000000000000000000001"}],"actions":["Read:cache","Write:cache/*","List:cache"]}, + {"name":"uploadreader","credentials":[{"accessKey":"UPREADERKEY","secretKey":"upreadersecret00000000000000001"}],"actions":["Read:cache/uploads/*"]}, + {"name":"policywriter","credentials":[{"accessKey":"POLICYKEY","secretKey":"policysecret00000000000000000001"}],"policyNames":["WriterPolicy"]}, + {"name":"uploadlister","credentials":[{"accessKey":"UPLISTERKEY","secretKey":"uplistersecret00000000000000001"}],"policyNames":["UploadsPolicy"]}, + {"name":"getonly","credentials":[{"accessKey":"GETONLYKEY","secretKey":"getonlysecret0000000000000000001"}],"policyNames":["GetOnlyPolicy"]} + ], + "policies":[ + {"name":"WriterPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::cache/*\"]}]}"}, + {"name":"UploadsPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:ListBucketMultipartUploads\"],\"Resource\":[\"arn:aws:s3:::cache\"]}]}"}, + {"name":"GetOnlyPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"arn:aws:s3:::cache/*\"]}]}"} + ] +}` + tmpFile, err := os.CreateTemp("", "s3-config-*.json") + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.Write([]byte(configContent)) + require.NoError(t, err) + require.NoError(t, tmpFile.Close()) + + iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmpFile.Name()}, nil, "memory") + require.True(t, iam.isEnabled(), "Auth should be enabled") + + for _, tc := range []struct { + name string + access string + secret string + object string + query string + action Action + want s3err.ErrorCode + }{ + {"writer put object", "WRITERKEY", "writersecret000000000000000000001", "key", "", s3_constants.ACTION_WRITE, s3err.ErrNone}, + {"writer put versioning", "WRITERKEY", "writersecret000000000000000000001", "", "versioning", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied}, + {"writer put versioning with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "versioning&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied}, + {"writer put lifecycle with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "lifecycle&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied}, + {"writer list with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "list-type=2&prefix=x", s3_constants.ACTION_LIST, s3err.ErrNone}, + {"policywriter put object", "POLICYKEY", "policysecret00000000000000000001", "key", "", s3_constants.ACTION_WRITE, s3err.ErrNone}, + {"policywriter put versioning with prefix", "POLICYKEY", "policysecret00000000000000000001", "", "versioning&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied}, + {"uploadreader lists uploads under prefix", "UPREADERKEY", "upreadersecret00000000000000001", "", "uploads&prefix=uploads/foo", s3_constants.ACTION_READ, s3err.ErrNone}, + {"uploadreader cannot list outside prefix", "UPREADERKEY", "upreadersecret00000000000000001", "", "uploads&prefix=other/", s3_constants.ACTION_READ, s3err.ErrAccessDenied}, + {"uploadlister lists uploads", "UPLISTERKEY", "uplistersecret00000000000000001", "", "uploads&prefix=x", s3_constants.ACTION_READ, s3err.ErrNone}, + {"getonly cannot list uploads", "GETONLYKEY", "getonlysecret0000000000000000001", "", "uploads&prefix=x", s3_constants.ACTION_READ, s3err.ErrAccessDenied}, + } { + t.Run(tc.name, func(t *testing.T) { + url := "http://localhost:8333/cache" + if tc.object != "" { + url += "/" + tc.object + } + if tc.query != "" { + url += "?" + tc.query + } + r := httptest.NewRequest(http.MethodPut, url, nil) + if tc.action == s3_constants.ACTION_LIST || tc.action == s3_constants.ACTION_READ { + r.Method = http.MethodGet + } + r = mux.SetURLVars(r, map[string]string{"bucket": "cache", "object": tc.object}) + require.NoError(t, signRawHTTPRequest(context.Background(), r, tc.access, tc.secret, "us-east-1")) + + _, errCode := iam.authRequest(r, tc.action) + assert.Equal(t, tc.want, errCode) + }) + } +} + +// The admin explicit-deny path resolves the same action and resource the +// policy engine sees, so a promoted prefix must not hide a listing variant: +// ?uploads resolves s3:ListBucketMultipartUploads on the bucket ARN, and +// ?versions resolves s3:ListBucketVersions, both at bucket level. +func TestResolveS3AuthTarget_BucketListings(t *testing.T) { + for _, tt := range []struct { + name string + url string + action Action + object string + wantAction string + wantResource string + }{ + {"uploads listing keeps its action", "/cache?uploads&prefix=uploads/", s3_constants.ACTION_READ, "uploads/", + s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS, "arn:aws:s3:::cache"}, + {"versions listing keeps its action", "/cache?versions&prefix=a/", s3_constants.ACTION_LIST, "a/", + s3_constants.S3_ACTION_LIST_BUCKET_VERSIONS, "arn:aws:s3:::cache"}, + {"plain list keeps its action", "/cache?list-type=2&prefix=a/", s3_constants.ACTION_LIST, "a/", + s3_constants.S3_ACTION_LIST_BUCKET, "arn:aws:s3:::cache"}, + } { + t.Run(tt.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, tt.url, nil) + r = mux.SetURLVars(r, map[string]string{"bucket": "cache"}) + action, resource := resolveS3AuthTarget(tt.action, "cache", tt.object, r) + assert.Equal(t, tt.wantAction, action) + assert.Equal(t, tt.wantResource, resource) + }) + } +} diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index 10cefce81..86851b5b2 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -260,7 +260,7 @@ func (s3iam *S3IAMIntegration) AuthorizeAction(ctx context.Context, identity *IA // Determine the specific S3 action based on the HTTP request details. The // prefix promoted into objectKey is not part of the URL; resolve against - // the bucket-level object so ?versions keeps its own action. + // the bucket-level object so ?versions and ?uploads keep their own action. specificAction := ResolveS3Action(r, string(action), bucket, resourceObjectKey) // Create action request