From 4f45777f24315d82408166dd6f1dfc7a0f1826bf Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 27 Oct 2026 19:10:12 +0100 Subject: [PATCH 1/8] user: fix typo in test-name Signed-off-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- user/user_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/user/user_test.go b/user/user_test.go index 8d08e1c5..94ef106b 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -168,9 +168,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 { From 0d3073e7875d86559c10cc83888b3a3720f91ab6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 27 Oct 2026 17:18:15 +0100 Subject: [PATCH 2/8] user: split numeric ID parsing from name detection Split parseNumeric into a strict numeric parser and a parseMaybeNumeric wrapper that allows non-numeric values to be interpreted as user or group names. This allows passwd and group file parsing to reject invalid numeric IDs without changing the behavior of callers that accept names. Signed-off-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- user/user.go | 36 ++++++++++++++++++++++-------------- user/user_utils.go | 4 ++-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/user/user.go b/user/user.go index 48c34a8b..60c26be5 100644 --- a/user/user.go +++ b/user/user.go @@ -235,33 +235,41 @@ 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. +// parseNumeric parses a UID or GID as an integer 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) { +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 + return 0, ErrRange } - 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 diff --git a/user/user_utils.go b/user/user_utils.go index 8084d3b0..ce5a9d8f 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 } From e994e3ece53a33283c9779a0120b518d14b38826 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 28 Oct 2026 11:00:56 +0100 Subject: [PATCH 3/8] user: extract accepted range validation to a helper Signed-off-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- user/user.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/user/user.go b/user/user.go index 60c26be5..966cab6f 100644 --- a/user/user.go +++ b/user/user.go @@ -235,13 +235,22 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath return GetExecUser(userSpec, defaults, passwd, group) } -// parseNumeric parses a UID or GID as an integer 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 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 { @@ -250,8 +259,8 @@ func parseNumeric(val string) (int, error) { } return 0, err } - if id < minID || id > maxID { - return 0, ErrRange + if err := validateID(id); err != nil { + return 0, err } return id, nil } @@ -431,8 +440,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 From 73884a633ea7a0c781bde132162142762709dfed Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 12 Nov 2026 18:02:34 -0800 Subject: [PATCH 4/8] user: skip empty and malformed records Skip empty and malformed records when parsing passwd and group files, instead of accepting incomplete records, with missing or invalid numeric fields silently parsed as 0. Treat missing fields as empty, allowing empty string fields while rejecting missing, non-numeric, or out-of-range numeric fields. This aligns with glibc, which skips malformed records when reading passwd and group files. For UIDs and GIDs, the accepted range is the one checked by validateID. This also rejects negative IDs (which setresuid and setresgid treat as "do not change"), and makes the result the same on 32-bit and 64-bit platforms. As parsed GIDs are now always in range, remove the redundant validateID check from GetAdditionalGroups. Since any parsing error makes the record malformed, parseLine and parseParts now only report whether the record is valid. Add tests for empty, incomplete, and malformed records, and document the behavior based on the corresponding glibc implementations. For reference: - https://sources.debian.org/src/glibc/2.42-13/nss/nss_fgetent_r.c/ - https://sources.debian.org/src/glibc/2.42-13/nss/fgetpwent_r.c/ Co-authored-by: Mickael Emirkanian Co-authored-by: Kir Kolyshkin Signed-off-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- user/user.go | 74 ++++++++++++++------- user/user_test.go | 161 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 188 insertions(+), 47 deletions(-) diff --git a/user/user.go b/user/user.go index 966cab6f..b6d6063f 100644 --- a/user/user.go +++ b/user/user.go @@ -57,32 +57,50 @@ 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 { @@ -95,6 +113,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 } func ParsePasswdFile(path string) ([]User, error) { @@ -136,7 +155,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 +217,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) @@ -440,9 +465,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 err := validateID(g.Gid); err != nil { - return nil, err - } gidMap[g.Gid] = struct{}{} found = true break @@ -514,7 +536,10 @@ 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) + if !parseLine(line, &p.Name, &p.SubID, &p.Count) { + // Skip malformed lines. + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -562,7 +587,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 94ef106b..bc10f0ea 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -18,67 +18,125 @@ 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: 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 +144,55 @@ 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 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"}, + }, } - 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)) } } @@ -196,6 +290,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 From 56b1b5d8ba9123d86b661311acc2a280566b2ce4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 28 Oct 2026 12:01:36 -0700 Subject: [PATCH 5/8] user: ParsePasswdFilter: skip comments ParseGroupFilter skips lines starting with '#', following glibc, but ParsePasswdFilter does not. As a result, a commented out entry such as "#olduser:x:0:0::/root:/bin/sh" is returned as a user named "#olduser" with UID 0, which GetExecUser can then match by UID. Skip comments in ParsePasswdFilter, too. Signed-off-by: Kir Kolyshkin --- user/user.go | 3 ++- user/user_test.go | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/user/user.go b/user/user.go index b6d6063f..9fe61381 100644 --- a/user/user.go +++ b/user/user.go @@ -144,8 +144,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 } diff --git a/user/user_test.go b/user/user_test.go index bc10f0ea..479f6fdd 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -98,6 +98,8 @@ 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 { From 6f25a8a34f174a2ab32cc0bd447fb52f48a71d3e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 28 Oct 2026 12:01:45 -0700 Subject: [PATCH 6/8] user: GetAdditionalGroups: fix panic on empty group name For an empty string, parseGroupArg returns nil, nil, which is then dereferenced, resulting in a nil pointer dereference panic. Return an error instead. Signed-off-by: Kir Kolyshkin --- user/user.go | 3 +++ user/user_test.go | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/user/user.go b/user/user.go index 9fe61381..9cc5e317 100644 --- a/user/user.go +++ b/user/user.go @@ -441,6 +441,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) } diff --git a/user/user_test.go b/user/user_test.go index 479f6fdd..7bac1745 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -749,6 +749,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"}, From e8b26ca26558ee03f25f96f8a98a25e7ac3f8c43 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 28 Oct 2026 13:06:09 -0700 Subject: [PATCH 7/8] user: skip leading whitespace and empty group members glibc skips whitespace before group members and drops empty ones, so a group line like "grp:x:1234:root, adm,,user" has three members: "root", "adm", and "user". We kept " adm" and "" as is, so user adm was not considered a member of grp, and GetExecUser silently omitted the group from the supplementary groups. Do the same as glibc: skip leading whitespace in group members, and drop empty ones. Note that, like in glibc, trailing whitespace is kept, so "root ,adm" has members "root " and "adm". Signed-off-by: Kir Kolyshkin --- user/user.go | 14 +++++++++----- user/user_test.go | 9 ++++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/user/user.go b/user/user.go index 9cc5e317..4043c45e 100644 --- a/user/user.go +++ b/user/user.go @@ -102,11 +102,15 @@ func parseParts(parts [][]byte, fields ...any) bool { } *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. diff --git a/user/user_test.go b/user/user_test.go index 7bac1745..45a31e58 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -153,6 +153,7 @@ 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 { @@ -182,6 +183,12 @@ this is just some garbage data Gid: 42, List: []string{"user1", "user2"}, }, + { + Name: "spaces", + Pass: "x", + Gid: 43, + List: []string{"user1 ", "user2", "user3"}, + }, } if len(groups) != len(expected)+1 { @@ -326,7 +333,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: From 335ef47d48eef51680f9377232f3d4009bc3dddb Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 28 Oct 2026 13:06:40 -0700 Subject: [PATCH 8/8] user: ParseSubIDFilter: validate records like shadow-utils ParseSubIDFilter accepts records which shadow-utils rejects, such as ones with negative or too large IDs or counts ("user:-1:65536", "user:4294967296:1"), with an empty name, or with an extra field. Validate records the same way as shadow-utils' subordinate_parse does: there must be exactly three fields, the name must not be empty, and the range must fit into 32-bit IDs. Skip records that fail the validation. Rename TestParseSubIDFilterOverflow to TestParseSubIDFilter, and add more test cases. Signed-off-by: Kir Kolyshkin --- user/user.go | 20 +++++++++++++++++++- user/user_test.go | 18 +++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/user/user.go b/user/user.go index 4043c45e..3d77c540 100644 --- a/user/user.go +++ b/user/user.go @@ -509,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) } @@ -543,8 +557,12 @@ 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{} - if !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 } diff --git a/user/user_test.go b/user/user_test.go index 45a31e58..ab750fd8 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -299,10 +299,23 @@ func TestParseGroupFilterDevZero(t *testing.T) { } } -func TestParseSubIDFilterOverflow(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 { @@ -311,6 +324,9 @@ func TestParseSubIDFilterOverflow(t *testing.T) { 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) {