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 c900ebc34a1a7529fc3245208a13be10d3bba0f0..9602eb55c3911a1b502029f8f5bb76d37d9921b9 100644 (file)
--- 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;