From: Hadi Chokr <hadichokr@icloud.com>
Date: Mon, 6 Jul 2026 12:29:13 +0000 (+0200)
Subject: subids: fix security regression which can cause overlapping ranges
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b31ba8af5d4a351282605e85687dbdc90cdec264;p=thirdparty%2Fshadow.git

subids: fix security regression which can cause overlapping ranges

Fix bug from de7f1c78f70f8df4b2956f7363da3774348bc58e 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: <https://github.com/shadow-maint/shadow/issues/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>
---

diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index c900ebc34..9602eb55c 100644
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -17,6 +17,7 @@
 #include <pwd.h>
 #include <ctype.h>
 #include <fcntl.h>
+#include <stdint.h>
 #include <string.h>
 
 #include "alloc/malloc.h"
@@ -30,6 +31,9 @@
 #include "string/strtok/strsep2arr.h"
 #include "typetraits.h"
 
+#undef NDEBUG
+#include <assert.h>
+
 
 #define ID_SIZE 31
 
@@ -333,10 +337,13 @@ static int subordinate_range_cmp (const void *p1, const void *p2)
 static id_t
 find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
 {
-	id_t                           low, high;
+	static_assert(sizeof(long long) > sizeof(id_t), "");
+
+	intmax_t                       n, low, high;
 	const struct subordinate_range *range;
 
-	if (count == 0 || max < min || count - 1 > max - min) {
+	n = count;
+	if (n == 0 || max < min || n > max - min + 1LL) {
 		errno = ERANGE;
 		return -1;
 	}
@@ -347,21 +354,21 @@ find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
 
 	low = min;
 	while (NULL != (range = commonio_next(db))) {
-		id_t  first, last;
+		intmax_t  first, last;
 
 		first = range->start;
-		last = first + range->count - 1;
+		last = first - 1LL + range->count;
 
 		/* Find the top end of the hole before this range */
 		high = first;
 
 		/* Don't allocate IDs after max (included) */
-		if (high > max + 1) {
-			high = max + 1;
+		if (high > max + 1LL) {
+			high = max + 1LL;
 		}
 
 		/* Is the hole before this range large enough? */
-		if ((high > low) && ((high - low) >= count))
+		if ((high > low) && ((high - low) >= n))
 			return low;
 
 		/* Compute the low end of the next hole */
@@ -372,7 +379,7 @@ find_free_range(struct commonio_db *db, id_t min, id_t max, unsigned long count)
 	}
 
 	/* Is the remaining unclaimed area large enough? */
-	if (((max - low) + 1) >= count)
+	if (max - low >= n - 1)
 		return low;
 fail:
 	errno = EUSERS;