user: prevent falling back to looking up numeric usernames#221
user: prevent falling back to looking up numeric usernames#221thaJeztah wants to merge 5 commits into
Conversation
vvoland
left a comment
There was a problem hiding this comment.
This only tests the case where gid/uid still falls within int .
If it's larger, it will still resolve to root because it will be treated as an username and still resolved.
So:
9223372036854775808:x:0:0:root:/root:/bin/sh
USER 9223372036854775808
Means that the runtime will actually resolve it to root.
This is "correct" behavior, but its confusing to treat a numeric value as an username.
|
@vvoland - trying to see where that codepath would be; testing |
|
On 64 bit |
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
Do we need the same for |
|
Yeah, makes sense; also updated As a follow-up, I'm considering cleaning up this code; the logic seems to be much too complicated. |
| if g.Name == ag || strconv.Itoa(g.Gid) == ag { | ||
| for _, ag := range addtlGroups { | ||
| if ag.isNumeric { | ||
| return g.Gid == ag.gid |
There was a problem hiding this comment.
Should this be
| return g.Gid == ag.gid | |
| if g.Gid == ag.gid { | |
| return true | |
| } |
?
There was a problem hiding this comment.
No, don't think so, because then it would continue to check if g.Name == ag.name {, and we know it's a numeric value, so we can return the results (did it match, or not?)
There was a problem hiding this comment.
Doh! You're right though; because we still want to continue the loop here 🙈
There was a problem hiding this comment.
Updated, and added a test-case;
diff --git a/user/user.go b/user/user.go
index 8012094..f47e250 100644
--- a/user/user.go
+++ b/user/user.go
@@ -480,8 +480,8 @@ func GetAdditionalGroups(additionalGroups []string, group io.Reader) ([]int, err
var err error
groups, err = ParseGroupFilter(group, func(g Group) bool {
for _, ag := range addtlGroups {
- if ag.isNumeric {
- return g.Gid == ag.gid
+ if ag.isNumeric && g.Gid == ag.gid {
+ return true
}
if g.Name == ag.name {
return true
diff --git a/user/user_test.go b/user/user_test.go
index 1e16861..6837e53 100644
--- a/user/user_test.go
+++ b/user/user_test.go
@@ -477,6 +477,11 @@ this is just some garbage data
groups: []string{"adm"},
expected: []int{43},
},
+ {
+ // numeric group miss must continue checking remaining groups
+ groups: []string{"10001", "adm"},
+ expected: []int{43, 10001},
+ },
{
// multiple groups
groups: []string{"adm", "grp"},| for i, r := range val { | ||
| if i == 0 && (r == '+' || r == '-') { | ||
| continue | ||
| } | ||
| if r < '0' || r > '9' { | ||
| return 0, false, nil | ||
| } | ||
| } | ||
| if val == "+" || val == "-" { | ||
| return 0, false, nil | ||
| } | ||
|
|
||
| id, err := strconv.Atoi(val) |
There was a problem hiding this comment.
I wonder why we don't use something like containerd/containerd@85706b6 (i.e. use strconv.Atoi or strconv.ParseUint) and check its errors, as well as the range. Basically, here we parse the string twice, duplicating the strconv functionality.
There was a problem hiding this comment.
Ah, you're right; I was trying to detect early because the logic is slightly different here, but we can use the error returned here to detect whether it was a range error, or a syntax error (non-numeric).
Let me update
Improve handling of numeric user/group to prevent looking up numeric values as usernames. This fixes a similar issue as CVE-2026-46680 in containerd. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b9d371a to
2d39888
Compare
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Addresses a similar issue as CVE-2026-46680 in containerd.