[PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 208 bytes --]

It turns out that the previous check did not actually fix the bug.

If we do not include this additional change in the upcoming release, for
consistency's sake, we'd have to allocate another CVE ID.

Florian

[-- Attachment #2: 0001-Improve-check-against-integer-wraparound-in-hcreate_.patch --]
[-- Type: text/x-patch, Size: 3263 bytes --]

2016-01-22  Florian Weimer  <fweimer@redhat.com>

	[BZ #18240]
	* misc/hsearch_r.c (__hcreate_r): Protect against unsigned int
	wraparound.
	* misc/bug18240.c: New test.
	* misc/Makefile (tests): Add it.

diff --git a/misc/Makefile b/misc/Makefile
index b9f854e..d7bbc85 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,7 @@ gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/bug18240.c b/misc/bug18240.c
new file mode 100644
index 0000000..1223486
--- /dev/null
+++ b/misc/bug18240.c
@@ -0,0 +1,69 @@
+/* Test integer wraparound in hcreate.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <limits.h>
+#include <search.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static void
+test_size (size_t size)
+{
+  int res = hcreate (size);
+  if (res == 0)
+    {
+      if (errno == ENOMEM)
+        return;
+      printf ("error: hcreate (%zu): %m\n", size);
+      exit (1);
+    }
+  char *keys[100];
+  for (int i = 0; i < 100; ++i)
+    {
+      if (asprintf (keys + i, "%d", i) < 0)
+        {
+          printf ("error: asprintf: %m\n");
+          exit (1);
+        }
+      ENTRY e = { keys[i], (char *) "value" };
+      if (hsearch (e, ENTER) == NULL)
+        {
+          printf ("error: hsearch (\"%s\"): %m\n", keys[i]);
+          exit (1);
+        }
+    }
+  hdestroy ();
+
+  for (int i = 0; i < 100; ++i)
+    free (keys[i]);
+}
+
+static int
+do_test (void)
+{
+  test_size (-1);
+  test_size (-3);
+  test_size (INT_MAX);
+  test_size (UINT_MAX);
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c
index f6f16ed..8955d85 100644
--- a/misc/hsearch_r.c
+++ b/misc/hsearch_r.c
@@ -71,7 +71,10 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
       return 0;
     }
 
-  if (nel >= SIZE_MAX / sizeof (_ENTRY))
+  /* This limit is sufficient to avoid unsigned wraparound below,
+     possibly after truncation to unsigned int.  (struct hsearch_data
+     is part of the public API and uses usigned ints.)  */
+  if (nel >= INT_MAX / sizeof (_ENTRY))
     {
       __set_errno (ENOMEM);
       return 0;

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Florian Weimer wrote:

> -  if (nel >= SIZE_MAX / sizeof (_ENTRY))
> +  /* This limit is sufficient to avoid unsigned wraparound below,
> +     possibly after truncation to unsigned int.  (struct hsearch_data
> +     is part of the public API and uses usigned ints.)  */
> +  if (nel >= INT_MAX / sizeof (_ENTRY))

This patch doesn't look right. nel should be bounded by UINT_MAX - 2, not by 
INT_MAX / sizeof (anything). (Not by UINT_MAX, since the code computes nel + 1 
later; and not by UINT_MAX - 1 since that cannot be prime.) Furthermore, calloc 
will check for size overflow on multiplication so hcreate_r need not worry about 
dividing by sizeof (anything). Also, "unsigned" is misspelled in the comment.

How about something like the attached (untested) patch instead?

[-- Attachment #2: hsearch.diff --]
[-- Type: text/x-diff, Size: 1042 bytes --]

diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c
index f6f16ed..c258397 100644
--- a/misc/hsearch_r.c
+++ b/misc/hsearch_r.c
@@ -71,13 +71,6 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
       return 0;
     }
 
-  if (nel >= SIZE_MAX / sizeof (_ENTRY))
-    {
-      __set_errno (ENOMEM);
-      return 0;
-    }
-
-
   /* There is still another table active. Return with error. */
   if (htab->table != NULL)
     return 0;
@@ -86,10 +79,19 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
      use will not work.  */
   if (nel < 3)
     nel = 3;
-  /* Change nel to the first prime number not smaller as nel. */
-  nel |= 1;      /* make odd */
-  while (!isprime (nel))
-    nel += 2;
+
+  /* Change nel to the first prime number in the range [nel, UINT_MAX - 2],
+     The '- 2' means 'nel += 2' cannot overflow.  */
+  for (nel |= 1; ; nel += 2)
+    {
+      if (UINT_MAX - 2 < nel)
+	{
+	  __set_errno (ENOMEM);
+	  return 0;
+	}
+      if (isprime (nel))
+	break;
+    }
 
   htab->size = nel;
   htab->filled = 0;

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

* Paul Eggert:

> Florian Weimer wrote:
>
>> -  if (nel >= SIZE_MAX / sizeof (_ENTRY))
>> +  /* This limit is sufficient to avoid unsigned wraparound below,
>> +     possibly after truncation to unsigned int.  (struct hsearch_data
>> +     is part of the public API and uses usigned ints.)  */
>> +  if (nel >= INT_MAX / sizeof (_ENTRY))
>
> This patch doesn't look right. nel should be bounded by UINT_MAX - 2,
> not by INT_MAX / sizeof (anything). (Not by UINT_MAX, since the code
> computes nel + 1 later; and not by UINT_MAX - 1 since that cannot be
> prime.) Furthermore, calloc will check for size overflow on
> multiplication so hcreate_r need not worry about dividing by sizeof
> (anything). Also, "unsigned" is misspelled in the comment.
>
> How about something like the attached (untested) patch instead?

Fair enough.  isprime needs to be fixed as well, like this.

Adhemerval, do we still have time to fix this?

diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c
index 7bc04cf..c73d3ed 100644
--- a/misc/hsearch_r.c
+++ b/misc/hsearch_r.c
@@ -48,7 +48,7 @@ isprime (unsigned int number)
   /* no even number will be passed */
   unsigned int div = 3;
 
-  while (div * div < number && number % div != 0)
+  while (div * (unsigned long long) div < number && number % div != 0)
     div += 2;
 
   return number % div != 0;

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On 01/25/2016 12:09 PM, Florian Weimer wrote:
> -  while (div * div < number && number % div != 0)
> +  while (div * (unsigned long long) div < number && number % div != 0)

Good catch. But better yet, get rid of the '*' so that we needn't worry 
about whether the multiplication overflows. On typical platforms the 
divide instruction that the '%' is already doing will give us the 
information we need, so this is faster anyway. Something like the 
attached (untested) patch.

[-- Attachment #2: hcreate.diff --]
[-- Type: text/x-patch, Size: 1598 bytes --]

diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c
index f6f16ed..1fca6b3 100644
--- a/misc/hsearch_r.c
+++ b/misc/hsearch_r.c
@@ -46,15 +46,12 @@ static int
 isprime (unsigned int number)
 {
   /* no even number will be passed */
-  unsigned int div = 3;
-
-  while (div * div < number && number % div != 0)
-    div += 2;
-
-  return number % div != 0;
+  for (unsigned int div = 3; div <= number / div; div += 2)
+    if (number % div == 0)
+      return 0;
+  return 1;
 }
 
-
 /* Before using the hash table we must allocate memory for it.
    Test for an existing table are done. We allocate one element
    more as the found prime number says. This is done for more effective
@@ -71,13 +68,6 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
       return 0;
     }
 
-  if (nel >= SIZE_MAX / sizeof (_ENTRY))
-    {
-      __set_errno (ENOMEM);
-      return 0;
-    }
-
-
   /* There is still another table active. Return with error. */
   if (htab->table != NULL)
     return 0;
@@ -86,10 +76,19 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
      use will not work.  */
   if (nel < 3)
     nel = 3;
-  /* Change nel to the first prime number not smaller as nel. */
-  nel |= 1;      /* make odd */
-  while (!isprime (nel))
-    nel += 2;
+
+  /* Change nel to the first prime number in the range [nel, UINT_MAX - 2],
+     The '- 2' means 'nel += 2' cannot overflow.  */
+  for (nel |= 1; ; nel += 2)
+    {
+      if (UINT_MAX - 2 < nel)
+	{
+	  __set_errno (ENOMEM);
+	  return 0;
+	}
+      if (isprime (nel))
+	break;
+    }
 
   htab->size = nel;
   htab->filled = 0;

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On 01/26/2016 01:06 AM, Paul Eggert wrote:
> On 01/25/2016 12:09 PM, Florian Weimer wrote:
>> -  while (div * div < number && number % div != 0)
>> +  while (div * (unsigned long long) div < number && number % div != 0)
> 
> Good catch. But better yet, get rid of the '*' so that we needn't worry
> about whether the multiplication overflows. On typical platforms the
> divide instruction that the '%' is already doing will give us the
> information we need, so this is faster anyway. Something like the
> attached (untested) patch.

I looked at the generated assembly on x86-64, and confirm that there's
just one division.  Rest looks good to me, too.

I'm attaching a real patch which also includes the test I wrote (it
still passes, but it really doesn't do anything).

Florian


[-- Attachment #2: 0001-Improve-check-against-integer-wraparound-in-hcreate_.patch --]
[-- Type: text/x-patch, Size: 4550 bytes --]

2016-01-27  Paul Eggert  <eggert@cs.ucla.edu>

	[BZ #18240]
	* misc/hsearch_r.c (isprime, __hcreate_r): Protect against
	unsigned int wraparound.

2016-01-27  Florian Weimer  <fweimer@redhat.com>

	[BZ #18240]
	* misc/bug18240.c: New test.
	* misc/Makefile (tests): Add it.

diff --git a/misc/Makefile b/misc/Makefile
index b9f854e..d7bbc85 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,7 @@ gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/bug18240.c b/misc/bug18240.c
new file mode 100644
index 0000000..4b26865
--- /dev/null
+++ b/misc/bug18240.c
@@ -0,0 +1,75 @@
+/* Test integer wraparound in hcreate.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <limits.h>
+#include <search.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static void
+test_size (size_t size)
+{
+  int res = hcreate (size);
+  if (res == 0)
+    {
+      if (errno == ENOMEM)
+        return;
+      printf ("error: hcreate (%zu): %m\n", size);
+      exit (1);
+    }
+  char *keys[100];
+  for (int i = 0; i < 100; ++i)
+    {
+      if (asprintf (keys + i, "%d", i) < 0)
+        {
+          printf ("error: asprintf: %m\n");
+          exit (1);
+        }
+      ENTRY e = { keys[i], (char *) "value" };
+      if (hsearch (e, ENTER) == NULL)
+        {
+          printf ("error: hsearch (\"%s\"): %m\n", keys[i]);
+          exit (1);
+        }
+    }
+  hdestroy ();
+
+  for (int i = 0; i < 100; ++i)
+    free (keys[i]);
+}
+
+static int
+do_test (void)
+{
+  test_size (500);
+  test_size (-1);
+  test_size (-3);
+  test_size (INT_MAX - 2);
+  test_size (INT_MAX - 1);
+  test_size (INT_MAX);
+  test_size (((unsigned) INT_MAX) + 1);
+  test_size (UINT_MAX - 2);
+  test_size (UINT_MAX - 1);
+  test_size (UINT_MAX);
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c
index f6f16ed..1fca6b3 100644
--- a/misc/hsearch_r.c
+++ b/misc/hsearch_r.c
@@ -46,15 +46,12 @@ static int
 isprime (unsigned int number)
 {
   /* no even number will be passed */
-  unsigned int div = 3;
-
-  while (div * div < number && number % div != 0)
-    div += 2;
-
-  return number % div != 0;
+  for (unsigned int div = 3; div <= number / div; div += 2)
+    if (number % div == 0)
+      return 0;
+  return 1;
 }
 
-
 /* Before using the hash table we must allocate memory for it.
    Test for an existing table are done. We allocate one element
    more as the found prime number says. This is done for more effective
@@ -71,13 +68,6 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
       return 0;
     }
 
-  if (nel >= SIZE_MAX / sizeof (_ENTRY))
-    {
-      __set_errno (ENOMEM);
-      return 0;
-    }
-
-
   /* There is still another table active. Return with error. */
   if (htab->table != NULL)
     return 0;
@@ -86,10 +76,19 @@ __hcreate_r (size_t nel, struct hsearch_data *htab)
      use will not work.  */
   if (nel < 3)
     nel = 3;
-  /* Change nel to the first prime number not smaller as nel. */
-  nel |= 1;      /* make odd */
-  while (!isprime (nel))
-    nel += 2;
+
+  /* Change nel to the first prime number in the range [nel, UINT_MAX - 2],
+     The '- 2' means 'nel += 2' cannot overflow.  */
+  for (nel |= 1; ; nel += 2)
+    {
+      if (UINT_MAX - 2 < nel)
+	{
+	  __set_errno (ENOMEM);
+	  return 0;
+	}
+      if (isprime (nel))
+	break;
+    }
 
   htab->size = nel;
   htab->filled = 0;

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Paul Eggert]

Thanks, that looks good to me.

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

On 01/27/2016 06:25 PM, Paul Eggert wrote:
> Thanks, that looks good to me.

Great.

Adhemerval, may I check this on the master branch?

Thanks,
Florian

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Adhemerval Zanella]

On 28-01-2016 08:35, Florian Weimer wrote:
> On 01/27/2016 06:25 PM, Paul Eggert wrote:
>> Thanks, that looks good to me.
> 
> Great.
> 
> Adhemerval, may I check this on the master branch?

Yes please, thanks.

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Szabolcs Nagy]

On 27/01/16 16:52, Florian Weimer wrote:
> 2016-01-27  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #18240]
> 	* misc/bug18240.c: New test.

this test now times out for me on aarch64 hw

> +  test_size (INT_MAX - 2);

this test case allocates a big buffer (INT_MAX*24)
with calloc and it actually tries to memset it to 0.

when i tried a simple example manually that worked:
calloc did not try to memset 48 GB to 0.

i think the test-skeleton changes malloc behaviour
in weird ways, but i'm not yet sure what's going on.

(i did not notice earlier as in my normal test
environment memory overcommit is off so the
test case just fails with ENOMEM, i'm not sure
how to handle such tests where huge memory is
allocated)

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Andreas Schwab]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:

> i think the test-skeleton changes malloc behaviour
> in weird ways, but i'm not yet sure what's going on.

  mallopt (M_PERTURB, 42);

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Szabolcs Nagy]

On 01/02/16 17:05, Andreas Schwab wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> 
>> i think the test-skeleton changes malloc behaviour
>> in weird ways, but i'm not yet sure what's going on.
> 
>   mallopt (M_PERTURB, 42);
> 

ok, so if the only line in main() is

  calloc(INT_MAX-2, 24);

then it does the memset, but if i do

  calloc (500, 24);
  calloc (INT_MAX-2, 24);

then it does not, however

  mallopt (M_PERTURB, 42);
  calloc (500, 24);
  calloc (INT_MAX-2, 24);

again does the memset (and this is what happens
in the actual test).

given the unreliable calloc behaviour i think
glibc should avoid such tests.

does it affect other 64bit targets?

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

On 02/01/2016 06:05 PM, Andreas Schwab wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> 
>> i think the test-skeleton changes malloc behaviour
>> in weird ways, but i'm not yet sure what's going on.
> 
>   mallopt (M_PERTURB, 42);

Yeah, this invalidates some of the malloc tests as well.

Florian

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Joseph Myers]

On Mon, 1 Feb 2016, Florian Weimer wrote:

> On 02/01/2016 06:05 PM, Andreas Schwab wrote:
> > Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> > 
> >> i think the test-skeleton changes malloc behaviour
> >> in weird ways, but i'm not yet sure what's going on.
> > 
> >   mallopt (M_PERTURB, 42);
> 
> Yeah, this invalidates some of the malloc tests as well.

I suppose we should look back at the bulk conversions to test-skeleton.c, 
to see if any tests were changed for which this is an issue?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Andreas Schwab]

Szabolcs Nagy <szabolcs.nagy@arm.com> writes:

> (i did not notice earlier as in my normal test
> environment memory overcommit is off so the
> test case just fails with ENOMEM, i'm not sure
> how to handle such tests where huge memory is
> allocated)

Perhaps the test should set a VM limit so that the allocation fails even
if you have huge amount of available memory.  Performing the allocation
isn't the main point of the test.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Improve check against integer wraparound in hcreate_r [BZ #18240]

[Florian Weimer]

On 02/01/2016 07:26 PM, Andreas Schwab wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> 
>> (i did not notice earlier as in my normal test
>> environment memory overcommit is off so the
>> test case just fails with ENOMEM, i'm not sure
>> how to handle such tests where huge memory is
>> allocated)
> 
> Perhaps the test should set a VM limit so that the allocation fails even
> if you have huge amount of available memory.  Performing the allocation
> isn't the main point of the test.

Yes, this seems like a reasonable improvement.

I didn't see this because I apparently do not have sufficient amounts of
memory in my test machine.

Florian