diff --git a/user/user.go b/user/user.go index 48c34a8..a35584d 100644 --- a/user/user.go +++ b/user/user.go @@ -57,32 +57,76 @@ 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. +// +// Empty lines and records with missing, invalid, or out-of-range numeric +// fields are considered malformed and return ok=false. Other parsing +// errors return ok=true with a non-nil error. +// +// 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/ +func parseLine(line []byte, v ...any) (ok bool, _ error) { + line = bytes.TrimSpace(line) + if len(line) == 0 { + return false, nil + } + return parseParts(bytes.Split(line, []byte(":")), v...) } -func parseParts(parts [][]byte, v ...any) { +// parseParts parses the provided parts into the corresponding fields. +// +// 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 (ok=false). +// +// The returned boolean indicates whether the record is valid, not whether +// parsing succeeded. Other parsing errors return ok=true with a non-nil error. +func parseParts(parts [][]byte, fields ...any) (ok bool, _ error) { if len(parts) == 0 { - return + return false, nil } - 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 + for i, v := range fields { + // Treat missing fields as empty. Like glibc, empty string fields are + // accepted, but missing or invalid numeric fields make the record invalid. + // + // See https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c/#L150-L165 + 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)) + n, err := strconv.Atoi(string(p)) + if err != nil { + if errors.Is(err, strconv.ErrSyntax) { + return false, fmt.Errorf("parsing integer field %d: %q is not numeric: %w", i, p, strconv.ErrSyntax) + } + if errors.Is(err, strconv.ErrRange) { + return false, fmt.Errorf("parsing integer field %d: %w", i, err) + } + return true, fmt.Errorf("parsing integer field %d: %w", i, err) + } + *e = n case *int64: - *e, _ = strconv.ParseInt(string(p), 10, 64) + n, err := strconv.ParseInt(string(p), 10, 64) + if err != nil { + if errors.Is(err, strconv.ErrSyntax) { + return false, fmt.Errorf("parsing integer field %d: %q is not numeric: %w", i, p, strconv.ErrSyntax) + } + if errors.Is(err, strconv.ErrRange) { + return false, fmt.Errorf("parsing integer field %d: %w", i, err) + } + return true, fmt.Errorf("parsing integer field %d: %w", i, err) + } + *e = n case *[]string: // Comma-separated lists. if len(p) != 0 { @@ -95,6 +139,7 @@ func parseParts(parts [][]byte, v ...any) { panic(fmt.Sprintf("parseLine only accepts {*string, *int, *int64, *[]string} as arguments! %#v is not a pointer!", e)) } } + return true, nil } func ParsePasswdFile(path string) ([]User, error) { @@ -136,7 +181,14 @@ 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) + ok, err := parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell) + if !ok { + // Skip malformed and empty lines, and don't consider them. + continue + } + if err != nil { + return nil, fmt.Errorf("invalid line %q: %w", line, err) + } if filter == nil || filter(p) { out = append(out, p) @@ -195,7 +247,14 @@ 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) + ok, err := parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List) + if !ok { + // Skip malformed and empty lines, and don't consider them. + continue + } + if err != nil { + return nil, fmt.Errorf("invalid line %q: %w", line, err) + } if filter == nil || filter(p) { out = append(out, p) @@ -235,33 +294,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 @@ -423,8 +499,8 @@ 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 + if err := validateID(g.Gid); err != nil { + return nil, err } gidMap[g.Gid] = struct{}{} found = true @@ -497,7 +573,14 @@ func ParseSubIDFilter(r io.Reader, filter func(SubID) bool) ([]SubID, error) { // see: man 5 subuid p := SubID{} - parseLine(line, &p.Name, &p.SubID, &p.Count) + ok, err := parseLine(line, &p.Name, &p.SubID, &p.Count) + if !ok { + // Skip malformed and empty lines, and don't consider them. + continue + } + if err != nil { + return nil, fmt.Errorf("invalid line %q: %w", line, err) + } if filter == nil || filter(p) { out = append(out, p) @@ -545,7 +628,14 @@ 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) + ok, err := parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count) + if !ok { + // Skip malformed and empty lines, and don't consider them. + continue + } + if err != nil { + return nil, fmt.Errorf("invalid line %q: %w", line, err) + } if filter == nil || filter(p) { out = append(out, p) diff --git a/user/user_test.go b/user/user_test.go index 8d08e1c..c38580d 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -18,67 +18,145 @@ func TestParseLine(t *testing.T) { var ( a, b string c []string - d int + d, e int ) - parseLine([]byte(""), &a, &b) + _, err := parseLine([]byte(""), &a, &b) + if err != nil { + t.Fatal(err) + } if a != "" || b != "" { t.Fatalf("a and b should be empty ('%v', '%v')", a, b) } - parseLine([]byte("a"), &a, &b) + _, err = parseLine([]byte("a"), &a, &b) + if err != nil { + t.Fatal(err) + } 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) + _, err = parseLine([]byte("bad boys:corny cows"), &a, &b) + if err != nil { + t.Fatal(err) + } 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) + ok, err := parseLine([]byte(""), &c) + if err != nil { + t.Fatal(err) + } + if ok { + t.Fatalf("ok should be false for empty lines") + } 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) + _, err = parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c) + if err != nil { + t.Fatal(err) + } 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) + _, err = parseLine([]byte("::::::::::"), &a, &b, &c) + if err != nil { + t.Fatal(err) + } 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) + _, err = parseLine([]byte("not a number"), &d) + if err == nil { + t.Fatal("expected an error") + } if d != 0 { t.Fatalf("d should be 0 (%v)", d) } - parseLine([]byte("b:12:c"), &a, &d, &b) + _, err = parseLine([]byte("b:12:c"), &a, &d, &b) + if err != nil { + t.Fatal(err) + } 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 + ok, err = parseLine([]byte("a:b:3"), &a, &b, &d, &e) + if ok { + t.Fatal("expected malformed record") + } + expErr := `parsing integer field 3: "" is not numeric: invalid syntax` + if err == nil || err.Error() != expErr { + t.Fatalf("expected '%s', got: %v", expErr, err) + } + 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: +valid:x:42:43::/home/valid: 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: "adm", + Pass: "x", + Uid: 2147483648, + Gid: 4, + }, + { + 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 +164,60 @@ 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: +valid:x:42:user1,user2 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: "invalid", + Pass: "x", + Gid: 2147483648, + List: []string{}, + }, + { + Name: "valid", + Pass: "x", + Gid: 42, + List: []string{"user1", "user2"}, + }, } - 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 +287,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 +315,25 @@ func TestParseGroupFilterDevZero(t *testing.T) { } } +func TestParseSubIDFilterOverflow(t *testing.T) { + entries, err := ParseSubIDFilter(strings.NewReader(` + first:100000:65536 + bad:9223372036854775808:65536 + last:200000:65536 + `), nil) + if err != nil { + t.Fatal(err) + } + + expected := []SubID{ + {Name: "first", SubID: 100000, Count: 65536}, + {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 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 }