diff --git a/user/user.go b/user/user.go index e2788a1..32be4d8 100644 --- a/user/user.go +++ b/user/user.go @@ -270,6 +270,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 @@ -315,8 +347,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 { @@ -325,8 +363,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 } @@ -352,18 +390,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 @@ -381,7 +412,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 } @@ -401,18 +432,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. @@ -432,12 +456,35 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( // 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) { + type groupArg struct { + name string + gid int + isNumeric bool + } + + 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.isNumeric { + if g.Gid == ag.gid { + return true + } + } else if g.Name == ag.name { return true } } @@ -449,32 +496,34 @@ 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.isNumeric { + if g.Gid != ag.gid { + continue } + } else if g.Name != ag.name { + continue + } + + 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 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{} diff --git a/user/user_test.go b/user/user_test.go index c0c762d..6837e53 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -10,7 +10,7 @@ import ( "testing" ) -func TestUserParseLine(t *testing.T) { +func TestParseLine(t *testing.T) { var ( a, b string c []string @@ -58,7 +58,7 @@ func TestUserParseLine(t *testing.T) { } } -func TestUserParsePasswd(t *testing.T) { +func TestParsePasswdFilter(t *testing.T) { users, err := ParsePasswdFilter(strings.NewReader(` root:x:0:0:root:/root:/bin/bash adm:x:3:4:adm:/var/adm:/bin/false @@ -78,7 +78,7 @@ this is just some garbage data } } -func TestUserParseGroup(t *testing.T) { +func TestParseGroupFilter(t *testing.T) { groups, err := ParseGroupFilter(strings.NewReader(` root:x:0:root adm:x:4:root,adm,daemon @@ -98,12 +98,15 @@ this is just some garbage data } } -func TestValidGetExecUser(t *testing.T) { +func TestGetExecUser(t *testing.T) { const passwdContent = ` root:x:0:0:root user:/root:/bin/bash adm:x:42:43:adm:/var/adm:/bin/false 111:x:222:333::/var/garbage 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 ` @@ -113,6 +116,9 @@ adm:x:43: grp:x:1234:root,adm,user7456 444:x:555:111 odd:x:444: +2147483647:x:1235: +2147483648:x:1236: +9223372036854775807:x:1237: this is just some garbage data ` + largeGroup() @@ -229,40 +235,78 @@ this is just some garbage data Home: "/home/user7456", }, }, + { + ref: "7456:2147483647", + expected: ExecUser{ + Uid: 7456, + Gid: 2147483647, // maxID + Sgids: defaultExecUser.Sgids, + Home: "/home/user7456", + }, + }, + { + ref: "2147483647:43", + expected: ExecUser{ + Uid: 2147483647, // maxID + Gid: 43, + Sgids: defaultExecUser.Sgids, + Home: defaultExecUser.Home, + }, + }, + { + ref: "2147483647", + expected: ExecUser{ + Uid: 2147483647, // maxID + Gid: defaultExecUser.Gid, + Sgids: defaultExecUser.Sgids, + Home: defaultExecUser.Home, + }, + }, } - for _, test := range tests { - passwd := strings.NewReader(passwdContent) - group := strings.NewReader(groupContent) - - execUser, err := GetExecUser(test.ref, &defaultExecUser, passwd, group) - if err != nil { - t.Logf("got unexpected error when parsing '%s': %s", test.ref, err.Error()) - t.Fail() - continue - } - - if !reflect.DeepEqual(test.expected, *execUser) { - t.Logf("ref: %v", test.ref) - t.Logf("got: %#v", execUser) - t.Logf("expected: %#v", test.expected) - t.Fail() - continue + for _, tc := range tests { + name := tc.ref + if name == "" { + name = "" } + t.Run(name, func(t *testing.T) { + passwd := strings.NewReader(passwdContent) + group := strings.NewReader(groupContent) + + execUser, err := GetExecUser(tc.ref, &defaultExecUser, passwd, group) + if err != nil { + t.Fatalf("got unexpected error when parsing '%s': %s", tc.ref, err.Error()) + } + + if !reflect.DeepEqual(tc.expected, *execUser) { + t.Logf("ref: %v", tc.ref) + t.Logf("got: %#v", execUser) + t.Logf("expected: %#v", tc.expected) + t.Fail() + } + }) } } -func TestInvalidGetExecUser(t *testing.T) { +func TestGetExecUserInvalid(t *testing.T) { const passwdContent = ` root:x:0:0:root user:/root:/bin/bash 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 = ` root:x:0:root 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 ` @@ -281,18 +325,26 @@ this is just some garbage data "-5:-2", "-42", "-43", + "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 _, test := range tests { - passwd := strings.NewReader(passwdContent) - group := strings.NewReader(groupContent) + for _, tc := range tests { + t.Run(tc, func(t *testing.T) { + passwd := strings.NewReader(passwdContent) + group := strings.NewReader(groupContent) - execUser, err := GetExecUser(test, nil, passwd, group) - if err == nil { - t.Logf("got unexpected success when parsing '%s': %#v", test, execUser) - t.Fail() - continue - } + execUser, err := GetExecUser(tc, nil, passwd, group) + if err == nil { + t.Fatalf("got unexpected success when parsing '%s': %#v", tc, execUser) + } + }) } } @@ -367,30 +419,33 @@ this is just some garbage data }, } - for _, test := range tests { - var passwd, group io.Reader - - if test.passwd { - passwd = strings.NewReader(passwdContent) - } - - if test.group { - group = strings.NewReader(groupContent) - } - - execUser, err := GetExecUser(test.ref, &defaultExecUser, passwd, group) - if err != nil { - t.Logf("got unexpected error when parsing '%s': %s", test.ref, err.Error()) - t.Fail() - continue - } - - if !reflect.DeepEqual(test.expected, *execUser) { - t.Logf("got: %#v", execUser) - t.Logf("expected: %#v", test.expected) - t.Fail() - continue + for _, tc := range tests { + name := tc.ref + if name == "" { + name = "" } + t.Run(name, func(t *testing.T) { + var passwd, group io.Reader + + if tc.passwd { + passwd = strings.NewReader(passwdContent) + } + + if tc.group { + group = strings.NewReader(groupContent) + } + + execUser, err := GetExecUser(tc.ref, &defaultExecUser, passwd, group) + if err != nil { + t.Fatalf("got unexpected error when parsing '%s': %s", tc.ref, err.Error()) + } + + if !reflect.DeepEqual(tc.expected, *execUser) { + t.Logf("got: %#v", execUser) + t.Logf("expected: %#v", tc.expected) + t.Fail() + } + }) } } @@ -406,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{ @@ -419,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"}, @@ -462,24 +525,43 @@ 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 _, test := range tests { - group := strings.NewReader(groupContent) - - gids, err := GetAdditionalGroups(test.groups, group) - if test.hasError && err == nil { - t.Errorf("Parse(%#v) expects error but has none", test) - continue - } - if !test.hasError && err != nil { - t.Errorf("Parse(%#v) has error %v", test, err) - continue - } - sort.Ints(gids) - if !reflect.DeepEqual(gids, test.expected) { - t.Errorf("Gids(%v), expect %v from groups %v", gids, test.expected, test.groups) - } + for _, tc := range tests { + name := strings.Join(tc.groups, ",") + t.Run(name, func(t *testing.T) { + group := strings.NewReader(groupContent) + + gids, err := GetAdditionalGroups(tc.groups, group) + if tc.hasError && err == nil { + t.Fatalf("Parse(%#v) expects error but has none", tc) + } + if !tc.hasError && err != nil { + t.Fatalf("Parse(%#v) has error %v", tc, err) + } + sort.Ints(gids) + if !reflect.DeepEqual(gids, tc.expected) { + t.Errorf("Gids(%v), expect %v from groups %v", gids, tc.expected, tc.groups) + } + }) } } @@ -502,20 +584,21 @@ func TestGetAdditionalGroupsNumeric(t *testing.T) { }, } - for _, test := range tests { - gids, err := GetAdditionalGroups(test.groups, nil) - if test.hasError && err == nil { - t.Errorf("Parse(%#v) expects error but has none", test) - continue - } - if !test.hasError && err != nil { - t.Errorf("Parse(%#v) has error %v", test, err) - continue - } - sort.Ints(gids) - if !reflect.DeepEqual(gids, test.expected) { - t.Errorf("Gids(%v), expect %v from groups %v", gids, test.expected, test.groups) - } + for _, tc := range tests { + name := strings.Join(tc.groups, ",") + t.Run(name, func(t *testing.T) { + gids, err := GetAdditionalGroups(tc.groups, nil) + if tc.hasError && err == nil { + t.Fatalf("Parse(%#v) expects error but has none", tc) + } + if !tc.hasError && err != nil { + t.Fatalf("Parse(%#v) has error %v", tc, err) + } + sort.Ints(gids) + if !reflect.DeepEqual(gids, tc.expected) { + t.Errorf("Gids(%v), expect %v from groups %v", gids, tc.expected, tc.groups) + } + }) } } @@ -524,7 +607,7 @@ func largeGroup() (res string) { var b strings.Builder b.WriteString("largegroup:x:1000:user1") for i := 2; i <= 7500; i++ { - fmt.Fprintf(&b, ",user%d", i) + _, _ = fmt.Fprintf(&b, ",user%d", i) } return b.String() }