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))
|
*e, _ = strconv.Atoi(string(p)) |
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/sh produces User{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
4294967296 comes 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 (containerd/containerd#13797 (comment)).
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.
parsePartsin user/user.go parses numeric fields with the conversion error discarded:sys/user/user.go
Line 82 in d6b812d
Two consequences for consumers that resolve container users from an image's /etc/passwd or /etc/group:
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.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 (containerd/containerd#13797 (comment)).
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.