Skip to content

Commit 5a96cbb

Browse files
authored
Fix user privilege escalation bug (#13976)
The AddUser() API endpoint was accepting a policy field. This API is used to update a user's secret key and account status, and allows a regular user to update their own secret key. The policy update is also applied though does not appear to be used by any existing client-side functionality. This fix changes the accepted request body type and removes the ability to apply policy changes as that is possible via the policy set API. NOTE: Changing passwords can be disabled as a workaround for this issue by adding an explicit "Deny" rule to disable the API for users.
1 parent 4169774 commit 5a96cbb

7 files changed

+143
-31
lines changed

Diff for: cmd/admin-handlers-users.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -416,14 +416,14 @@ func (a adminAPIHandlers) AddUser(w http.ResponseWriter, r *http.Request) {
416416
return
417417
}
418418

419-
var uinfo madmin.UserInfo
420-
if err = json.Unmarshal(configBytes, &uinfo); err != nil {
419+
var ureq madmin.AddOrUpdateUserReq
420+
if err = json.Unmarshal(configBytes, &ureq); err != nil {
421421
logger.LogIf(ctx, err)
422422
writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), r.URL)
423423
return
424424
}
425425

426-
if err = globalIAMSys.CreateUser(ctx, accessKey, uinfo); err != nil {
426+
if err = globalIAMSys.CreateUser(ctx, accessKey, ureq); err != nil {
427427
writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
428428
return
429429
}

Diff for: cmd/admin-handlers-users_test.go

+130
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,15 @@
1818
package cmd
1919

2020
import (
21+
"bytes"
2122
"context"
23+
"crypto/sha256"
24+
"encoding/hex"
25+
"encoding/json"
2226
"fmt"
27+
"io/ioutil"
28+
"net/http"
29+
"net/url"
2330
"os"
2431
"strings"
2532
"testing"
@@ -29,7 +36,9 @@ import (
2936
minio "github.com/minio/minio-go/v7"
3037
"github.com/minio/minio-go/v7/pkg/credentials"
3138
cr "github.com/minio/minio-go/v7/pkg/credentials"
39+
"github.com/minio/minio-go/v7/pkg/s3utils"
3240
"github.com/minio/minio-go/v7/pkg/set"
41+
"github.com/minio/minio-go/v7/pkg/signer"
3342
"github.com/minio/minio/internal/auth"
3443
)
3544

@@ -177,6 +186,7 @@ func TestIAMInternalIDPServerSuite(t *testing.T) {
177186

178187
suite.SetUpSuite(c)
179188
suite.TestUserCreate(c)
189+
suite.TestUserPolicyEscalationBug(c)
180190
suite.TestPolicyCreate(c)
181191
suite.TestCannedPolicies(c)
182192
suite.TestGroupAddRemove(c)
@@ -222,6 +232,24 @@ func (s *TestSuiteIAM) TestUserCreate(c *check) {
222232
c.Fatalf("user could not create bucket: %v", err)
223233
}
224234

235+
// 3.10. Check that user's password can be updated.
236+
_, newSecretKey := mustGenerateCredentials(c)
237+
err = s.adm.SetUser(ctx, accessKey, newSecretKey, madmin.AccountEnabled)
238+
if err != nil {
239+
c.Fatalf("Unable to update user's secret key: %v", err)
240+
}
241+
// 3.10.1 Check that old password no longer works.
242+
err = client.MakeBucket(ctx, getRandomBucketName(), minio.MakeBucketOptions{})
243+
if err == nil {
244+
c.Fatalf("user was unexpectedly able to create bucket with bad password!")
245+
}
246+
// 3.10.2 Check that new password works.
247+
client = s.getUserClient(c, accessKey, newSecretKey, "")
248+
err = client.MakeBucket(ctx, getRandomBucketName(), minio.MakeBucketOptions{})
249+
if err != nil {
250+
c.Fatalf("user could not create bucket: %v", err)
251+
}
252+
225253
// 4. Check that user can be disabled and verify it.
226254
err = s.adm.SetUserStatus(ctx, accessKey, madmin.AccountDisabled)
227255
if err != nil {
@@ -260,6 +288,108 @@ func (s *TestSuiteIAM) TestUserCreate(c *check) {
260288
}
261289
}
262290

291+
func (s *TestSuiteIAM) TestUserPolicyEscalationBug(c *check) {
292+
ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
293+
defer cancel()
294+
295+
bucket := getRandomBucketName()
296+
err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{})
297+
if err != nil {
298+
c.Fatalf("bucket creat error: %v", err)
299+
}
300+
301+
// 2. Create a user, associate policy and verify access
302+
accessKey, secretKey := mustGenerateCredentials(c)
303+
err = s.adm.SetUser(ctx, accessKey, secretKey, madmin.AccountEnabled)
304+
if err != nil {
305+
c.Fatalf("Unable to set user: %v", err)
306+
}
307+
// 2.1 check that user does not have any access to the bucket
308+
uClient := s.getUserClient(c, accessKey, secretKey, "")
309+
c.mustNotListObjects(ctx, uClient, bucket)
310+
311+
// 2.2 create and associate policy to user
312+
policy := "mypolicy-test-user-update"
313+
policyBytes := []byte(fmt.Sprintf(`{
314+
"Version": "2012-10-17",
315+
"Statement": [
316+
{
317+
"Effect": "Allow",
318+
"Action": [
319+
"s3:PutObject",
320+
"s3:GetObject",
321+
"s3:ListBucket"
322+
],
323+
"Resource": [
324+
"arn:aws:s3:::%s/*"
325+
]
326+
}
327+
]
328+
}`, bucket))
329+
err = s.adm.AddCannedPolicy(ctx, policy, policyBytes)
330+
if err != nil {
331+
c.Fatalf("policy add error: %v", err)
332+
}
333+
err = s.adm.SetPolicy(ctx, policy, accessKey, false)
334+
if err != nil {
335+
c.Fatalf("Unable to set policy: %v", err)
336+
}
337+
// 2.3 check user has access to bucket
338+
c.mustListObjects(ctx, uClient, bucket)
339+
// 2.3 check that user cannot delete the bucket
340+
err = uClient.RemoveBucket(ctx, bucket)
341+
if err == nil || err.Error() != "Access Denied." {
342+
c.Fatalf("bucket was deleted unexpectedly or got unexpected err: %v", err)
343+
}
344+
345+
// 3. Craft a request to update the user's permissions
346+
ep := s.adm.GetEndpointURL()
347+
urlValue := url.Values{}
348+
urlValue.Add("accessKey", accessKey)
349+
u, err := url.Parse(fmt.Sprintf("%s://%s/minio/admin/v3/add-user?%s", ep.Scheme, ep.Host, s3utils.QueryEncode(urlValue)))
350+
if err != nil {
351+
c.Fatalf("unexpected url parse err: %v", err)
352+
}
353+
req, err := http.NewRequestWithContext(ctx, http.MethodPut, u.String(), nil)
354+
if err != nil {
355+
c.Fatalf("unexpected new request err: %v", err)
356+
}
357+
reqBodyArg := madmin.UserInfo{
358+
SecretKey: secretKey,
359+
PolicyName: "consoleAdmin",
360+
Status: madmin.AccountEnabled,
361+
}
362+
buf, err := json.Marshal(reqBodyArg)
363+
if err != nil {
364+
c.Fatalf("unexpected json encode err: %v", err)
365+
}
366+
buf, err = madmin.EncryptData(secretKey, buf)
367+
if err != nil {
368+
c.Fatalf("unexpected encryption err: %v", err)
369+
}
370+
371+
req.ContentLength = int64(len(buf))
372+
sum := sha256.Sum256(buf)
373+
req.Header.Set("X-Amz-Content-Sha256", hex.EncodeToString(sum[:]))
374+
req.Body = ioutil.NopCloser(bytes.NewReader(buf))
375+
req = signer.SignV4(*req, accessKey, secretKey, "", "")
376+
377+
// 3.1 Execute the request.
378+
resp, err := s.TestSuiteCommon.client.Do(req)
379+
if err != nil {
380+
c.Fatalf("unexpected request err: %v", err)
381+
}
382+
if resp.StatusCode != 200 {
383+
c.Fatalf("got unexpected response: %#v\n", resp)
384+
}
385+
386+
// 3.2 check that user cannot delete the bucket
387+
err = uClient.RemoveBucket(ctx, bucket)
388+
if err == nil || err.Error() != "Access Denied." {
389+
c.Fatalf("User was able to escalate privileges (Err=%v)!", err)
390+
}
391+
}
392+
263393
func (s *TestSuiteIAM) TestAddServiceAccountPerms(c *check) {
264394
ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
265395
defer cancel()

Diff for: cmd/iam-store.go

+3-21
Original file line numberDiff line numberDiff line change
@@ -1629,7 +1629,7 @@ func (store *IAMStoreSys) ListServiceAccounts(ctx context.Context, accessKey str
16291629
}
16301630

16311631
// AddUser - adds/updates long term user account to storage.
1632-
func (store *IAMStoreSys) AddUser(ctx context.Context, accessKey string, uinfo madmin.UserInfo) error {
1632+
func (store *IAMStoreSys) AddUser(ctx context.Context, accessKey string, ureq madmin.AddOrUpdateUserReq) error {
16331633
cache := store.lock()
16341634
defer store.unlock()
16351635

@@ -1642,9 +1642,9 @@ func (store *IAMStoreSys) AddUser(ctx context.Context, accessKey string, uinfo m
16421642

16431643
u := newUserIdentity(auth.Credentials{
16441644
AccessKey: accessKey,
1645-
SecretKey: uinfo.SecretKey,
1645+
SecretKey: ureq.SecretKey,
16461646
Status: func() string {
1647-
if uinfo.Status == madmin.AccountEnabled {
1647+
if ureq.Status == madmin.AccountEnabled {
16481648
return auth.AccountOn
16491649
}
16501650
return auth.AccountOff
@@ -1657,25 +1657,7 @@ func (store *IAMStoreSys) AddUser(ctx context.Context, accessKey string, uinfo m
16571657

16581658
cache.iamUsersMap[accessKey] = u.Credentials
16591659

1660-
// Set policy if specified.
1661-
if uinfo.PolicyName != "" {
1662-
policy := uinfo.PolicyName
1663-
// Handle policy mapping set/update
1664-
mp := newMappedPolicy(policy)
1665-
for _, p := range mp.toSlice() {
1666-
if _, found := cache.iamPolicyDocsMap[policy]; !found {
1667-
logger.LogIf(GlobalContext, fmt.Errorf("%w: (%s)", errNoSuchPolicy, p))
1668-
return errNoSuchPolicy
1669-
}
1670-
}
1671-
1672-
if err := store.saveMappedPolicy(ctx, accessKey, regUser, false, mp); err != nil {
1673-
return err
1674-
}
1675-
cache.iamUserPolicyMap[accessKey] = mp
1676-
}
16771660
return nil
1678-
16791661
}
16801662

16811663
// UpdateUserSecretKey - sets user secret key to storage.

Diff for: cmd/iam.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -954,7 +954,7 @@ func (sys *IAMSys) DeleteServiceAccount(ctx context.Context, accessKey string, n
954954

955955
// CreateUser - create new user credentials and policy, if user already exists
956956
// they shall be rewritten with new inputs.
957-
func (sys *IAMSys) CreateUser(ctx context.Context, accessKey string, uinfo madmin.UserInfo) error {
957+
func (sys *IAMSys) CreateUser(ctx context.Context, accessKey string, ureq madmin.AddOrUpdateUserReq) error {
958958
if !sys.Initialized() {
959959
return errServerNotInitialized
960960
}
@@ -967,11 +967,11 @@ func (sys *IAMSys) CreateUser(ctx context.Context, accessKey string, uinfo madmi
967967
return auth.ErrInvalidAccessKeyLength
968968
}
969969

970-
if !auth.IsSecretKeyValid(uinfo.SecretKey) {
970+
if !auth.IsSecretKeyValid(ureq.SecretKey) {
971971
return auth.ErrInvalidSecretKeyLength
972972
}
973973

974-
err := sys.store.AddUser(ctx, accessKey, uinfo)
974+
err := sys.store.AddUser(ctx, accessKey, ureq)
975975
if err != nil {
976976
return err
977977
}

Diff for: cmd/signature-v4-utils_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func TestCheckValid(t *testing.T) {
7676
t.Fatalf("unable create credential, %s", err)
7777
}
7878

79-
globalIAMSys.CreateUser(ctx, ucreds.AccessKey, madmin.UserInfo{
79+
globalIAMSys.CreateUser(ctx, ucreds.AccessKey, madmin.AddOrUpdateUserReq{
8080
SecretKey: ucreds.SecretKey,
8181
Status: madmin.AccountEnabled,
8282
})

Diff for: go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ require (
4949
github.com/minio/csvparser v1.0.0
5050
github.com/minio/highwayhash v1.0.2
5151
github.com/minio/kes v0.14.0
52-
github.com/minio/madmin-go v1.1.18
52+
github.com/minio/madmin-go v1.1.20
5353
github.com/minio/minio-go/v7 v7.0.17
5454
github.com/minio/parquet-go v1.1.0
5555
github.com/minio/pkg v1.1.9

Diff for: go.sum

+2-2
Original file line numberDiff line numberDiff line change
@@ -1088,8 +1088,8 @@ github.com/minio/kes v0.14.0/go.mod h1:OUensXz2BpgMfiogslKxv7Anyx/wj+6bFC6qA7BQc
10881088
github.com/minio/madmin-go v1.0.12/go.mod h1:BK+z4XRx7Y1v8SFWXsuLNqQqnq5BO/axJ8IDJfgyvfs=
10891089
github.com/minio/madmin-go v1.1.15/go.mod h1:Iu0OnrMWNBYx1lqJTW+BFjBMx0Hi0wjw8VmqhiOs2Jo=
10901090
github.com/minio/madmin-go v1.1.17/go.mod h1:Iu0OnrMWNBYx1lqJTW+BFjBMx0Hi0wjw8VmqhiOs2Jo=
1091-
github.com/minio/madmin-go v1.1.18 h1:1jXRb9LTiXqbZWBjXYmlqI5eCWsyZlprrI5CEVQjjqY=
1092-
github.com/minio/madmin-go v1.1.18/go.mod h1:Iu0OnrMWNBYx1lqJTW+BFjBMx0Hi0wjw8VmqhiOs2Jo=
1091+
github.com/minio/madmin-go v1.1.20 h1:jig4gJi0CD+FYz+Cnd+TNo0oqhNaZcLmfUqNl5b46Eo=
1092+
github.com/minio/madmin-go v1.1.20/go.mod h1:Iu0OnrMWNBYx1lqJTW+BFjBMx0Hi0wjw8VmqhiOs2Jo=
10931093
github.com/minio/mc v0.0.0-20211207230606-23a05f5a17f2 h1:xocb1RGyrDJ8PxkNn0NSbaBlfdU6J/Ag9QK62pb7nR8=
10941094
github.com/minio/mc v0.0.0-20211207230606-23a05f5a17f2/go.mod h1:siI9jWTzj1KsNXgz6NOL/S7OTaAUM0OMi+zEkF08gnA=
10951095
github.com/minio/md5-simd v1.1.0/go.mod h1:XpBqgZULrMYD3R+M28PcmP0CkI7PEMzB3U77ZrKZ0Gw=

0 commit comments

Comments
 (0)