Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/idmapping.c: Fix get_map_ranges regression #1117

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

stoeckmann
Copy link
Contributor

If get_map_ranges is called on a 32 bit system, where UINT_MAX equals ULONG_MAX, the count check allows illegal values if uid and loweruid are both UINT_MAX due to unsigned integer underflow.

Always use the fix size UINT_MAX as count limitation due to guarantee that sizeof(unsigned int) <= sizeof(unsigned long) in C language.

Fixes: 7c43eb2 ("lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call")

Proof of Concept (32 bit system):

newuidmap $$ 4294967295 4294967295 10

Output:

newuidmap: uid range [4294967295-9) -> [4294967295-9) not allowed

Expected:

newuidmap: subuid overflow detected.
usage: newuidmap [<pid>|fd:<pidfd>] <uid> <loweruid> <count> [ <uid> <loweruid> <count> ] ...

lib/idmapping.c Outdated Show resolved Hide resolved
@alejandro-colomar
Copy link
Collaborator

(I've approved it, but it's really waiting for the commit message update.)

The get_map_ranges function shall support the whole accepted range
as specified in user_namespaces(7), i.e. upper and lower from 0 to
UINT_MAX - 1 as well as range from 1 to UINT_MAX. The actual limit of
range depends on values of upper and lower and adding the range
to either upper or lower shall never overflow UINT_MAX.

Fixes: 7c43eb2 (2024-07-11, "lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call")
Fixes: ff2baed (2016-08-14, "idmapping: add more checks for overflow")
Fixes: 94da3dc (2016-08-14, "also check upper for wrap")
Fixes: 7f5a148 (2016-07-31, "get_map_ranges: check for overflow")
Co-authored-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@stoeckmann
Copy link
Contributor Author

(I've approved it, but it's really waiting for the commit message update.)

Almost just in time ... ;)

@alejandro-colomar
Copy link
Collaborator

(I've approved it, but it's really waiting for the commit message update.)

Almost just in time ... ;)

:) Thanks! I'll merge.

@alejandro-colomar alejandro-colomar merged commit 929e61d into shadow-maint:master Nov 13, 2024
9 checks passed
@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Nov 13, 2024

I'm happy that refactoring/simplifying the old code, even though it introduced a bug, it made it more readable and obvious that the previous code was bad, and we've been able to improve the checks beyond what was before my regression. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants