Reject negative numbers in strtoul(3)

alejandro-colomar commented 8 months ago

!! This may break some users (already broken users, that is).

We'll need at least a release note.

alejandro-colomar commented 8 months ago

Queued after https://github.com/shadow-maint/shadow/pull/863 (done)

alejandro-colomar commented 8 months ago

v2 changes:

Rebase to master.

$ git range-diff master..gh/strtou shadow/master..strtou
1:  6ac96ba4 = 1:  fe41ee9a lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
2:  8423d2e2 ! 2:  4448e24d lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
    @@ lib/getulong.c
      #include "prototypes.h"

    -@@ lib/getulong.c: getulong(const char *numstr, /*@out@*/unsigned long *result)
    +@@ lib/getulong.c: getulong(const char *restrict numstr, unsigned long *restrict result)
        unsigned long  val;

        errno = 0;
3:  1c097bac = 3:  6735732d tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar commented 8 months ago

v3 changes:

Use ATTR_ACCESS()

$ git range-diff shadow/master gh/strtou strtou
1:  fe41ee9a ! 1:  e3afc5e9 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ lib/atoi/strtou_noneg.h (new)
     +#include "attr.h"
     +
     +
    -+ATTR_STRING(1)
    ++ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
     +inline unsigned long strtoul_noneg(const char *nptr,
     +    char **restrict endptr, int base);
    -+ATTR_STRING(1)
    ++ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
     +inline unsigned long long strtoull_noneg(const char *nptr,
     +    char **restrict endptr, int base);
     +
2:  4448e24d = 2:  15f03a7c lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  6735732d = 3:  bdd8f033 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar commented 8 months ago

v3b changes:

Rebase to master

$ git range-diff master..gh/strtou shadow/master..strtou
1:  e3afc5e9 = 1:  76405163 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
2:  15f03a7c = 2:  11415e3f lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  bdd8f033 = 3:  5fcbea27 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar commented 8 months ago

v4 changes:

Drop tests for strtoul(3) and strtoull(3). [@ikerexxe ]

$ git range-diff shadow/master gh/strtou strtou
1:  e3afc5e9 ! 1:  e8a48296 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
@@ Commit message
     These functions reject negative numbers, instead of silently converting
     them into unsigned, which strtou[l]l(3) do.

+    Cc: Iker Pedrosa <ipedrosa@redhat.com>
+    Cc: "Serge E. Hallyn" <serge@hallyn.com>
     Signed-off-by: Alejandro Colomar <alx@kernel.org>

  ## lib/Makefile.am ##
2:  15f03a7c ! 2:  8211b28f lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
@@ Commit message
     the smallest possible value, 0, and set errno to ERANGE to report an
     error.

+    Cc: Iker Pedrosa <ipedrosa@redhat.com>
+    Cc: "Serge E. Hallyn" <serge@hallyn.com>
     Signed-off-by: Alejandro Colomar <alx@kernel.org>

  ## lib/getrange.c ##
3:  bdd8f033 ! 3:  183d3322 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
@@ Metadata
  ## Commit message ##
     tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

+    Cc: Iker Pedrosa <ipedrosa@redhat.com>
     Signed-off-by: Alejandro Colomar <alx@kernel.org>

  ## tests/unit/Makefile.am ##
@@ tests/unit/Makefile.am: test_adds_LDADD = \

  ## tests/unit/test_atoi_strtou_noneg.c (new) ##
 @@
-+// SPDX-FileCopyrightText: 2023, Alejandro Colomar <alx@kernel.org>
++// SPDX-FileCopyrightText: 2023-2024, Alejandro Colomar <alx@kernel.org>
 +// SPDX-License-Identifier: BSD-3-Clause
 +
 +
@@ tests/unit/test_atoi_strtou_noneg.c (new)
 +#include "atoi/strtou_noneg.h"
 +
 +
-+static void test_strtoul(void **state);
 +static void test_strtoul_noneg(void **state);
-+static void test_strtoull(void **state);
 +static void test_strtoull_noneg(void **state);
 +
 +
@@ tests/unit/test_atoi_strtou_noneg.c (new)
 +main(void)
 +{
 +    const struct CMUnitTest  tests[] = {
-+        cmocka_unit_test(test_strtoul),
 +        cmocka_unit_test(test_strtoul_noneg),
-+        cmocka_unit_test(test_strtoull),
 +        cmocka_unit_test(test_strtoull_noneg),
 +    };
 +
@@ tests/unit/test_atoi_strtou_noneg.c (new)
 +
 +
 +static void
-+test_strtoul(void **state)
-+{
-+  errno = 0;
-+  assert_true(strtoul("42", NULL, 0) == 42);
-+  assert_true(errno == 0);
-+
-+  assert_true(strtoul("-1", NULL, 0) == -1ul);
-+  assert_true(errno == 0);
-+  assert_true(strtoul("-3", NULL, 0) == -3ul);
-+  assert_true(errno == 0);
-+  switch (sizeof(long)) {
-+  case 8:
-+          assert_true(strtoul("-0xFFFFFFFFFFFFFFFF", NULL, 0) == 1);
-+          break;
-+  case 4:
-+          assert_true(strtoul("-0xFFFFFFFF", NULL, 0) == 1);
-+          break;
-+  }
-+  assert_true(errno == 0);
-+
-+  assert_true(strtoul("-0x10000000000000000", NULL, 0) == ULONG_MAX);
-+  assert_true(errno == ERANGE);
-+}
-+
-+
-+static void
 +test_strtoul_noneg(void **state)
 +{
 +  errno = 0;
@@ tests/unit/test_atoi_strtou_noneg.c (new)
 +
 +
 +static void
-+test_strtoull(void **state)
-+{
-+  errno = 0;
-+  assert_true(strtoull("42", NULL, 0) == 42);
-+  assert_true(errno == 0);
-+
-+  assert_true(strtoull("-1", NULL, 0) == -1ul);
-+  assert_true(errno == 0);
-+  assert_true(strtoull("-3", NULL, 0) == -3ul);
-+  assert_true(errno == 0);
-+  assert_true(strtoull("-0xFFFFFFFFFFFFFFFF", NULL, 0) == 1);
-+  assert_true(errno == 0);
-+
-+  assert_true(strtoull("-0x10000000000000000", NULL, 0) == ULONG_MAX);
-+  assert_true(errno == ERANGE);
-+}
-+
-+
-+static void
 +test_strtoull_noneg(void **state)
 +{
 +  errno = 0;

alejandro-colomar commented 8 months ago

v4b changes:

Rename function parameters

$ git range-diff shadow/master gh/strtou strtou
1:  e8a48296 ! 1:  fda49d88 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ lib/atoi/strtou_noneg.c (new)
     +#include "atoi/strtou_noneg.h"
     +
     +
    -+extern inline unsigned long strtoul_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    -+extern inline unsigned long long strtoull_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++extern inline unsigned long strtoul_noneg(const char *s,
    ++    char **restrict endp, int base);
    ++extern inline unsigned long long strtoull_noneg(const char *s,
    ++    char **restrict endp, int base);

      ## lib/atoi/strtou_noneg.h (new) ##
     @@
    @@ lib/atoi/strtou_noneg.h (new)
     +
     +
     +ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
    -+inline unsigned long strtoul_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++inline unsigned long strtoul_noneg(const char *s,
    ++    char **restrict endp, int base);
     +ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
    -+inline unsigned long long strtoull_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++inline unsigned long long strtoull_noneg(const char *s,
    ++    char **restrict endp, int base);
     +
     +
     +inline unsigned long
    -+strtoul_noneg(const char *nptr, char **restrict endptr, int base)
    ++strtoul_noneg(const char *s, char **restrict endp, int base)
     +{
    -+  if (strtol(nptr, endptr, base) < 0) {
    ++  if (strtol(s, endp, base) < 0) {
     +          errno = ERANGE;
     +          return 0;
     +  }
    -+  return strtoul(nptr, endptr, base);
    ++  return strtoul(s, endp, base);
     +}
     +
     +
     +inline unsigned long long
    -+strtoull_noneg(const char *nptr, char **restrict endptr, int base)
    ++strtoull_noneg(const char *s, char **restrict endp, int base)
     +{
    -+  if (strtol(nptr, endptr, base) < 0) {
    ++  if (strtol(s, endp, base) < 0) {
     +          errno = ERANGE;
     +          return 0;
     +  }
    -+  return strtoull(nptr, endptr, base);
    ++  return strtoull(s, endp, base);
     +}
     +
     +
2:  8211b28f = 2:  6cb0d537 lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  183d3322 = 3:  8873245d tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar commented 8 months ago

v4c changes:

Add Reviewed-by

$ git range-diff shadow/master gh/strtou strtou
1:  fda49d88 ! 1:  40fe823a lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ Commit message
         These functions reject negative numbers, instead of silently converting
         them into unsigned, which strtou[l]l(3) do.

    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

2:  6cb0d537 ! 2:  f586e354 lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
    @@ Commit message
         the smallest possible value, 0, and set errno to ERANGE to report an
         error.

    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

3:  8873245d ! 3:  2fa4837e tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
    @@ Metadata
      ## Commit message ##
         tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## tests/unit/Makefile.am ##

alejandro-colomar commented 8 months ago

!! This may break some users (already broken users, that is).

We'll need at least a release note.

And I'll remind of this before merging.

hallyn commented 7 months ago

Thanks, looks good

hallyn commented 7 months ago

!! This may break some users (already broken users, that is). We'll need at least a release note.

And I'll remind of this before merging.

Is there any case here worth warning about? Certainly not in parsing id mappings.

alejandro-colomar commented 7 months ago

!! This may break some users (already broken users, that is). We'll need at least a release note.

And I'll remind of this before merging.

Is there any case here worth warning about? Certainly not in parsing id mappings.

I'm not sure. :\

Maybe I would just say in the release notes that we're now more strict in what (numeric) input is valid. That would put on alert those who have broken files.

shadow-maint / shadow

Reject negative numbers in strtoul(3) #875