CVE-2016-3697: Permissions, Privileges, and Access Controls (CWE-264)
value: libcontainer/user/user.go in runC before 0.1.0, as used in Docker before 1.11.2, improperly treats a numeric UID as a potential username, which allows local users to gain privileges via a numeric username in the password file in a container.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
CVSS v3 base score: 7.8
Published: 2016-06-01T20:59Z
Repository: https://github.com/opencontainers/runc
Language: Go
Files changed: libcontainer/user/user.go
Commit date: 2016-03-31 07:33:31 +1100
Commit message: libcontainer: user: always treat numeric ids numerically

Most shadow-related tools don't treat numeric ids as potential
usernames, so change our behaviour to match that. Previously, using an
explicit specification like 111:222 could result in the UID and GID not
being 111 and 222 respectively (which is confusing).

Signed-off-by: Aleksa Sarai <asarai@suse.de>

Diff: diff --git a/libcontainer/user/user.go b/libcontainer/user/user.go
index e6375ea..9f4c2cb 100644
--- a/libcontainer/user/user.go
+++ b/libcontainer/user/user.go
@@ -233,14 +233,18 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath
 //     * "uid"
 //     * "user:group"
 //     * "uid:gid
 //     * "user:gid"
 //     * "uid:group"
+//
+// It should be noted that if you specify a numeric user or group id, they will
+// not be evaluated as usernames (only the metadata will be filled). So attempting
+// to parse a user with user.Name = "1337" will produce the user with a UID of
+// 1337.
 func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (*ExecUser, error) {
 	var (
 		userArg, groupArg string
-		name              string
 	)
 
 	if defaults == nil {
 		defaults = new(ExecUser)
 	}
@@ -259,88 +263,109 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 	}
 
 	// allow for userArg to have either "user" syntax, or optionally "user:group" syntax
 	parseLine(userSpec, &userArg, &groupArg)
 
+	// Convert userArg and groupArg to be numeric, so we don't have to execute
+	// Atoi *twice* for each iteration over lines.
+	uidArg, uidErr := strconv.Atoi(userArg)
+	gidArg, gidErr := strconv.Atoi(groupArg)
+
 	users, err := ParsePasswdFilter(passwd, func(u User) bool {
 		if userArg == "" {
 			return u.Uid == user.Uid
 		}
-		return u.Name == userArg || strconv.Itoa(u.Uid) == userArg
+
+		if uidErr == nil {
+			// If the userArg is numeric, always treat it as a UID.
+			return uidArg == u.Uid
+		}
+
+		return u.Name == userArg
 	})
 	if err != nil && passwd != nil {
 		if userArg == "" {
 			userArg = strconv.Itoa(user.Uid)
 		}
 		return nil, fmt.Errorf("Unable to find user %v: %v", userArg, err)
 	}
 
-	haveUser := users != nil && len(users) > 0
-	if haveUser {
-		// if we found any user entries that matched our filter, let's take the first one as "correct"
-		name = users[0].Name
+	var matchedUserName string
+	if len(users) > 0 {
+		// First match wins, even if there's more than one matching entry.
+		matchedUserName = users[0].Name
 		user.Uid = users[0].Uid
 		user.Gid = users[0].Gid
 		user.Home = users[0].Home
 	} else if userArg != "" {
-		// we asked for a user but didn't find them...  let's check to see if we wanted a numeric user
-		user.Uid, err = strconv.Atoi(userArg)
-		if err != nil {
-			// not numeric - we have to bail
-			return nil, fmt.Errorf("Unable to find user %v", userArg)
+		// If we can't find a user with the given username, the only other valid
+		// option is if it's a numeric username with no associated entry in passwd.
+
+		if uidErr != nil {
+			// Not numeric.
+			return nil, fmt.Errorf("unable to find user %s: %v", userArg, ErrNoPasswdEntries)
 		}
+		user.Uid = uidArg
 
 		// Must be inside valid uid range.
 		if user.Uid < minId || user.Uid > maxId {
 			return nil, ErrRange
 		}
 
-		// if userArg couldn't be found in /etc/passwd but is numeric, just roll with it - this is legit
+		// Okay, so it's numeric. We can just roll with this.
 	}
 
-	if groupArg != "" || name != "" {
+	// On to the groups. If we matched a username, we need to do this because of
+	// the supplementary group IDs.
+	if groupArg != "" || matchedUserName != "" {
 		groups, err := ParseGroupFilter(group, func(g Group) bool {
-			// Explicit group format takes precedence.
-			if groupArg != "" {
-				return g.Name == groupArg || strconv.Itoa(g.Gid) == groupArg
+			// If the group argument isn't explicit, we'll just search for it.
+			if groupArg == "" {
+				// Check if user is a member of this group.
+				for _, u := range g.List {
+					if u == matchedUserName {
+						return true
+					}
+				}
+				return false
 			}
 
-			// Check if user is a member.
-			for _, u := range g.List {
-				if u == name {
-					return true
-				}
+			if gidErr == nil {
+				// If the groupArg is numeric, always treat it as a GID.
+				return gidArg == g.Gid
 			}
 
-			return false
+			return g.Name == groupArg
 		})
 		if err != nil && group != nil {
-			return nil, fmt.Errorf("Unable to find groups for user %v: %v", users[0].Name, err)
+			return nil, fmt.Errorf("unable to find groups for spec %v: %v", matchedUserName, err)
 		}
 
 		haveGroup := groups != nil && len(groups) > 0
 		if groupArg != "" {
 			if haveGroup {
 				// if we found any group entries that matched our filter, let's take the first one as "correct"
 				user.Gid = groups[0].Gid
-			} else {
-				// we asked for a group but didn't find id...  let's check to see if we wanted a numeric group
-				user.Gid, err = strconv.Atoi(groupArg)
-				if err != nil {
-					// not numeric - we have to bail
-					return nil, fmt.Errorf("Unable to find group %v", groupArg)
+			} else if groupArg != "" {
+				// If we can't find a group with the given name, the only other valid
+				// option is if it's a numeric group name with no associated entry in group.
+
+				if gidErr != nil {
+					// Not numeric.
+					return nil, fmt.Errorf("unable to find group %s: %v", groupArg, ErrNoGroupEntries)
 				}
+				user.Gid = gidArg
 
 				// Ensure gid is inside gid range.
 				if user.Gid < minId || user.Gid > maxId {
 					return nil, ErrRange
 				}
 
-				// if groupArg couldn't be found in /etc/group but is numeric, just roll with it - this is legit
+				// Okay, so it's numeric. We can just roll with this.
 			}
-		} else if haveGroup {
-			// If implicit group format, fill supplementary gids.
+		} else if len(groups) > 0 {
+			// Supplementary group ids only make sense if in the implicit form.
 			user.Sgids = make([]int, len(groups))
 			for i, group := range groups {
 				user.Sgids[i] = group.Gid
 			}
 		}

Vulnerable code: 		name              string
		return u.Name == userArg || strconv.Itoa(u.Uid) == userArg
	haveUser := users != nil && len(users) > 0
	if haveUser {
		// if we found any user entries that matched our filter, let's take the first one as "correct"
		name = users[0].Name
		// we asked for a user but didn't find them...  let's check to see if we wanted a numeric user
		user.Uid, err = strconv.Atoi(userArg)
		if err != nil {
			// not numeric - we have to bail
			return nil, fmt.Errorf("Unable to find user %v", userArg)
		// if userArg couldn't be found in /etc/passwd but is numeric, just roll with it - this is legit
	if groupArg != "" || name != "" {
			// Explicit group format takes precedence.
			if groupArg != "" {
				return g.Name == groupArg || strconv.Itoa(g.Gid) == groupArg
			// Check if user is a member.
			for _, u := range g.List {
				if u == name {
					return true
				}
			return false
			return nil, fmt.Errorf("Unable to find groups for user %v: %v", users[0].Name, err)
			} else {
				// we asked for a group but didn't find id...  let's check to see if we wanted a numeric group
				user.Gid, err = strconv.Atoi(groupArg)
				if err != nil {
					// not numeric - we have to bail
					return nil, fmt.Errorf("Unable to find group %v", groupArg)
				// if groupArg couldn't be found in /etc/group but is numeric, just roll with it - this is legit
		} else if haveGroup {
			// If implicit group format, fill supplementary gids.
Fixed code: //
// It should be noted that if you specify a numeric user or group id, they will
// not be evaluated as usernames (only the metadata will be filled). So attempting
// to parse a user with user.Name = "1337" will produce the user with a UID of
// 1337.
	// Convert userArg and groupArg to be numeric, so we don't have to execute
	// Atoi *twice* for each iteration over lines.
	uidArg, uidErr := strconv.Atoi(userArg)
	gidArg, gidErr := strconv.Atoi(groupArg)


		if uidErr == nil {
			// If the userArg is numeric, always treat it as a UID.
			return uidArg == u.Uid
		}

		return u.Name == userArg
	var matchedUserName string
	if len(users) > 0 {
		// First match wins, even if there's more than one matching entry.
		matchedUserName = users[0].Name
		// If we can't find a user with the given username, the only other valid
		// option is if it's a numeric username with no associated entry in passwd.

		if uidErr != nil {
			// Not numeric.
			return nil, fmt.Errorf("unable to find user %s: %v", userArg, ErrNoPasswdEntries)
		user.Uid = uidArg
		// Okay, so it's numeric. We can just roll with this.
	// On to the groups. If we matched a username, we need to do this because of
	// the supplementary group IDs.
	if groupArg != "" || matchedUserName != "" {
			// If the group argument isn't explicit, we'll just search for it.
			if groupArg == "" {
				// Check if user is a member of this group.
				for _, u := range g.List {
					if u == matchedUserName {
						return true
					}
				}
				return false
			if gidErr == nil {
				// If the groupArg is numeric, always treat it as a GID.
				return gidArg == g.Gid
			return g.Name == groupArg
			return nil, fmt.Errorf("unable to find groups for spec %v: %v", matchedUserName, err)
			} else if groupArg != "" {
				// If we can't find a group with the given name, the only other valid
				// option is if it's a numeric group name with no associated entry in group.

				if gidErr != nil {
					// Not numeric.
					return nil, fmt.Errorf("unable to find group %s: %v", groupArg, ErrNoGroupEntries)
				user.Gid = gidArg
				// Okay, so it's numeric. We can just roll with this.
		} else if len(groups) > 0 {
			// Supplementary group ids only make sense if in the implicit form.