From 0ae4915a9391ef4b3ec80f5fcdcf24ee6884e776 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Wed, 31 Jan 2024 10:56:45 -0800 Subject: [PATCH] fix: permission checks for editing access keys (#18928) With this change, only a user with `UpdateServiceAccountAdminAction` permission is able to edit access keys. We would like to let a user edit their own access keys, however the feature needs to be re-designed for better security and integration with external systems like AD/LDAP and OpenID. This change prevents privilege escalation via service accounts. --- cmd/admin-handlers-users.go | 21 ++++--- cmd/admin-handlers-users_test.go | 98 +++++++++++++++++++++++++++++--- cmd/auth-handler.go | 3 +- cmd/iam.go | 45 +++++---------- cmd/signature-v4-utils.go | 4 +- cmd/sts-handlers_test.go | 32 ----------- 6 files changed, 120 insertions(+), 83 deletions(-) diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 1d9f2a5b1..228cf5b5d 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -774,6 +774,16 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re return } + // Permission checks: + // + // 1. Any type of account (i.e. access keys (previously/still called service + // accounts), STS accounts, internal IDP accounts, etc) with the + // policy.UpdateServiceAccountAdminAction permission can update any service + // account. + // + // 2. We would like to let a user update their own access keys, however it + // is currently blocked pending a re-design. Users are still able to delete + // and re-create them. if !globalIAMSys.IsAllowed(policy.Args{ AccountName: cred.AccessKey, Groups: cred.Groups, @@ -782,15 +792,8 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re IsOwner: owner, Claims: cred.Claims, }) { - requestUser := cred.AccessKey - if cred.ParentUser != "" { - requestUser = cred.ParentUser - } - - if requestUser != svcAccount.ParentUser { - writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) - return - } + writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL) + return } password := cred.SecretKey diff --git a/cmd/admin-handlers-users_test.go b/cmd/admin-handlers-users_test.go index 8fd52f816..ba4bff2dd 100644 --- a/cmd/admin-handlers-users_test.go +++ b/cmd/admin-handlers-users_test.go @@ -206,6 +206,7 @@ func TestIAMInternalIDPServerSuite(t *testing.T) { suite.TestCannedPolicies(c) suite.TestGroupAddRemove(c) suite.TestServiceAccountOpsByAdmin(c) + suite.TestServiceAccountPrivilegeEscalationBug(c) suite.TestServiceAccountOpsByUser(c) suite.TestAddServiceAccountPerms(c) suite.TearDownSuite(c) @@ -896,14 +897,6 @@ func (s *TestSuiteIAM) TestServiceAccountOpsByUser(c *check) { // 3. Check S3 access c.assertSvcAccS3Access(ctx, s, cr, bucket) - // 4. Check that svc account can restrict the policy, and that the - // session policy can be updated. - c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, accessKey, bucket) - - // 4. Check that service account's secret key and account status can be - // updated. - c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, accessKey, bucket) - // 5. Check that service account can be deleted. c.assertSvcAccDeletion(ctx, s, userAdmClient, accessKey, bucket) @@ -979,6 +972,95 @@ func (s *TestSuiteIAM) TestServiceAccountOpsByAdmin(c *check) { c.assertSvcAccDeletion(ctx, s, s.adm, accessKey, bucket) } +func (s *TestSuiteIAM) TestServiceAccountPrivilegeEscalationBug(c *check) { + ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) + defer cancel() + + err := s.client.MakeBucket(ctx, "public", minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket creat error: %v", err) + } + + err = s.client.MakeBucket(ctx, "private", minio.MakeBucketOptions{}) + if err != nil { + c.Fatalf("bucket creat error: %v", err) + } + + pubPolicyBytes := []byte(`{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "s3:*" + ], + "Resource": [ + "arn:aws:s3:::public", + "arn:aws:s3:::public/*" + ] + } + ] +}`) + + fullS3PolicyBytes := []byte(`{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "s3:*" + ], + "Resource": [ + "arn:aws:s3:::*" + ] + } + ] +} +`) + + // Create a service account for the root user. + cr, err := s.adm.AddServiceAccount(ctx, madmin.AddServiceAccountReq{ + TargetUser: globalActiveCred.AccessKey, + Policy: pubPolicyBytes, + }) + if err != nil { + c.Fatalf("admin should be able to create service account for themselves %s", err) + } + + svcClient := s.getUserClient(c, cr.AccessKey, cr.SecretKey, "") + + // Check that the service account can access the public bucket. + buckets, err := svcClient.ListBuckets(ctx) + if err != nil { + c.Fatalf("err fetching buckets %s", err) + } + if len(buckets) != 1 || buckets[0].Name != "public" { + c.Fatalf("service account should only have access to public bucket") + } + + // Create an madmin client with the service account creds. + svcAdmClient, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{ + Creds: credentials.NewStaticV4(cr.AccessKey, cr.SecretKey, ""), + Secure: s.secure, + }) + if err != nil { + c.Fatalf("Err creating svcacct admin client: %v", err) + } + svcAdmClient.SetCustomTransport(s.TestSuiteCommon.client.Transport) + + // Attempt to update the policy on the service account. + err = svcAdmClient.UpdateServiceAccount(ctx, cr.AccessKey, + madmin.UpdateServiceAccountReq{ + NewPolicy: fullS3PolicyBytes, + }) + + if err == nil { + c.Fatalf("service account should not be able to update policy on itself") + } else if !strings.Contains(err.Error(), "Access Denied") { + c.Fatalf("unexpected error: %v", err) + } +} + func (s *TestSuiteIAM) SetUpAccMgmtPlugin(c *check) { ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout) defer cancel() diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 5793f30e3..9537970d7 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -162,7 +162,8 @@ func validateAdminSignature(ctx context.Context, r *http.Request, region string) s3Err := ErrAccessDenied if _, ok := r.Header[xhttp.AmzContentSha256]; ok && getRequestAuthType(r) == authTypeSigned { - // We only support admin credentials to access admin APIs. + + // Get credential information from the request. cred, owner, s3Err = getReqAccessKeyV4(r, region, serviceS3) if s3Err != ErrNone { return cred, owner, s3Err diff --git a/cmd/iam.go b/cmd/iam.go index 15782fc98..e9571de52 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -974,7 +974,7 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro m[iamPolicyClaimNameSA()] = inheritedPolicyType } - // Add all the necessary claims for the service accounts. + // Add all the necessary claims for the service account. for k, v := range opts.claims { _, ok := m[k] if !ok { @@ -1848,37 +1848,14 @@ func (sys *IAMSys) IsAllowedServiceAccount(args policy.Args, parentUser string) return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs) } - // Now check if we have a sessionPolicy. - spolicy, ok := args.Claims[sessionPolicyNameExtracted] - if !ok { - return false + // 3. If an inline session-policy is present, evaluate it. + hasSessionPolicy, isAllowedSP := isAllowedBySessionPolicy(args) + if hasSessionPolicy { + return isAllowedSP && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)) } - spolicyStr, ok := spolicy.(string) - if !ok { - // Sub policy if set, should be a string reject - // malformed/malicious requests. - return false - } - - // Check if policy is parseable. - subPolicy, err := policy.ParseConfig(bytes.NewReader([]byte(spolicyStr))) - if err != nil { - // Log any error in input session policy config. - logger.LogIf(GlobalContext, err) - return false - } - - // This can only happen if policy was set but with an empty JSON. - if subPolicy.Version == "" && len(subPolicy.Statements) == 0 { - return isOwnerDerived || combinedPolicy.IsAllowed(parentArgs) - } - - if subPolicy.Version == "" { - return false - } - - return subPolicy.IsAllowed(parentArgs) && (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)) + // Sub policy not set. Evaluate only the parent policies. + return (isOwnerDerived || combinedPolicy.IsAllowed(parentArgs)) } // IsAllowedSTS is meant for STS based temporary credentials, @@ -2000,8 +1977,14 @@ func isAllowedBySessionPolicy(args policy.Args) (hasSessionPolicy bool, isAllowe return } + // As the session policy exists, even if the parent is the root account, it + // must be restricted by it. So, we set `.IsOwner` to false here + // unconditionally. + sessionPolicyArgs := args + sessionPolicyArgs.IsOwner = false + // Sub policy is set and valid. - return hasSessionPolicy, subPolicy.IsAllowed(args) + return hasSessionPolicy, subPolicy.IsAllowed(sessionPolicyArgs) } // GetCombinedPolicy returns a combined policy combining all policies diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index 10cb3c5c8..07374858b 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -158,8 +158,8 @@ func checkKeyValid(r *http.Request, accessKey string) (auth.Credentials, bool, A // Check if the access key is part of users credentials. u, ok := globalIAMSys.GetUser(r.Context(), accessKey) if !ok { - // Credentials will be invalid but and disabled - // return a different error in such a scenario. + // Credentials could be valid but disabled - return a different + // error in such a scenario. if u.Credentials.Status == auth.AccountOff { return cred, false, ErrAccessKeyDisabled } diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index 004602b72..827511438 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -909,14 +909,6 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccounts(c *check) { // 3. Check S3 access c.assertSvcAccS3Access(ctx, s, cr, bucket) - // 4. Check that svc account can restrict the policy, and that the - // session policy can be updated. - c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - - // 4. Check that service account's secret key and account status can be - // updated. - c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - // 5. Check that service account can be deleted. c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket) @@ -1101,14 +1093,6 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithGroups(c *check) { // 3. Check S3 access c.assertSvcAccS3Access(ctx, s, cr, bucket) - // 4. Check that svc account can restrict the policy, and that the - // session policy can be updated. - c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - - // 4. Check that service account's secret key and account status can be - // updated. - c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - // 5. Check that service account can be deleted. c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket) @@ -1358,14 +1342,6 @@ func (s *TestSuiteIAM) TestOpenIDServiceAcc(c *check) { // 3. Check S3 access c.assertSvcAccS3Access(ctx, s, cr, bucket) - // 4. Check that svc account can restrict the policy, and that the - // session policy can be updated. - c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - - // 4. Check that service account's secret key and account status can be - // updated. - c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - // 5. Check that service account can be deleted. c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket) @@ -1658,14 +1634,6 @@ func (s *TestSuiteIAM) TestOpenIDServiceAccWithRolePolicy(c *check) { // 3. Check S3 access c.assertSvcAccS3Access(ctx, s, cr, bucket) - // 4. Check that svc account can restrict the policy, and that the - // session policy can be updated. - c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - - // 4. Check that service account's secret key and account status can be - // updated. - c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket) - // 5. Check that service account can be deleted. c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket) }