Fix find free range 1629#1668
Merged
alejandro-colomar merged 3 commits intoJul 10, 2026
Merged
Conversation
Contributor
Author
silverhadch
force-pushed
the
fix-find-free-range-1629
branch
from
e41a47e to
c6c8540
Compare
silverhadch
force-pushed
the
fix-find-free-range-1629
branch
from
c6c8540 to
3cda4d9
Compare
silverhadch
force-pushed
the
fix-find-free-range-1629
branch
2 times, most recently
from
270e76b to
f8b3bcc
Compare
alejandro-colomar
approved these changes
Jul 9, 2026
Fix bug from de7f1c7 that changed from using unsigned long to id_t. new_subid_range() returns the same value as the ceiling allowing potentially overlapping subordinate ID ranges. In a nutshell, the commit updated lib/subordinateio.c which now allows creation of overlapping subordinate ID ranges. This is caused because the commit changed from unsigned long to id_t. The bug is that the internal algorithm still used an exclusive endpoint expression, max + 1, which was only safe while the intermediate type was wide enough. So find_free_range() accepts max as an inclusive ID ceiling, but internally compares holes against max + 1. When max is (id_t)-1 on an unsigned-ID build, max + 1 wraps to zero. new_subid_range() passes exactly that value as the ceiling. Fix it by doing the internal arithmetic in signed types that are strictly wider than id_t, so that max + 1, last + 1, and the hole computations cannot wrap: - The static_assert checks that long long is wider than id_t. That makes max + 1LL exact, and since intmax_t is at least as wide as long long, it also guarantees that the intmax_t variables are wider than id_t. - count is converted up front into the intmax_t variable n, which the input check and the later size comparisons use directly, without casts. The input check is done in signed arithmetic, as n > max - min + 1LL: max >= min holds at that point, so max - min is in the range 0 .. (id_t)-1, and adding 1LL to that fits in long long. - The end of each range is computed as first - 1LL + range->count, which likewise needs no cast. - The final check on the remaining unclaimed area is written as max - low >= n - 1 rather than (max - low) + 1 >= n: low <= max and n >= 1 hold there, so neither side of the comparison can overflow. This was found with N184, an open source bug and security vulnerability scanner: https://github.com/MillaFleurs/N184 Closes: <shadow-maint#1629> Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
range->start and range->count are parsed from /etc/subuid and /etc/subgid as unsigned long, so a corrupt or malicious entry was not bounded by id_t: find_free_range() assigned such a start to intmax_t, which is implementation-defined above INTMAX_MAX, and the range-end computation could exceed INTMAX_MAX. Validate the values where they are parsed, instead of hardening each consumer: bound start to maxof(id_t) in subordinate_parse(), and bound count so that the last ID of the range, start + count - 1, fits in id_t as well; the MIN() caps that bound at maxof(id_t), which also keeps it representable in unsigned long on 32-bit systems. a2ul() reports ERANGE for out-of-range values. Rejected entries are skipped when iterating the database (commonio_next() skips entries whose parse failed), while the raw line is still preserved if the file is rewritten. This protects all consumers of these fields, not just allocation. After this, every visible range satisfies 0 <= start <= start + count - 1 <= (id_t)-1, so the intmax_t arithmetic in find_free_range() is exact and cannot overflow, and oversized values no longer need to be handled there. This is a separate, pre-existing problem from the regression fixed in the previous commit: before de7f1c7 this arithmetic was done in unsigned long and silently wrapped for such values. Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
Empty (count == 0) ranges constrain nothing, so skip them instead of processing them. Besides documenting that invariant, this keeps the range-end computation, first - 1LL + range->count, away from the count == 0 corner, where for first == 0 it would produce -1 through unsigned long wrap-around and an implementation-defined conversion on 64-bit systems. Such entries exist in the wild, since old useradd versions used to create them, so they are still accepted by the parser; they are only ignored during allocation. Co-authored-by: Dan Anderson <https://github.com/MillaFleurs> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Hadi Chokr <hadichokr@icloud.com>
silverhadch
force-pushed
the
fix-find-free-range-1629
branch
from
f8b3bcc to
e92b9dd
Compare
alejandro-colomar
approved these changes
Jul 10, 2026
alejandro-colomar
left a comment
alejandro-colomar
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! I'll merge.
alejandro-colomar
merged commit 36072d8
into
shadow-maint:master
14 of 16 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the security regression from de7f1c7 (overlapping subordinate ID ranges, #1629), continuing @MillaFleurs' work from #1630 with all review comments applied. Kept Co-authored-by: Dan Anderson https://github.com/MillaFleurs in both commits as requested.
Closes #1629
Replaces #1630
Full disclosure carried over from the original PR: this was found with N184, @MillaFleurs' open source bug and security vulnerability scanner (https://github.com/MillaFleurs/N184).