From 586614c73f902085465d07a1f854ca6c1efb9845 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Thu, 19 Dec 2019 14:21:21 -0800
Subject: [PATCH] fix: temp credentials shouldn't allow policy/group changes
 (#8675)

This PR fixes the issue where we might allow policy changes
for temporary credentials out of band, this situation allows
privilege escalation for those temporary credentials. We
should disallow any external actions on temporary creds
as a practice and we should clearly differentiate which
are static and which are temporary credentials.

Refer #8667
---
 cmd/admin-handlers-users.go | 22 ++++++++++++++
 cmd/iam.go                  | 60 ++++++++++++++++++++++++++++++-------
 pkg/auth/credentials.go     |  5 ++++
 3 files changed, 77 insertions(+), 10 deletions(-)

diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go
index d7e1fe12d..991a842b3 100644
--- a/cmd/admin-handlers-users.go
+++ b/cmd/admin-handlers-users.go
@@ -69,6 +69,16 @@ func (a adminAPIHandlers) RemoveUser(w http.ResponseWriter, r *http.Request) {
 	vars := mux.Vars(r)
 	accessKey := vars["accessKey"]
 
+	ok, err := globalIAMSys.IsTempUser(accessKey)
+	if err != nil {
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		return
+	}
+	if ok {
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errIAMActionNotAllowed), r.URL)
+		return
+	}
+
 	if err := globalIAMSys.DeleteUser(accessKey); err != nil {
 		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
@@ -518,6 +528,18 @@ func (a adminAPIHandlers) SetPolicyForUserOrGroup(w http.ResponseWriter, r *http
 		return
 	}
 
+	if !isGroup {
+		ok, err := globalIAMSys.IsTempUser(entityName)
+		if err != nil {
+			writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+			return
+		}
+		if ok {
+			writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, errIAMActionNotAllowed), r.URL)
+			return
+		}
+	}
+
 	if err := globalIAMSys.PolicyDBSet(entityName, policyName, isGroup); err != nil {
 		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
diff --git a/cmd/iam.go b/cmd/iam.go
index c54620324..ecbe6ca1e 100644
--- a/cmd/iam.go
+++ b/cmd/iam.go
@@ -600,20 +600,40 @@ func (sys *IAMSys) ListUsers() (map[string]madmin.UserInfo, error) {
 	}
 
 	for k, v := range sys.iamUsersMap {
-		users[k] = madmin.UserInfo{
-			PolicyName: sys.iamUserPolicyMap[k].Policy,
-			Status: func() madmin.AccountStatus {
-				if v.IsValid() {
-					return madmin.AccountEnabled
-				}
-				return madmin.AccountDisabled
-			}(),
+		if !v.IsTemp() {
+			users[k] = madmin.UserInfo{
+				PolicyName: sys.iamUserPolicyMap[k].Policy,
+				Status: func() madmin.AccountStatus {
+					if v.IsValid() {
+						return madmin.AccountEnabled
+					}
+					return madmin.AccountDisabled
+				}(),
+			}
 		}
 	}
 
 	return users, nil
 }
 
+// IsTempUser - returns if given key is a temporary user.
+func (sys *IAMSys) IsTempUser(name string) (bool, error) {
+	objectAPI := newObjectLayerWithoutSafeModeFn()
+	if objectAPI == nil {
+		return false, errServerNotInitialized
+	}
+
+	sys.RLock()
+	defer sys.RUnlock()
+
+	creds, found := sys.iamUsersMap[name]
+	if !found {
+		return false, errNoSuchUser
+	}
+
+	return creds.IsTemp(), nil
+}
+
 // GetUserInfo - get info on a user.
 func (sys *IAMSys) GetUserInfo(name string) (u madmin.UserInfo, err error) {
 	objectAPI := newObjectLayerWithoutSafeModeFn()
@@ -636,6 +656,10 @@ func (sys *IAMSys) GetUserInfo(name string) (u madmin.UserInfo, err error) {
 		return u, errNoSuchUser
 	}
 
+	if creds.IsTemp() {
+		return u, errIAMActionNotAllowed
+	}
+
 	u = madmin.UserInfo{
 		PolicyName: sys.iamUserPolicyMap[name].Policy,
 		Status: func() madmin.AccountStatus {
@@ -672,6 +696,10 @@ func (sys *IAMSys) SetUserStatus(accessKey string, status madmin.AccountStatus)
 		return errNoSuchUser
 	}
 
+	if cred.IsTemp() {
+		return errIAMActionNotAllowed
+	}
+
 	uinfo := newUserIdentity(auth.Credentials{
 		AccessKey: accessKey,
 		SecretKey: cred.SecretKey,
@@ -719,9 +747,15 @@ func (sys *IAMSys) SetUser(accessKey string, uinfo madmin.UserInfo) error {
 		return errServerNotInitialized
 	}
 
+	cr, ok := sys.iamUsersMap[accessKey]
+	if cr.IsTemp() && ok {
+		return errIAMActionNotAllowed
+	}
+
 	if err := sys.store.saveUserIdentity(accessKey, false, u); err != nil {
 		return err
 	}
+
 	sys.iamUsersMap[accessKey] = u.Credentials
 
 	// Set policy if specified.
@@ -794,10 +828,13 @@ func (sys *IAMSys) AddUsersToGroup(group string, members []string) error {
 
 	// Validate that all members exist.
 	for _, member := range members {
-		_, ok := sys.iamUsersMap[member]
+		cr, ok := sys.iamUsersMap[member]
 		if !ok {
 			return errNoSuchUser
 		}
+		if cr.IsTemp() {
+			return errIAMActionNotAllowed
+		}
 	}
 
 	gi, ok := sys.iamGroupsMap[group]
@@ -856,10 +893,13 @@ func (sys *IAMSys) RemoveUsersFromGroup(group string, members []string) error {
 
 	// Validate that all members exist.
 	for _, member := range members {
-		_, ok := sys.iamUsersMap[member]
+		cr, ok := sys.iamUsersMap[member]
 		if !ok {
 			return errNoSuchUser
 		}
+		if cr.IsTemp() {
+			return errIAMActionNotAllowed
+		}
 	}
 
 	gi, ok := sys.iamGroupsMap[group]
diff --git a/pkg/auth/credentials.go b/pkg/auth/credentials.go
index 06b2c8754..be9413dd9 100644
--- a/pkg/auth/credentials.go
+++ b/pkg/auth/credentials.go
@@ -117,6 +117,11 @@ func (cred Credentials) IsExpired() bool {
 	return cred.Expiration.Before(time.Now().UTC())
 }
 
+// IsTemp - returns whether credential is temporary or not.
+func (cred Credentials) IsTemp() bool {
+	return cred.SessionToken != ""
+}
+
 // IsValid - returns whether credential is valid or not.
 func (cred Credentials) IsValid() bool {
 	// Verify credentials if its enabled or not set.