diff --git a/user/user.go b/user/user.go index 48c34a8..3d77c54 100644 --- a/user/user.go +++ b/user/user.go @@ -57,44 +57,67 @@ type IDMap struct { Count int64 } -func parseLine(line []byte, v ...any) { - parseParts(bytes.Split(line, []byte(":")), v...) +// parseLine parses a colon-separated record into the provided fields. +// See [parseParts] for details. +func parseLine(line []byte, v ...any) bool { + return parseParts(bytes.Split(line, []byte(":")), v...) } -func parseParts(parts [][]byte, v ...any) { - if len(parts) == 0 { - return - } - - for i, p := range parts { - // Ignore cases where we don't have enough fields to populate the arguments. - // Some configuration files like to misbehave. - if len(v) <= i { - break +// parseParts parses the provided parts into the corresponding fields, +// and reports whether the record is valid. +// +// Missing fields are treated as empty. Empty string fields are accepted, +// but missing, empty, non-numeric, or out-of-range integer fields make +// the record invalid. For *int fields (UIDs and GIDs), the valid range +// is the one checked by [validateID]. +// +// This follows the behavior of glibc's passwd and group parsers, which +// skip malformed records when reading entries. +// +// See https://sources.debian.org/src/glibc/2.42-13/nss/nss_fgetent_r.c/ +// and https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c/#L150-L165 +func parseParts(parts [][]byte, fields ...any) bool { + for i, v := range fields { + var p []byte + if i < len(parts) { + p = parts[i] } // Use the type of the argument to figure out how to parse it, scanf() style. // This is legit. - switch e := v[i].(type) { + switch e := v.(type) { case *string: *e = string(p) case *int: - // "numbers", with conversion errors ignored because of some misbehaving configuration files. - *e, _ = strconv.Atoi(string(p)) + // The only *int fields are UIDs and GIDs. + n, err := parseNumeric(string(p)) + if err != nil { + return false + } + *e = n case *int64: - *e, _ = strconv.ParseInt(string(p), 10, 64) + n, err := strconv.ParseInt(string(p), 10, 64) + if err != nil { + return false + } + *e = n case *[]string: - // Comma-separated lists. - if len(p) != 0 { - *e = strings.Split(string(p), ",") - } else { - *e = []string{} + // Comma-separated lists. Like glibc, skip leading whitespace + // (but keep trailing one), and drop empty elements. + // + // See https://sources.debian.org/src/glibc/2.42-13/nss/nss_files/files-parse.c/#L278-L291 + *e = []string{} + for s := range strings.SplitSeq(string(p), ",") { + if s = strings.TrimLeft(s, " \t\n\v\f\r"); s != "" { + *e = append(*e, s) + } } default: // Someone goof'd when writing code using this function. Scream so they can hear us. panic(fmt.Sprintf("parseLine only accepts {*string, *int, *int64, *[]string} as arguments! %#v is not a pointer!", e)) } } + return true } func ParsePasswdFile(path string) ([]User, error) { @@ -125,8 +148,9 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { ) for s.Scan() { + // Skip blank lines and comments, like glibc does (see ParseGroupFilter). line := bytes.TrimSpace(s.Bytes()) - if len(line) == 0 { + if len(line) == 0 || line[0] == '#' { continue } @@ -136,7 +160,10 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { // root:x:0:0:root:/root:/bin/bash // adm:x:3:4:adm:/var/adm:/bin/false p := User{} - parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell) + if !parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell) { + // Skip malformed lines. + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -195,7 +222,10 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) { // root:x:0:root // adm:x:4:root,adm,daemon p := Group{} - parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List) + if !parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List) { + // Skip malformed lines. + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -235,33 +265,50 @@ 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. +// validateID checks whether a UID or GID is within the supported range, +// returning [ErrRange] if it is not. // -// 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. +// While the Linux kernel allows UIDs up to MaxUint32 - 2, and the OCI +// Runtime Spec does not define a maximum UID, we restrict IDs to MaxInt32 +// for compatibility with 32-bit systems and OCI runtimes. // // See https://github.com/containerd/containerd/commit/de1341c201ffb0effebbf51d00376181968c8779 -func parseNumeric(val string) (int, bool, error) { +func validateID(id int) error { + if id < minID || id > maxID { + return ErrRange + } + return nil +} + +// parseNumeric parses a UID or GID as an integer within the supported range. +func parseNumeric(val string) (int, error) { 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 + return 0, ErrRange } - // Other errors ("invalid base", "invalid bit size"); should never - // happen with strconv.Atoi. - return 0, false, err + return 0, err } - if id < minID || id > maxID { - return 0, true, ErrRange + if err := validateID(id); err != nil { + return 0, err } - return id, true, nil + return id, nil +} + +// parseMaybeNumeric parses a UID or GID as a numeric ID, or indicates +// that the value should be interpreted as a username or group name. +// +// Non-numeric values are expected when passing a username or group name, +// so syntax errors are ignored. The boolean indicates whether the value +// was interpreted as a numeric ID. +func parseMaybeNumeric(val string) (int, bool, error) { + id, err := parseNumeric(val) + 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 + } + return id, true, err } // GetExecUser parses a user specification string (using the passwd and group @@ -398,6 +445,9 @@ func GetAdditionalGroups(additionalGroups []string, group io.Reader) ([]int, err if err != nil { return nil, err } + if groupArg == nil { + return nil, errors.New("empty group name") + } addtlGroups = append(addtlGroups, *groupArg) } @@ -423,9 +473,6 @@ func GetAdditionalGroups(additionalGroups []string, group io.Reader) ([]int, err for _, g := range groups { 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 @@ -462,6 +509,20 @@ func GetAdditionalGroupsPath(additionalGroups []string, groupPath string) ([]int return GetAdditionalGroups(additionalGroups, group) } +// maxSubID is the maximum subordinate ID (maxof(id_t) in shadow-utils). +const maxSubID = 1<<32 - 1 + +// valid reports whether s is a valid subordinate ID range. It follows the +// checks done by shadow-utils: the name must not be empty, and the range +// must fit into 32-bit IDs. +// +// See https://github.com/shadow-maint/shadow/blob/24bc15eac4dcdec9c73c59988fe030b44a98c3de/lib/subordinateio.c#L93-L127 +func (s SubID) valid() bool { + return s.Name != "" && + s.SubID >= 0 && s.SubID <= maxSubID && + s.Count >= 0 && s.Count <= min(maxSubID+1-s.SubID, maxSubID) +} + func ParseSubIDFile(path string) ([]SubID, error) { return ParseSubIDFileFilter(path, nil) } @@ -496,8 +557,15 @@ func ParseSubIDFilter(r io.Reader, filter func(SubID) bool) ([]SubID, error) { } // see: man 5 subuid + // name:subid:count + // Name:SubID:Count + // user:100000:65536 + parts := bytes.Split(line, []byte(":")) p := SubID{} - parseLine(line, &p.Name, &p.SubID, &p.Count) + if len(parts) != 3 || !parseParts(parts, &p.Name, &p.SubID, &p.Count) || !p.valid() { + // Skip malformed lines. + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -545,7 +613,10 @@ func ParseIDMapFilter(r io.Reader, filter func(IDMap) bool) ([]IDMap, error) { // see: man 7 user_namespaces p := IDMap{} - parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count) + if !parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count) { + // Skip malformed lines. + continue + } if filter == nil || filter(p) { out = append(out, p) diff --git a/user/user_test.go b/user/user_test.go index 8d08e1c..ab750fd 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -18,67 +18,127 @@ func TestParseLine(t *testing.T) { var ( a, b string c []string - d int + d, e int ) - parseLine([]byte(""), &a, &b) + if !parseLine([]byte(""), &a, &b) { + t.Fatal("unexpected malformed record") + } if a != "" || b != "" { t.Fatalf("a and b should be empty ('%v', '%v')", a, b) } - parseLine([]byte("a"), &a, &b) + if !parseLine([]byte("a"), &a, &b) { + t.Fatal("unexpected malformed record") + } if a != "a" || b != "" { t.Fatalf("a should be 'a' and b should be empty ('%v', '%v')", a, b) } - parseLine([]byte("bad boys:corny cows"), &a, &b) + if !parseLine([]byte("bad boys:corny cows"), &a, &b) { + t.Fatal("unexpected malformed record") + } if a != "bad boys" || b != "corny cows" { t.Fatalf("a should be 'bad boys' and b should be 'corny cows' ('%v', '%v')", a, b) } - parseLine([]byte(""), &c) + if !parseLine([]byte(""), &c) { + t.Fatal("unexpected malformed record") + } if len(c) != 0 { t.Fatalf("c should be empty (%#v)", c) } - parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c) + if !parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c) { + t.Fatal("unexpected malformed record") + } if a != "g" || b != "h" || len(c) != 3 || c[0] != "i" || c[1] != "j" || c[2] != "k" { t.Fatalf("a should be 'g', b should be 'h', and c should be ['i','j','k'] ('%v', '%v', '%#v')", a, b, c) } - parseLine([]byte("::::::::::"), &a, &b, &c) + if !parseLine([]byte("::::::::::"), &a, &b, &c) { + t.Fatal("unexpected malformed record") + } if a != "" || b != "" || len(c) != 0 { t.Fatalf("a, b, and c should all be empty ('%v', '%v', '%#v')", a, b, c) } - parseLine([]byte("not a number"), &d) + if parseLine([]byte("not a number"), &d) { + t.Fatal("expected malformed record") + } if d != 0 { t.Fatalf("d should be 0 (%v)", d) } - parseLine([]byte("b:12:c"), &a, &d, &b) + if !parseLine([]byte("b:12:c"), &a, &d, &b) { + t.Fatal("unexpected malformed record") + } if a != "b" || b != "c" || d != 12 { t.Fatalf("a should be 'b' and b should be 'c', and d should be 12 ('%v', '%v', %v)", a, b, d) } + + // partial line, missing numeric value + if parseLine([]byte("a:b:3"), &a, &b, &d, &e) { + t.Fatal("expected malformed record") + } + if a != "a" || b != "b" || d != 3 || e != 0 { + t.Fatalf("expected 'a', 'b', 3, 0, got ('%v', '%v', '%v', '%v')", a, b, d, e) + } } 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 +:x:0:0::: +adm:x:3 +adm:x::4: +adm:x:abc:4: +adm:x:2147483648:4: +adm:x:-1:4: +adm:x:3:-1: +valid:x:42:43::/home/valid: +#commented:x:0:0::/root:/bin/sh + # commented:x:0:0::/root:/bin/sh this is just some garbage data `), nil) if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if len(users) != 3 { - t.Fatalf("Expected 3 users, got %v", len(users)) + t.Fatal(err) } - if users[0].Uid != 0 || users[0].Name != "root" { - t.Fatalf("Expected users[0] to be 0 - root, got %v - %v", users[0].Uid, users[0].Name) + + expected := []User{ + { + Name: "root", + Pass: "x", + Uid: 0, + Gid: 0, + Gecos: "root", + Home: "/root", + Shell: "/bin/bash", + }, + { + Name: "adm", + Pass: "x", + Uid: 3, + Gid: 4, + Gecos: "adm", + Home: "/var/adm", + Shell: "/bin/false", + }, + { + Pass: "x", + }, + { + Name: "valid", + Pass: "x", + Uid: 42, + Gid: 43, + Home: "/home/valid", + }, } - if users[1].Uid != 3 || users[1].Name != "adm" { - t.Fatalf("Expected users[1] to be 3 - adm, got %v - %v", users[1].Uid, users[1].Name) + + if !reflect.DeepEqual(users, expected) { + t.Fatalf("unexpected users:\ngot: %#v\nwant: %#v", users, expected) } } @@ -86,19 +146,62 @@ func TestParseGroupFilter(t *testing.T) { groups, err := ParseGroupFilter(strings.NewReader(` root:x:0:root adm:x:4:root,adm,daemon +:x:0: +invalid:x: +invalid:x:: +invalid:x:abc: +invalid:x:2147483648: +invalid:x:-1: +valid:x:42:user1,user2 +spaces:x:43: user1 , user2,, , user3, this is just some garbage data `+largeGroup()), nil) if err != nil { - t.Fatalf("Unexpected error: %v", err) + t.Fatal(err) + } + + expected := []Group{ + { + Name: "root", + Pass: "x", + Gid: 0, + List: []string{"root"}, + }, + { + Name: "adm", + Pass: "x", + Gid: 4, + List: []string{"root", "adm", "daemon"}, + }, + { + Pass: "x", + List: []string{}, + }, + { + Name: "valid", + Pass: "x", + Gid: 42, + List: []string{"user1", "user2"}, + }, + { + Name: "spaces", + Pass: "x", + Gid: 43, + List: []string{"user1 ", "user2", "user3"}, + }, } - if len(groups) != 4 { - t.Fatalf("Expected 4 groups, got %v", len(groups)) + + if len(groups) != len(expected)+1 { + t.Fatalf("expected %d groups, got %d", len(expected)+1, len(groups)) } - if groups[0].Gid != 0 || groups[0].Name != "root" || len(groups[0].List) != 1 { - t.Fatalf("Expected groups[0] to be 0 - root - 1 member, got %v - %v - %v", groups[0].Gid, groups[0].Name, len(groups[0].List)) + if !reflect.DeepEqual(groups[:len(expected)], expected) { + t.Fatalf("unexpected groups:\ngot: %#v\nwant: %#v", groups[:len(expected)], expected) } - if groups[1].Gid != 4 || groups[1].Name != "adm" || len(groups[1].List) != 3 { - t.Fatalf("Expected groups[1] to be 4 - adm - 3 members, got %v - %v - %v", groups[1].Gid, groups[1].Name, len(groups[1].List)) + + // The last entry is generated by largeGroup(). + large := groups[len(expected)] + if large.Name != "largegroup" || large.Pass != "x" || large.Gid != 1000 || len(large.List) != 7500 { + t.Fatalf("unexpected large group: name=%q pass=%q gid=%d members=%d", large.Name, large.Pass, large.Gid, len(large.List)) } } @@ -168,9 +271,9 @@ func TestParseGroupFileCapsReads(t *testing.T) { } } -// TestTestParseGroupFileCapsReadsonRegularFile verifies that non-regular files +// TestParseGroupFileCapsReadsonRegularFile verifies that non-regular files // are refused. -func TestTestParseGroupFileCapsReadsonRegularFile(t *testing.T) { +func TestParseGroupFileCapsReadsonRegularFile(t *testing.T) { fileName := t.TempDir() _, err := ParseGroupFile(fileName) if err == nil { @@ -196,6 +299,41 @@ func TestParseGroupFilterDevZero(t *testing.T) { } } +func TestParseSubIDFilter(t *testing.T) { + entries, err := ParseSubIDFilter(strings.NewReader(` + first:100000:65536 + bad:9223372036854775808:65536 + negative:-1:65536 + negative:100000:-1 + toolarge:4294967296:1 + toolarge:4294967295:2 + toolarge:0:4294967296 + :100000:65536 + short:100000 + long:100000:65536: + empty::65536 + empty:100000: + zero:300000:0 + maxid:4294967295:1 + full:0:4294967295 + last:200000:65536 + `), nil) + if err != nil { + t.Fatal(err) + } + + expected := []SubID{ + {Name: "first", SubID: 100000, Count: 65536}, + {Name: "zero", SubID: 300000, Count: 0}, + {Name: "maxid", SubID: 4294967295, Count: 1}, + {Name: "full", SubID: 0, Count: 4294967295}, + {Name: "last", SubID: 200000, Count: 65536}, + } + if !reflect.DeepEqual(entries, expected) { + t.Fatalf("unexpected entries:\ngot: %#v\nwant: %#v", entries, expected) + } +} + func TestGetExecUser(t *testing.T) { const passwdContent = ` root:x:0:0:root user:/root:/bin/bash @@ -211,7 +349,7 @@ this is just some garbage data groupContent := ` root:x:0:root adm:x:43: -grp:x:1234:root,adm,user7456 +grp:x:1234:root, adm, user7456 444:x:555:111 odd:x:444: 2147483647:x:1235: @@ -634,6 +772,12 @@ this is just some garbage data expected: nil, hasError: true, }, + { + doc: "empty group name", + groups: []string{"adm", ""}, + expected: nil, + hasError: true, + }, { doc: "group entry with out-of-range gid", groups: []string{"toolarge"}, diff --git a/user/user_utils.go b/user/user_utils.go index 8084d3b..ce5a9d8 100644 --- a/user/user_utils.go +++ b/user/user_utils.go @@ -78,7 +78,7 @@ func parseUserArg(name string) (*userArg, error) { if name == "" { return nil, nil } - uid, isNumeric, err := parseNumeric(name) + uid, isNumeric, err := parseMaybeNumeric(name) if err != nil { return nil, err } @@ -109,7 +109,7 @@ func parseGroupArg(name string) (*groupArg, error) { if name == "" { return nil, nil } - gid, isNumeric, err := parseNumeric(name) + gid, isNumeric, err := parseMaybeNumeric(name) if err != nil { return nil, err }