Merge pull request #221 from thaJeztah/limit_uidgid

user: prevent falling back to looking up numeric usernames

Author: kolyshkin

user/user.go

@@ -259,6 +259,38 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath
 	return GetExecUser(userSpec, defaults, passwd, group)
 }
 
+// parseNumeric parses the given UID or GID value to an integer and within
+// the minID - maxID range.
+//
+// While the Linux kernel allows the max UID to be MaxUint32 - 2,
+// and the OCI Runtime Spec has no definition about the max UID, we require
+// the UID to be <= MaxInt32.
+//
+// See https://github.com/containerd/containerd/commit/de1341c201ffb0effebbf51d00376181968c8779
+func parseNumeric(val string) (int, bool, error) {
+	if val == "" {
+		return 0, false, nil
+	}
+	id, err := strconv.Atoi(val)
+	if err != nil {
+		if errors.Is(err, strconv.ErrSyntax) {
+			// Discard the error, because non-numeric values are expected
+			// when passing a username or group-name.
+			return 0, false, nil
+		}
+		if errors.Is(err, strconv.ErrRange) {
+			return 0, true, ErrRange
+		}
+		// Other errors ("invalid base", "invalid bit size"); should never
+		// happen with strconv.Atoi.
+		return 0, false, err
+	}
+	if id < minID || id > maxID {
+		return 0, true, ErrRange
+	}
+	return id, true, nil
+}
+
 // GetExecUser parses a user specification string (using the passwd and group
 // readers as sources for /etc/passwd and /etc/group data, respectively). In
 // the case of blank fields or missing data from the sources, the values in
@@ -304,8 +336,14 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 
 	// 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)
+	uidArg, isUID, err := parseNumeric(userArg)
+	if err != nil {
+		return nil, err
+	}
+	gidArg, isGID, err := parseNumeric(groupArg)
+	if err != nil {
+		return nil, err
+	}
 
 	// Find the matching user.
 	users, err := ParsePasswdFilter(passwd, func(u User) bool {
@@ -314,8 +352,8 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 			return u.Uid == user.Uid
 		}
 
-		if uidErr == nil {
-			// If the userArg is numeric, always treat it as a UID.
+		if isUID {
+			// If the userArg is a valid numeric value, always treat it as a UID.
 			return uidArg == u.Uid
 		}
 
@@ -341,18 +379,11 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 		// 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 {
+		if !isUID {
 			// Not numeric.
 			return nil, fmt.Errorf("unable to find user %s: %w", userArg, ErrNoPasswdEntries)
 		}
 		user.Uid = uidArg
-
-		// Must be inside valid uid range.
-		if user.Uid < minID || user.Uid > maxID {
-			return nil, ErrRange
-		}
-
-		// 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
@@ -370,7 +401,7 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 				return false
 			}
 
-			if gidErr == nil {
+			if isGID {
 				// If the groupArg is numeric, always treat it as a GID.
 				return gidArg == g.Gid
 			}
@@ -390,18 +421,11 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 				// 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 {
+				if !isGID {
 					// Not numeric.
 					return nil, fmt.Errorf("unable to find group %s: %w", groupArg, ErrNoGroupEntries)
 				}
 				user.Gid = gidArg
-
-				// Must be inside valid gid range.
-				if user.Gid < minID || user.Gid > maxID {
-					return nil, ErrRange
-				}
-
-				// 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.
@@ -415,18 +439,47 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 	return user, nil
 }
 
+// groupArg is a parsed group argument for [GetAdditionalGroups].
+type groupArg struct {
+	name      string
+	gid       int
+	isNumeric bool
+}
+
+// matches reports whether group g satisfies the argument. Numeric arguments
+// are matched by GID only, others by name.
+func (ag groupArg) matches(g Group) bool {
+	if ag.isNumeric {
+		return g.Gid == ag.gid
+	}
+	return g.Name == ag.name
+}
+
 // GetAdditionalGroups looks up a list of groups by name or group id
 // against the given /etc/group formatted data. If a group name cannot
 // be found, an error will be returned. If a group id cannot be found,
 // or the given group data is nil, the id will be returned as-is
 // provided it is in the legal range.
 func GetAdditionalGroups(additionalGroups []string, group io.Reader) ([]int, error) {
+	addtlGroups := make([]groupArg, len(additionalGroups))
+	for i, ag := range additionalGroups {
+		gid, ok, err := parseNumeric(ag)
+		if err != nil {
+			return nil, err
+		}
+		addtlGroups[i] = groupArg{
+			name:      ag,
+			gid:       gid,
+			isNumeric: ok,
+		}
+	}
+
 	groups := []Group{}
 	if group != nil {
 		var err error
 		groups, err = ParseGroupFilter(group, func(g Group) bool {
-			for _, ag := range additionalGroups {
-				if g.Name == ag || strconv.Itoa(g.Gid) == ag {
+			for _, ag := range addtlGroups {
+				if ag.matches(g) {
 					return true
 				}
 			}
@@ -438,32 +491,28 @@ func GetAdditionalGroups(additionalGroups []string, group io.Reader) ([]int, err
 	}
 
 	gidMap := make(map[int]struct{})
-	for _, ag := range additionalGroups {
+	for _, ag := range addtlGroups {
 		var found bool
 		for _, g := range groups {
-			// if we found a matched group either by name or gid, take the
-			// first matched as correct
-			if g.Name == ag || strconv.Itoa(g.Gid) == ag {
-				if _, ok := gidMap[g.Gid]; !ok {
-					gidMap[g.Gid] = struct{}{}
-					found = true
-					break
+			if ag.matches(g) {
+				// take the first matched group as correct
+				if g.Gid < minID || g.Gid > maxID {
+					return nil, ErrRange
 				}
+				gidMap[g.Gid] = struct{}{}
+				found = true
+				break
 			}
 		}
-		// we asked for a group but didn't find it. let's check to see
-		// if we wanted a numeric group
+		// We asked for a group but didn't find it. Numeric group IDs may be
+		// used as-is even when they are not present in /etc/group; non-numeric
+		// group names must be found.
 		if !found {
-			gid, err := strconv.ParseInt(ag, 10, 64)
-			if err != nil {
+			if !ag.isNumeric {
 				// Not a numeric ID either.
-				return nil, fmt.Errorf("unable to find group %s: %w", ag, ErrNoGroupEntries)
-			}
-			// Ensure gid is inside gid range.
-			if gid < minID || gid > maxID {
-				return nil, ErrRange
+				return nil, fmt.Errorf("unable to find group %s: %w", ag.name, ErrNoGroupEntries)
 			}
-			gidMap[int(gid)] = struct{}{}
+			gidMap[ag.gid] = struct{}{}
 		}
 	}
 	gids := []int{}

user/user_test.go

@@ -106,6 +106,7 @@ adm:x:42:43:adm:/var/adm:/bin/false
 odd:x:111:112::/home/odd:::::
 2147483647:x:0:0:maxint32:/root:/bin/bash
 2147483648:x:0:0:toolarge:/root:/bin/bash
+9223372036854775807:x:0:0:maxint64:/root:/bin/bash
 user7456:x:7456:100:Vasya:/home/user7456
 this is just some garbage data
 `
@@ -117,6 +118,7 @@ grp:x:1234:root,adm,user7456
 odd:x:444:
 2147483647:x:1235:
 2147483648:x:1236:
+9223372036854775807:x:1237:
 this is just some garbage data
 ` + largeGroup()
 
@@ -293,6 +295,8 @@ adm:x:42:43:adm:/var/adm:/bin/false
 -42:x:12:13:broken:/very/broken
 2147483647:x:0:0:maxint32:/root:/bin/bash
 2147483648:x:0:0:toolarge:/root:/bin/bash
+9223372036854775807:x:0:0:maxint64:/root:/bin/bash
+9223372036854775808:x:0:0:maxint64plusone:/root:/bin/bash
 this is just some garbage data
 `
 	const groupContent = `
@@ -301,6 +305,8 @@ adm:x:43:
 grp:x:1234:root,adm
 2147483647:x:1235:
 2147483648:x:1236:
+9223372036854775807:x:1237:
+9223372036854775808:x:1238:
 this is just some garbage data
 `
 
@@ -319,9 +325,14 @@ this is just some garbage data
 		"-5:-2",
 		"-42",
 		"-43",
-		"42:2147483648", // maxID + 1
-		"2147483648:43", // maxID + 1
-		"2147483648",    // maxID + 1
+		"42:2147483648",            // maxID + 1
+		"2147483648:43",            // maxID + 1
+		"2147483648",               // maxID + 1
+		"7456:9223372036854775807", // maxInt64
+		"9223372036854775807:43",   // maxInt64
+		"9223372036854775807",      // maxInt64
+		"9223372036854775808",      // maxInt64+1, must not resolve as username
+		"0:9223372036854775808",    // maxInt64+1, must not resolve as group name
 	}
 
 	for _, tc := range tests {
@@ -450,6 +461,9 @@ root:x:0:root
 adm:x:43:
 grp:x:1234:root,adm
 adm:x:4343:root,adm-duplicate
+2147483648:x:0:
+9223372036854775808:x:0:
+toolarge:x:2147483648:
 this is just some garbage data
 ` + largeGroup()
 	tests := []foo{
@@ -463,6 +477,11 @@ this is just some garbage data
 			groups:   []string{"adm"},
 			expected: []int{43},
 		},
+		{
+			// numeric group miss must continue checking remaining groups
+			groups:   []string{"10001", "adm"},
+			expected: []int{43, 10001},
+		},
 		{
 			// multiple groups
 			groups:   []string{"adm", "grp"},
@@ -506,6 +525,24 @@ this is just some garbage data
 			groups:   []string{"largegroup"},
 			expected: []int{1000},
 		},
+		{
+			// numeric group must not resolve as group name
+			groups:   []string{"2147483648"},
+			expected: nil,
+			hasError: true,
+		},
+		{
+			// numeric group must not resolve as group name
+			groups:   []string{"9223372036854775808"},
+			expected: nil,
+			hasError: true,
+		},
+		{
+			// group entry with out-of-range gid
+			groups:   []string{"toolarge"},
+			expected: nil,
+			hasError: true,
+		},
 	}
 
 	for _, tc := range tests {