From 9bab45122243842766ce715f2a2a853fdb457f08 Mon Sep 17 00:00:00 2001 From: Eduard Zateev Date: Sun, 18 Oct 2026 10:38:19 +0200 Subject: [PATCH] user: skip passwd entries with invalid IDs The passwd and group parsers discard integer conversion errors, which turns malformed UID and GID fields into zero. On 32-bit systems, overflowing values can also saturate to the maximum valid ID. Reject invalid and out-of-range UID/GID fields, skipping those passwd/group entries while continuing to parse valid entries. Add regression coverage for malformed, negative, and overflowing IDs. Fixes: #248 Signed-off-by: Eduard Zateev --- user/user.go | 35 +++++++++++++++++++-------- user/user_test.go | 61 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 76 insertions(+), 20 deletions(-) diff --git a/user/user.go b/user/user.go index e7477677..708c29eb 100644 --- a/user/user.go +++ b/user/user.go @@ -56,13 +56,13 @@ type IDMap struct { Count int64 } -func parseLine(line []byte, v ...any) { - parseParts(bytes.Split(line, []byte(":")), v...) +func parseLine(line []byte, v ...any) error { + return parseParts(bytes.Split(line, []byte(":")), v...) } -func parseParts(parts [][]byte, v ...any) { +func parseParts(parts [][]byte, v ...any) error { if len(parts) == 0 { - return + return nil } for i, p := range parts { @@ -78,8 +78,18 @@ func parseParts(parts [][]byte, v ...any) { case *string: *e = string(p) case *int: - // "numbers", with conversion errors ignored because of some misbehaving configuration files. - *e, _ = strconv.Atoi(string(p)) + // UID and GID values must fit the range supported by all platforms. + id, err := strconv.ParseInt(string(p), 10, 32) + if err != nil { + if errors.Is(err, strconv.ErrRange) { + return ErrRange + } + return fmt.Errorf("parsing integer field %d: %w", i, err) + } + if id < minID || id > maxID { + return ErrRange + } + *e = int(id) case *int64: *e, _ = strconv.ParseInt(string(p), 10, 64) case *[]string: @@ -94,6 +104,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 nil } func ParsePasswdFile(path string) ([]User, error) { @@ -135,7 +146,9 @@ 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 err := parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell); err != nil { + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -194,7 +207,9 @@ 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 err := parseLine(line, &p.Name, &p.Pass, &p.Gid, &p.List); err != nil { + continue + } if filter == nil || filter(p) { out = append(out, p) @@ -501,7 +516,7 @@ 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) + _ = parseLine(line, &p.Name, &p.SubID, &p.Count) if filter == nil || filter(p) { out = append(out, p) @@ -549,7 +564,7 @@ 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) + _ = parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count) if filter == nil || filter(p) { out = append(out, p) diff --git a/user/user_test.go b/user/user_test.go index 8d08e1c5..42b49c6d 100644 --- a/user/user_test.go +++ b/user/user_test.go @@ -15,48 +15,53 @@ import ( ) func TestParseLine(t *testing.T) { + mustParseLine := func(line []byte, v ...any) { + t.Helper() + if err := parseLine(line, v...); err != nil { + t.Fatal(err) + } + } var ( a, b string c []string d int ) - parseLine([]byte(""), &a, &b) + mustParseLine([]byte(""), &a, &b) if a != "" || b != "" { t.Fatalf("a and b should be empty ('%v', '%v')", a, b) } - parseLine([]byte("a"), &a, &b) + mustParseLine([]byte("a"), &a, &b) 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) + mustParseLine([]byte("bad boys:corny cows"), &a, &b) 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) + mustParseLine([]byte(""), &c) 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) + mustParseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c) 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) + mustParseLine([]byte("::::::::::"), &a, &b, &c) 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 d != 0 { - t.Fatalf("d should be 0 (%v)", d) + if err := parseLine([]byte("not a number"), &d); err == nil { + t.Fatal("expected an error for an invalid numeric field") } - parseLine([]byte("b:12:c"), &a, &d, &b) + mustParseLine([]byte("b:12:c"), &a, &d, &b) 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) } @@ -82,6 +87,42 @@ this is just some garbage data } } +func TestParsePasswdFilterSkipsInvalidIDs(t *testing.T) { + valid := "valid:x:100:100::/home/valid:/bin/sh" + tests := []struct { + name string + line string + }{ + {name: "invalid UID", line: "evil:x:not-a-uid:10::/home/evil:/bin/sh"}, + {name: "invalid GID", line: "evil:x:10:not-a-gid::/home/evil:/bin/sh"}, + {name: "UID overflow", line: "evil:x:4294967296:10::/home/evil:/bin/sh"}, + {name: "negative UID", line: "evil:x:-1:10::/home/evil:/bin/sh"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + users, err := ParsePasswdFilter(strings.NewReader(tc.line+"\n"+valid), nil) + if err != nil { + t.Fatalf("ParsePasswdFilter: %v", err) + } + if len(users) != 1 || users[0].Name != "valid" || users[0].Uid != 100 { + t.Fatalf("expected only the valid passwd entry, got %+v", users) + } + }) + } +} + +func TestParseGroupFilterSkipsInvalidGID(t *testing.T) { + for _, line := range []string{"evil:x:not-a-gid:root", "evil:x:4294967296:root"} { + groups, err := ParseGroupFilter(strings.NewReader(line+"\nvalid:x:42:validuser"), nil) + if err != nil { + t.Fatalf("ParseGroupFilter: %v", err) + } + if len(groups) != 1 || groups[0].Name != "valid" || groups[0].Gid != 42 { + t.Errorf("expected only the valid group entry, got %+v", groups) + } + } +} + func TestParseGroupFilter(t *testing.T) { groups, err := ParseGroupFilter(strings.NewReader(` root:x:0:root