String-to-numeric hardening

[alejandro-colomar]

---

v3

v3 changes:

• Rebase

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  62e21a24 =  1:  14ef2653 lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  c993320f =  2:  c4ad6a5d lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  d7186a5a =  3:  212a171b lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  3efa3a4d =  4:  8c0b20ed lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  06d88123 =  5:  2b1b6da9 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  962b4e45 =  6:  7c20cbef lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  41adf242 =  7:  1c6e2418 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  36ff918f =  8:  686d8de8 lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  9f049eef !  9:  fa5581d1 lib/atoi/: Use libbsd's strto[iu](3bsd) when available
    @@ lib/atoi/a2i.h
      
      #include "atoi/strtoi.h"
      #include "atoi/strtou_noneg.h"
    -@@ lib/atoi/a2i.h: a2sh_nc(short *restrict n, const char *s,
    +@@ lib/atoi/a2i.h: a2sh_nc(short *restrict n, char *s,
      {
        int  status;
      
    @@ lib/atoi/a2i.h: a2sh_nc(short *restrict n, const char *s,
        if (status != 0) {
                errno = status;
                return -1;
    -@@ lib/atoi/a2i.h: a2si_nc(int *restrict n, const char *s,
    +@@ lib/atoi/a2i.h: a2si_nc(int *restrict n, char *s,
      {
        int  status;
      
    @@ lib/atoi/a2i.h: a2si_nc(int *restrict n, const char *s,
        if (status != 0) {
                errno = status;
                return -1;
    -@@ lib/atoi/a2i.h: a2sl_nc(long *restrict n, const char *s,
    +@@ lib/atoi/a2i.h: a2sl_nc(long *restrict n, char *s,
      {
        int  status;
      
    @@ lib/atoi/a2i.h: a2sl_nc(long *restrict n, const char *s,
        if (status != 0) {
                errno = status;
                return -1;
    -@@ lib/atoi/a2i.h: a2sll_nc(long long *restrict n, const char *s,
    +@@ lib/atoi/a2i.h: a2sll_nc(long long *restrict n, char *s,
      {
        int  status;
      
10:  cf609014 = 10:  78104ba6 src/usermod.c: Rename identifiers ralated to [ug]id's
11:  acc77934 = 11:  34d92c06 src/usermod.c: Use id_t for parsing uid_t and gid_t

v4

v4 changes:

• Rebase
• Fix printf of id_t, by adding casts to uintmax_t and using %ju.

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  14ef2653 =  1:  277a0e52 lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  c4ad6a5d =  2:  e097f630 lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  212a171b =  3:  90cdadbf lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  8c0b20ed =  4:  f6ac4bf6 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  2b1b6da9 =  5:  7498a8b6 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  7c20cbef =  6:  ebe7ce37 lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  1c6e2418 =  7:  03852427 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  686d8de8 =  8:  d8047a69 lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  fa5581d1 =  9:  32b0a816 lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  78104ba6 ! 10:  8dc64f71 src/usermod.c: Rename identifiers ralated to [ug]id's
    @@ src/usermod.c: static int get_groups (char *list)
     +static struct id_range
     +getid_range(const char *str)
      {
    --  char                *pos;
    +-  const char          *pos;
     -  unsigned long       first, last;
     -  struct ulong_range  result = { .first = ULONG_MAX, .last = 0 };
    -+  char             *pos;
    ++  const char       *pos;
     +  unsigned long    first, last;
     +  struct id_range  result = { .first = ULONG_MAX, .last = 0 };
      
11:  34d92c06 ! 11:  d432ac27 src/usermod.c: Use id_t for parsing uid_t and gid_t
    @@ Commit message
     
         Use a static_assert(3) to make sure that id_t == uid_t == gid_t.
     
    +    And use uintmax_t to print it, since on Linux they are unsigned types.
    +
         Link: <https://github.com/shadow-maint/shadow/pull/951>
         Cc: Tycho Andersen <tycho@tycho.pizza>
         Cc: Serge Hallyn <serge@hallyn.com>
    @@ src/usermod.c: static int get_groups (char *list)
      static struct id_range
      getid_range(const char *str)
      {
    -   char             *pos;
    ++  id_t             first, last;
    +   const char       *pos;
     -  unsigned long    first, last;
     -  struct id_range  result = { .first = ULONG_MAX, .last = 0 };
    -+  id_t             first, last;
     +  struct id_range  result = {
     +          .first = type_max(id_t),
     +          .last = type_min(id_t)
    @@ src/usermod.c: int main (int argc, char **argv)
     +                  id_t  count = ptr->range.last - ptr->range.first + 1;
     +
                        if (sub_uid_remove(user_name, ptr->range.first, count) == 0) {
    -                           fprintf (stderr,
    -                                   _("%s: failed to remove uid range %lu-%lu from '%s'\n"),
    +-                          fprintf (stderr,
    +-                                  _("%s: failed to remove uid range %lu-%lu from '%s'\n"),
    +-                                  Prog, ptr->range.first, ptr->range.last,
    +-                                  sub_uid_dbname ());
    ++                          fprintf(stderr,
    ++                                  _("%s: failed to remove uid range %ju-%ju from '%s'\n"),
    ++                                  Prog,
    ++                                  (uintmax_t) ptr->range.first,
    ++                                  (uintmax_t) ptr->range.last,
    ++                                  sub_uid_dbname());
    +                           fail_exit (E_SUB_UID_UPDATE);
    +                   }
    +           }
     @@ src/usermod.c: int main (int argc, char **argv)
                struct id_range_list_entry  *ptr;
      
    @@ src/usermod.c: int main (int argc, char **argv)
     +                  id_t  count = ptr->range.last - ptr->range.first + 1;
      
                        if (sub_uid_add(user_name, ptr->range.first, count) == 0) {
    -                           fprintf (stderr,
    +-                          fprintf (stderr,
    +-                                  _("%s: failed to add uid range %lu-%lu to '%s'\n"),
    +-                                  Prog, ptr->range.first, ptr->range.last,
    +-                                  sub_uid_dbname ());
    ++                          fprintf(stderr,
    ++                                  _("%s: failed to add uid range %ju-%ju to '%s'\n"),
    ++                                  Prog,
    ++                                  (uintmax_t) ptr->range.first,
    ++                                  (uintmax_t) ptr->range.last,
    ++                                  sub_uid_dbname());
    +                           fail_exit (E_SUB_UID_UPDATE);
    +                   }
    +           }
     @@ src/usermod.c: int main (int argc, char **argv)
                struct id_range_list_entry  *ptr;
      
    @@ src/usermod.c: int main (int argc, char **argv)
     +                  id_t  count = ptr->range.last - ptr->range.first + 1;
      
                        if (sub_gid_remove(user_name, ptr->range.first, count) == 0) {
    -                           fprintf (stderr,
    +-                          fprintf (stderr,
    +-                                  _("%s: failed to remove gid range %lu-%lu from '%s'\n"),
    +-                                  Prog, ptr->range.first, ptr->range.last,
    +-                                  sub_gid_dbname ());
    ++                          fprintf(stderr,
    ++                                  _("%s: failed to remove gid range %ju-%ju from '%s'\n"),
    ++                                  Prog,
    ++                                  (uintmax_t) ptr->range.first,
    ++                                  (uintmax_t) ptr->range.last,
    ++                                  sub_gid_dbname());
    +                           fail_exit (E_SUB_GID_UPDATE);
    +                   }
    +           }
     @@ src/usermod.c: int main (int argc, char **argv)
                struct id_range_list_entry  *ptr;
      
    @@ src/usermod.c: int main (int argc, char **argv)
     +                  id_t  count = ptr->range.last - ptr->range.first + 1;
      
                        if (sub_gid_add(user_name, ptr->range.first, count) == 0) {
    -                           fprintf (stderr,
    +-                          fprintf (stderr,
    +-                                  _("%s: failed to add gid range %lu-%lu to '%s'\n"),
    +-                                  Prog, ptr->range.first, ptr->range.last,
    +-                                  sub_gid_dbname ());
    ++                          fprintf(stderr,
    ++                                  _("%s: failed to add gid range %ju-%ju to '%s'\n"),
    ++                                  Prog,
    ++                                  (uintmax_t) ptr->range.first,
    ++                                  (uintmax_t) ptr->range.last,
    ++                                  sub_gid_dbname());
    +                           fail_exit (E_SUB_GID_UPDATE);
    +                   }
    +           }

v4b

• Rebase

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  277a0e52 =  1:  776c3711 lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  e097f630 =  2:  3e0ac255 lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  90cdadbf =  3:  f1512ea1 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  f6ac4bf6 =  4:  4a7d0d3d lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  7498a8b6 =  5:  7bc521cc lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  ebe7ce37 =  6:  0618c66e lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  03852427 =  7:  ffb55677 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  d8047a69 =  8:  f9ac734b lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  32b0a816 =  9:  fe80f69a lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  8dc64f71 = 10:  e0a4502c src/usermod.c: Rename identifiers ralated to [ug]id's
11:  d432ac27 = 11:  b531af95 src/usermod.c: Use id_t for parsing uid_t and gid_t

v4c

• Rebase

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  776c3711 =  1:  b8cbe5c8 lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  3e0ac255 =  2:  8ecab761 lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  f1512ea1 =  3:  c51482f1 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  4a7d0d3d =  4:  4f694b04 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  7bc521cc =  5:  c8e2796d lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  0618c66e =  6:  fb31a081 lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  ffb55677 =  7:  edf3b099 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  f9ac734b =  8:  7754af30 lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  fe80f69a =  9:  31560fdd lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  e0a4502c = 10:  a141e80e src/usermod.c: Rename identifiers ralated to [ug]id's
11:  b531af95 = 11:  c3076ea5 src/usermod.c: Use id_t for parsing uid_t and gid_t

v4d

• Rebase

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  b8cbe5c8 =  1:  848aabf9 lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  8ecab761 =  2:  57e44e82 lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  c51482f1 =  3:  d74b29b1 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  4f694b04 =  4:  89d3f3a3 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  c8e2796d =  5:  71d25538 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  fb31a081 =  6:  0a519115 lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  edf3b099 =  7:  539adc20 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  7754af30 =  8:  316f0c9c lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  31560fdd =  9:  5c302c4f lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  a141e80e = 10:  ddf63b19 src/usermod.c: Rename identifiers ralated to [ug]id's
11:  c3076ea5 = 11:  7d933c74 src/usermod.c: Use id_t for parsing uid_t and gid_t

v4e

• Rebase

$ git range-diff gh/getuh..gh/atoi getuh..atoi 
 1:  848aabf9 =  1:  41cb4d0e lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  57e44e82 =  2:  f477b220 lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  d74b29b1 =  3:  0bd955ea lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  89d3f3a3 =  4:  0c323552 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  71d25538 =  5:  473ea791 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  0a519115 =  6:  a16bc9c2 lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  539adc20 =  7:  c09e6b77 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  316f0c9c =  8:  59bdbd2c lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  5c302c4f =  9:  f2512a29 lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  ddf63b19 = 10:  e7045798 src/usermod.c: Rename identifiers ralated to [ug]id's
11:  7d933c74 = 11:  d87c576a src/usermod.c: Use id_t for parsing uid_t and gid_t

v5

• Rebase
• Don't use liba2i's strtoiu.
• Split files (as done in liba2i). This makes the source code significantly more readable.
• Correctly handle a (invalid) base of 1.

$ git range-diff 41cb4d0e^..gh/atoi getuh..atoi 
 1:  41cb4d0e =  1:  fbc841bf lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  f477b220 =  2:  b0aa86fd lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  0bd955ea =  3:  cbed21fe lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  0c323552 =  4:  768c69f2 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  473ea791 =  5:  22a0534f lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  a16bc9c2 =  6:  730d321c lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  c09e6b77 =  7:  a1bfbff5 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  59bdbd2c =  8:  653efe3c lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  f2512a29 <  -:  -------- lib/atoi/: Use libbsd's strto[iu](3bsd) when available
10:  e7045798 =  9:  44fb001d src/usermod.c: Rename identifiers ralated to [ug]id's
11:  d87c576a = 10:  669222c1 src/usermod.c: Use id_t for parsing uid_t and gid_t
 -:  -------- > 11:  c5dbe653 lib/atoi/, *: Split files
 -:  -------- > 12:  4232f065 lib/atoi/strtoi/, tests/: strto[iu]_(): 1 is an invalid base

v5b

• Fix spurious backslash from v5.

$ git range-diff getuh gh/atoi atoi 
 1:  fbc841bf =  1:  fbc841bf lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  b0aa86fd =  2:  b0aa86fd lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  cbed21fe =  3:  cbed21fe lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  768c69f2 =  4:  768c69f2 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  22a0534f =  5:  22a0534f lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  730d321c =  6:  730d321c lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  a1bfbff5 =  7:  a1bfbff5 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  653efe3c =  8:  653efe3c lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  44fb001d =  9:  44fb001d src/usermod.c: Rename identifiers ralated to [ug]id's
10:  669222c1 = 10:  669222c1 src/usermod.c: Use id_t for parsing uid_t and gid_t
11:  c5dbe653 ! 11:  d732c335 lib/atoi/, *: Split files
    @@ lib/atoi/strtoi/strtoi.h (new)
     +
     +  if (base != 0 && (base < 0 || base > 36)) {
     +          *status = EINVAL;
    -+          return MAX(min, MIN(max, 0));                                             \
    ++          return MAX(min, MIN(max, 0));
     +  }
     +
     +  e = errno;
12:  4232f065 ! 12:  617dae99 lib/atoi/strtoi/, tests/: strto[iu]_(): 1 is an invalid base
    @@ lib/atoi/strtoi/strtoi.h: strtoi_(const char *s, char **restrict endp, int base,
     -  if (base != 0 && (base < 0 || base > 36)) {
     +  if (base != 0 && (base < 2 || base > 36)) {
                *status = EINVAL;
    -           return MAX(min, MIN(max, 0));                                             \
    +           return MAX(min, MIN(max, 0));
        }
     
      ## lib/atoi/strtoi/strtou.h ##

v5c

• Update tests/unit/Makefile.am

$ git range-diff getuh gh/atoi atoi 
 1:  fbc841bf =  1:  fbc841bf lib/limits.c: check_logins(): Report LOGIN_ERROR_LOGIN if str2ul() ERANGE
 2:  b0aa86fd =  2:  b0aa86fd lib/idmapping.c: get_map_ranges(): Rename local variable
 3:  cbed21fe =  3:  cbed21fe lib/idmapping.c: get_map_ranges(): Move range check to a2ul() calls
 4:  768c69f2 =  4:  768c69f2 lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 5:  22a0534f =  5:  22a0534f lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call
 6:  730d321c =  6:  730d321c lib/idmapping.c: get_map_ranges(): Remove dead code
 7:  a1bfbff5 =  7:  a1bfbff5 lib/idmapping.c: get_map_ranges(): Simplify iterator variables
 8:  653efe3c =  8:  653efe3c lib/idmapping.c: get_map_ranges(): Don't exit() from a library function
 9:  44fb001d =  9:  44fb001d src/usermod.c: Rename identifiers ralated to [ug]id's
10:  669222c1 = 10:  669222c1 src/usermod.c: Use id_t for parsing uid_t and gid_t
11:  d732c335 ! 11:  91dba81a lib/atoi/, *: Split files
    @@ src/usermod.c
      #include "chkname.h"
      #include "defines.h"
     
    + ## tests/unit/Makefile.am ##
    +@@ tests/unit/Makefile.am: test_adds_LDADD = \
    +     $(NULL)
    + 
    + test_atoi_strtoi_SOURCES = \
    +-    ../../lib/atoi/strtoi.c \
    ++    ../../lib/atoi/strtoi/strtoi.c \
    ++    ../../lib/atoi/strtoi/strtou_noneg.c \
    +     test_atoi_strtoi.c \
    +     $(NULL)
    + test_atoi_strtoi_CFLAGS = \
    +
      ## tests/unit/test_atoi_strtoi.c ##
     @@
      #include <stdint.h>  // Required by <cmocka.h>
12:  617dae99 = 12:  6a5525fe lib/atoi/strtoi/, tests/: strto[iu]_(): 1 is an invalid base

[alejandro-colomar]

@lamby

I was wondering... maybe I could write a library, libgetnum, and add these functions there. That way, all those code bases in Debian that have the problem we found in gettime() could be fixed easily by just calling getnum() internally as I did here.

I also fixed so many other hidden bugs in this PR, that other projects could benefit from such a library.