Bug 22375 (CVE-2017-17426) - malloc returns pointer from tcache_get when should return NULL (CVE-2017-17426)
Summary: malloc returns pointer from tcache_get when should return NULL (CVE-2017-17426)
Status: RESOLVED FIXED
Alias: CVE-2017-17426
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: 2.27
Assignee: Arjun Shankar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-31 13:48 UTC by Iain Buclaw
Modified: 2017-12-06 06:58 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2017-10-31 00:00:00
fweimer: security+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Iain Buclaw 2017-10-31 13:48:22 UTC
---
#include <assert.h>
#include <stdlib.h>

int main()
{
  void* x = malloc(10);            // normal allocation
  assert(x != NULL);               // allocation should succeed
  free(x);

  void* z = malloc(((size_t)~0) - 2); // overflow allocation (size_t.max-2)
  assert(z == NULL);                  // allocation should fail
}
---

The second call to malloc returns the cached pointer originally allocated for 'x'.  But should return NULL.
Comment 1 Iain Buclaw 2017-10-31 13:52:34 UTC
Trying out a test program:
---
#include <assert.h>
#include <stdlib.h>
#include <stdio.h>

int main()
{
  void* x = malloc(10);
  assert(x != NULL);
  free(x);

  size_t memsize = ~0;  // overflow allocation (ulong.max)
  printf("Calling malloc(%zu), then decrementing "
         "by one until first found failure.\n", memsize);
  while (1)
    {
      void* z = malloc(memsize);
      if (z == NULL)
        {
          printf("First failed call was malloc(%zu)\n", memsize);
          break;
        }
      free(z);
      memsize--;
    }
}
---

$ ./a.out
Calling malloc(18446744073709551615), then decrementing by one until first found failure.
First failed call was malloc(18446744073709551592)

---

So it looks like malloc doesn't do the right thing if given a value between (size_t)~0-23 and (size_t)~0.
Comment 2 Iain Buclaw 2017-10-31 13:58:47 UTC
And this did not happen with 2.24.

As the last thing that is called before returning is tcache_get(), it seems this is specific to the introduction of a per-thread cache.
Comment 3 Carlos O'Donell 2017-10-31 14:53:18 UTC
Iain,

Thanks for posting this.

Arjun,

Could you look at this and see if we have a missing overflow check? I expect that the large sized allocation overflows the size computation in the tcache and that there might be a window on the high end where the value is being treated as if it was small.
Comment 4 Iain Buclaw 2017-10-31 15:04:11 UTC
Just to satisfy my curiosity, I had a look at the tcache code.

---
#define request2size(req)                                         \
  (((req) + SIZE_SZ + MALLOC_ALIGN_MASK < MINSIZE)  ?             \
   MINSIZE :                                                      \
   ((req) + SIZE_SZ + MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK)

...

#if USE_TCACHE
  /* int_free also calls request2size, be careful to not pad twice.  */
  size_t tbytes = request2size (bytes);

---

SIZE_SZ + MALLOC_ALIGN_MASK would be 23.  Which matches the small test in post 2, and confirms that size_t overflow is happening.
Comment 5 Arjun Shankar 2017-10-31 16:50:54 UTC
This is a regression that appeared in commit d5c3fafc4307c9b7a4c7d5cb381fcdbfad340bcc. It's because of a missing overflow check. This fixes it:

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f94d51c..2ebd97f 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3022,7 +3022,8 @@ __libc_malloc (size_t bytes)
     return (*hook)(bytes, RETURN_ADDRESS (0));
 #if USE_TCACHE
   /* int_free also calls request2size, be careful to not pad twice.  */
-  size_t tbytes = request2size (bytes);
+  size_t tbytes;
+  checked_request2size (bytes, tbytes);
   size_t tc_idx = csize2tidx (tbytes);
 
   MAYBE_INIT_TCACHE ();

I will look at this a bit more before posting the patch.
Comment 6 Arjun Shankar 2017-10-31 16:52:54 UTC
Iain, just noticed your comment. You are of course right. The correct macro is checked_request2size (defined earlier in malloc/malloc.c). Nice find!
Comment 7 Carlos O'Donell 2017-10-31 20:06:46 UTC
(In reply to Arjun Shankar from comment #6)
> Iain, just noticed your comment. You are of course right. The correct macro
> is checked_request2size (defined earlier in malloc/malloc.c). Nice find!

Arjun,

Could you test the change to use checked_request2size?

To test it I think we need to add a test to glibc, I suggest something like this:

* Assume that all architectures are limited to 50-bits of physical memory, and that anything with more than 50-bits of size is going to fail.

* In a loop, loop through all remaining 14-bit values, shifted up to 64-bits, and make sure each one returns ENOMEM. Note that tst-malloc.c has *one* test for this, and we want to cover the whole range of larger than physical memory allocations to make sure we don't have any bugs.

* Do the loop for malloc.

* Do the loop for calloc.

* Do the loop for a small constant malloc, followed by a realloc for the small constant malloc (with the invalid size), and a free (to free the original malloc).

That should cover the major paths. It should also be a fast test to do because it's just checking we covered all the overflow paths.

If it verifies, please submit to libc-alpha for discussion. Thanks!
Comment 8 cvs-commit@gcc.gnu.org 2017-11-30 12:51:18 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  34697694e8a93b325b18f25f7dcded55d6baeaf6 (commit)
      from  18305fba5575a09063652014cfc483b898d8bdcd (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=34697694e8a93b325b18f25f7dcded55d6baeaf6

commit 34697694e8a93b325b18f25f7dcded55d6baeaf6
Author: Arjun Shankar <arjun@redhat.com>
Date:   Thu Nov 30 13:31:45 2017 +0100

    Fix integer overflow in malloc when tcache is enabled [BZ #22375]
    
    When the per-thread cache is enabled, __libc_malloc uses request2size (which
    does not perform an overflow check) to calculate the chunk size from the
    requested allocation size. This leads to an integer overflow causing malloc
    to incorrectly return the last successfully allocated block when called with
    a very large size argument (close to SIZE_MAX).
    
    This commit uses checked_request2size instead, removing the overflow.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog       |    6 ++++++
 malloc/malloc.c |    3 ++-
 2 files changed, 8 insertions(+), 1 deletions(-)
Comment 9 Arjun Shankar 2017-11-30 12:59:33 UTC
I wrote a reasonably thorough regression test for this, but discussions on libc-alpha as well as further testing have revealed that the test when properly expanded (a) will also trigger bug 22343, and (b) has exposed another bug in 32 bit Intel and POWER (related to the relatively larger MALLOC_ALIGNMENT on these machines). Because of this, I haven't checked the regression test into master, but only the fix for now.
Comment 10 cvs-commit@gcc.gnu.org 2017-12-06 06:58:20 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.26/master has been updated
       via  df8c219cb987cfe85c550efa693a1383a11e38aa (commit)
      from  0890d5379cac9b7e2a5f09c3647ebad235c1442d (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=df8c219cb987cfe85c550efa693a1383a11e38aa

commit df8c219cb987cfe85c550efa693a1383a11e38aa
Author: Arjun Shankar <arjun@redhat.com>
Date:   Thu Nov 30 13:31:45 2017 +0100

    Fix integer overflow in malloc when tcache is enabled [BZ #22375]
    
    When the per-thread cache is enabled, __libc_malloc uses request2size (which
    does not perform an overflow check) to calculate the chunk size from the
    requested allocation size. This leads to an integer overflow causing malloc
    to incorrectly return the last successfully allocated block when called with
    a very large size argument (close to SIZE_MAX).
    
    This commit uses checked_request2size instead, removing the overflow.
    
    (cherry picked from commit 34697694e8a93b325b18f25f7dcded55d6baeaf6)

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog       |    7 +++++++
 NEWS            |    6 ++++++
 malloc/malloc.c |    3 ++-
 3 files changed, 15 insertions(+), 1 deletions(-)