subids: reject subid file entries that do not fit in id_t

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 de7f1c78f70f8df4b2956f7363da3774348bc58e
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>

diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index 9602eb55c3911a1b502029f8f5bb76d37d9921b9..af3b2807cd13f8b177d26b5e63dc51020b5221a5 100644 (file)
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -14,6 +14,7 @@
 #include "getdef.h"
 #include "../libsubid/subid.h"
 #include <sys/types.h>
+#include <sys/param.h>
 #include <pwd.h>
 #include <ctype.h>
 #include <fcntl.h>
@@ -113,10 +114,13 @@ subordinate_parse(const char *line)
        if (streq(fields[2], ""))
                return NULL;
        range.owner = fields[0];
-       if (str2ul(&range.start, fields[1]) == -1)
+       if (a2ul(&range.start, fields[1], NULL, 0, 0, maxof(id_t)) == -1)
                return NULL;
-       if (str2ul(&range.count, fields[2]) == -1)
+       if (a2ul(&range.count, fields[2], NULL, 0, 0,
+                MIN(maxof(id_t) + 1LL - range.start, maxof(id_t))) == -1)
+       {
                return NULL;
+       }
 
        return &range;
 }