user: parseLine silently ignores uid/gid conversion errors, malformed ids parse as 0
Nobody has claimed this yet.
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 68/100
Research direction
Start in user/user.go at parseParts and trace how ParsePasswdFilter and ParseGroupFilter expose parsed IDs. Reproduce the malformed and out-of-range examples from the issue, then determine whether the chosen behavior is to return conversion errors or skip invalid entries. Done means callers can distinguish invalid IDs from legitimate uid 0 values.
Written by the indexing model from the issue text.
Description
parseParts in user/user.go parses numeric fields with the conversion error discarded:
case *int:
// "numbers", with conversion errors ignored because of some misbehaving configuration files.
*e, _ = strconv.Atoi(string(p))
https://github.com/moby/sys/blob/d6b812df97ee7e0c1e16437fd4d1fa61738427db/user/user.go#L82
Two consequences for consumers that resolve container users from an image's /etc/passwd or /etc/group:
- A malformed id field parses as 0. The line
evil:x:not-a-uid:10::/:/bin/shproducesUser{Name: "evil", Uid: 0}, so looking up that user silently yields root. The caller can't detect this, since uid 0 is also a perfectly legitimate value. - An id past the int range saturates instead of erroring. On 32-bit platforms
4294967296comes back as 2147483647 with no error, so the caller can't range-check it either.
Context: containerd/containerd#13797 added bounds checks on ids resolved from image user databases, but both cases above are invisible to that check because the value has already been collapsed by the time ParsePasswdFilter/ParseGroupFilter return. @fuweid suggested fixing this at the parser level (https://github.com/containerd/containerd/pull/13797#discussion_r3867287873).
I understand the error was dropped deliberately to tolerate misbehaving config files, but for the id fields specifically the failure mode is "arbitrary string becomes root", which seems worth surfacing. Happy to send a PR, either returning an error from the parse functions when an id field doesn't parse, or skipping such entries, whichever direction you prefer.
- Dominant language
- Go
- Stars
- 100
- Forks
- 61
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 3
Getting set up
This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from moby/sys
-
Difficulty 1/5 Under an hour Newbie friendliness 88/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 3/5 1-2 days Newbie friendliness 74/100
-
Difficulty 3/5 1-2 days Newbie friendliness 68/100
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
Similar issues
-
area/docs kind/documentation priority/backlog triage/accepted
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
lexfrei/cloudflare-tunnel-gateway-controller#943 ·
Maintainers usually reply within 1 day
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
keyxmakerx/Chronicle#967 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
Maintainers usually reply within 1 day
-
type/bug
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
Maintainers usually reply within 1 day
-
[E2E Scenario Tests] HTTP logs capture export requests from test framework, polluting golden filesOpen
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
GoogleCloudPlatform/k8s-config-connector#13675 ·
Maintainers usually reply within 1 day