[Bug malloc/25945] New: memory block return by tcache_get() may contain anather valid memory block pointer, leading to memory leak

[Bug malloc/25945] New: memory block return by tcache_get() may contain anather valid memory block pointer, leading to memory leak

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

            Bug ID: 25945
           Summary: memory block return by tcache_get()  may contain
                    anather valid memory block pointer, leading to memory
                    leak
           Product: glibc
           Version: 2.26
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: malloc
          Assignee: unassigned at sourceware dot org
          Reporter: wangxuszcn at foxmail dot com
  Target Milestone: ---

The malloc function in the GNU C Library (aka glibc or libc6) since 2.26, may
return a memory block which contain another valid memory block pointer,
potentially leading to memory leak. This occurs because the  function
tcache_get() of per-thread cache (aka tcache) feature does not set e->next =
NULL. 

patche canbe:

@@ -2936,6 +3074,7 @@ tcache_get (size_t tc_idx)
   tcache->entries[tc_idx] = e->next;
   --(tcache->counts[tc_idx]);
   e->key = NULL;
+  e->next = NULL;
   return (void *) e;
 }

BTW,can we request a CVE?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] memory block return by tcache_get() may contain anather valid memory block pointer, leading to memory leak

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #1 from wangxu <wangxuszcn at foxmail dot com> ---
with Safe-Linking support, the memory block pointer can be resolved by
REVEAL_PTR(&p).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] memory block return by tcache_get() may contain anather valid memory block pointer, leading to information disclosure

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

wangxu <wangxuszcn at foxmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|memory block return by      |memory block return by
                   |tcache_get()  may contain   |tcache_get()  may contain
                   |anather valid memory block  |anather valid memory block
                   |pointer, leading to memory  |pointer, leading to
                   |leak                        |information disclosure

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] memory block return by tcache_get() may contain anather valid memory block pointer, leading to information disclosure

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #2 from wangxu <wangxuszcn at foxmail dot com> ---
(In reply to wangxu from comment #0)
> The malloc function in the GNU C Library (aka glibc or libc6) since 2.26,
> may return a memory block which contain another valid memory block pointer,
> potentially leading to memory leak. This occurs because the  function
> tcache_get() of per-thread cache (aka tcache) feature does not set e->next =
> NULL. 
> 
> patche canbe:
> 
> @@ -2936,6 +3074,7 @@ tcache_get (size_t tc_idx)
>    tcache->entries[tc_idx] = e->next;
>    --(tcache->counts[tc_idx]);
>    e->key = NULL;
> +  e->next = NULL;
>    return (void *) e;
>  }
> 
> BTW,can we request a CVE?

"memory leak" should be "information disclosure", disclosing another valid
memory block pointer.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] memory block return by tcache_get() may contain anather valid memory block pointer, leading to information disclosure

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #3 from Carlos O'Donell <carlos at redhat dot com> ---
Wangxu,

Thank you very much for the report and the discussion of possible leakage of
the ASLR bits via chunk re-use.

Since we are only clearing an additional pointer-sized word per chunk, and the
chunk is already in cache (previously touched), the cost to change this
additional word is probably not measurable.

However, we need a little more because we want to prevent the disclosure of
ASLR bits in the tcache *and* fastbin chunks.

diff --git a/malloc/malloc.c b/malloc/malloc.c
index ee87ddbbf9..970e4b5e3d 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2953,7 +2953,11 @@ tcache_get (size_t tc_idx)
     malloc_printerr ("malloc(): unaligned tcache chunk detected");
   tcache->entries[tc_idx] = REVEAL_PTR (e->next);
   --(tcache->counts[tc_idx]);
+  /* Clear the key to make free's double-free check effective.  */
   e->key = NULL;
+  /* Clear the Safe-Link'ed pointer to avoid a user from being
+     able to print this and expose ASLR bits.  */
+  e->next = NULL;
   return (void *) e;
 }

@@ -3613,6 +3617,7 @@ _int_malloc (mstate av, size_t bytes)
            *fb = REVEAL_PTR (victim->fd);
          else
            REMOVE_FB (fb, pp, victim);
+
          if (__glibc_likely (victim != NULL))
            {
              size_t victim_idx = fastbin_index (chunksize (victim));
@@ -3646,6 +3651,9 @@ _int_malloc (mstate av, size_t bytes)
                }
 #endif
              void *p = chunk2mem (victim);
+             /* Clear the Safe-Link'ed pointer to avoid a user from being
+                able to print this and expose ASLR bits.  */
+             victim->fd = NULL;
              alloc_perturb (p, bytes);
              return p;
            }
---

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|memory block return by      |ASLR information leak via
                   |tcache_get()  may contain   |Safe-Linking and tcache or
                   |anather valid memory block  |fastbin chunks.
                   |pointer, leading to         |
                   |information disclosure      |

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #4 from Florian Weimer <fweimer at redhat dot com> ---
I'm not sure if this actually eligible for a CVE ID because it is an unreleased
feature.  I've asked here:

<https://www.openwall.com/lists/oss-security/2020/05/08/3>

It's also a bug in a mitigation measure that requires another application bug
in order to matter, and we usually do not treat those bugs as vulnerabilities
in their own right.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #5 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Florian Weimer from comment #4)
> I'm not sure if this actually eligible for a CVE ID because it is an
> unreleased feature.  I've asked here:
> 
> <https://www.openwall.com/lists/oss-security/2020/05/08/3>
> 
> It's also a bug in a mitigation measure that requires another application
> bug in order to matter, and we usually do not treat those bugs as
> vulnerabilities in their own right.

I also appreciate that this is that it does not encourage prompt disclosure.

I will do my best to give positive incentive for prompt disclosure.

The things that are within my control are:

- Recognition with git authorship for patches you created.
  - See https://sourceware.org/glibc/wiki/Contribution%20checklist for how to
submit a patch and specifically the copyright requirements.

- Recognition with "Reported-by:" markup in git commit messages (if you are not
the patch author).

- Recognition with your name in the project NEWS for the release as the
reporter.

Please feel free to suggest any other actions I can take to provide incentive.

Regarding the assignment of the CVE ID, like Florian says, we're gathering
consensus on this for unreleased products.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[dj at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

dj at redhat dot com <dj at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dj at redhat dot com

--- Comment #6 from dj at redhat dot com <dj at redhat dot com> ---
In the grand scheme of things, the big question is, "what can an attacker learn
that the attacker doesn't already know?"  Memory returned by malloc() has
ALWAYS had dirty data in it, and in our case, has always had some form of
pointer in it.  However, if you have the address of the memory itself, you
already know the base address of the heap and the address of the data in each
chunk.  Since the Safe-Linking feature uses the address of the data as the
"randomness", a malicious actor already has everything they need.  Safe-Linking
reduces the ease of using that information, and protects against accidental
corruption, but our implementation in no way hides information.  If we had used
real random bits from the system, I would conclude differently.

I have no objection to clearing internal data before giving memory to the
application, but we must be careful about performance - we don't want to turn
malloc into calloc, but clearing a few words that are likely already in cache
should be fine.  So do we want to clear such data systematically, such as the
proposed patch?  Or in bulk, in malloc() itself, for all chunks?

Note also that these chunks do NOT protect against an attacker calling free()
and using the free'd chunk to access the safe-linking data anyway.

So IMHO this type of patch is a good idea if it protects against accidental
corruption and/or helps us detect such cases (which is what Safe-Linking does),
but if the goal is to protect against malice we should consider a more blanket
solution, one which hides smallbin and largebin pointers as well.

As for the proposed patch, LGTM otherwise ;-)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #7 from wangxu <wangxuszcn at foxmail dot com> ---
Thanks for patient reply.
I'm considering whether there's a more blanket solution. Thanks again.

BTW, is it better to give a change to perturb pointers returned by
tcache_get(), using alloc_perturb (p, bytes) ?

patches may be like this:

diff --git a/malloc/malloc.c b/malloc/malloc.c
index e8abb4e..057e5ad 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3108,7 +3108,9 @@ __libc_malloc (size_t bytes)
       && tcache
       && tcache->counts[tc_idx] > 0)
     {
-      return tcache_get (tc_idx);
+      void *p = tcache_get (tc_idx);
+      alloc_perturb (p, bytes);
+      return p;
     }
   DIAG_POP_NEEDS_COMMENT;
 #endif
@@ -3963,7 +3965,9 @@ _int_malloc (mstate av, size_t bytes)
          && mp_.tcache_unsorted_limit > 0
          && tcache_unsorted_count > mp_.tcache_unsorted_limit)
        {
-     return tcache_get (tc_idx);
+   void *p = tcache_get (tc_idx);
+   alloc_perturb (p, bytes);
+   return p;
        }
 #endif

@@ -3976,7 +3980,9 @@ _int_malloc (mstate av, size_t bytes)
       /* If all the small chunks we found ended up cached, return one now.  */
       if (return_cached)
        {
-     return tcache_get (tc_idx);
+   void *p = tcache_get (tc_idx);
+   alloc_perturb (p, bytes);
+   return p;
        }

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #8 from wangxu <wangxuszcn at foxmail dot com> ---
Just like pointers returned from fastbin, smallbin, largebin.
(In reply to wangxu from comment #7)
> Thanks for patient reply.
> I'm considering whether there's a more blanket solution. Thanks again.
> 
> BTW, is it better to give a change to perturb pointers returned by
> tcache_get(), using alloc_perturb (p, bytes) ?
> 
> patches may be like this:
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index e8abb4e..057e5ad 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3108,7 +3108,9 @@ __libc_malloc (size_t bytes)
>        && tcache
>        && tcache->counts[tc_idx] > 0)
>      {
> -      return tcache_get (tc_idx);
> +      void *p = tcache_get (tc_idx);
> +      alloc_perturb (p, bytes);
> +      return p;
>      }
>    DIAG_POP_NEEDS_COMMENT;
>  #endif
> @@ -3963,7 +3965,9 @@ _int_malloc (mstate av, size_t bytes)
>           && mp_.tcache_unsorted_limit > 0
>           && tcache_unsorted_count > mp_.tcache_unsorted_limit)
>         {
> -     return tcache_get (tc_idx);
> +   void *p = tcache_get (tc_idx);
> +   alloc_perturb (p, bytes);
> +   return p;
>         }
>  #endif
> 
> @@ -3976,7 +3980,9 @@ _int_malloc (mstate av, size_t bytes)
>        /* If all the small chunks we found ended up cached, return one now. 
> */
>        if (return_cached)
>         {
> -     return tcache_get (tc_idx);
> +   void *p = tcache_get (tc_idx);
> +   alloc_perturb (p, bytes);
> +   return p;
>         }

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #9 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to wangxu from comment #7)
> Thanks for patient reply.
> I'm considering whether there's a more blanket solution. Thanks again.
> 
> BTW, is it better to give a change to perturb pointers returned by
> tcache_get(), using alloc_perturb (p, bytes) ?

This is a *distinct* issue. Should M_PERTURB effect tcache? I think it should,
so please file another bug for this.

The issue we are talking about today is the prevention of ASLR bits leaking
from known locations in the chunk metadata. If we can reduce this leakage with
minimal performance loss, because the data is already in the cache and the
write is cheap, then that has value.

I haven't seen any more responses on OSS Security regarding the issue of
allocating a CVE. Our current policy is that "Information disclosure can be
security bugs, especially if exposure through applications can be determined."
and in this case we have no direct exposure through applications that we know
about, so I think allocating a CVE, particularly for an unreleased version of
glibc, is premature.

In which case we should continue working on this as a patch to fix the ASLR
leakage.

Could you please review my comments in
https://sourceware.org/bugzilla/show_bug.cgi?id=25945#c3 and review that we
need to clear the pointers for tcache *and* fastbin?

After such review, please post a patch to libc-alpha for review. It would be
good if you run `make bench` before and after your patch and review the malloc
performance doesn't show any unexpected problems (bench-malloc-simple,
bench-malloc-thread). Likewise confirm no regressions on x86_64.

Thanks.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

wangxu <wangxuszcn at foxmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wangxuszcn at foxmail dot com

--- Comment #10 from wangxu <wangxuszcn at foxmail dot com> ---
Created attachment 12538
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12538&action=edit
patch and bench-malloc-xxx test result

Hi Carlos
  Sorry for late reply.
  I just clear other pointer from tcache and fastbin(without small,large).

1. bench-malloc-simple-[8-64] looks like better performance than before.
2. bench-malloc-simple-[128-4096] and bench-malloc-thread-xx are almost the
same.

Attachment is the results of two rounds testing of bench-malloc-simple and
bench-malloc-thread.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/25945] ASLR information leak via Safe-Linking and tcache or fastbin chunks.

[wangxuszcn at foxmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25945

--- Comment #11 from wangxu <wangxuszcn at foxmail dot com> ---
Created attachment 12540
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12540&action=edit
patch2(tcache,fast,small,large,unsorted) and bench-malloc-xxx test result

(In reply to wangxu from comment #10)
> Created attachment 12538 [details]
> patch and bench-malloc-xxx test result
> 
> Hi Carlos
>   Sorry for late reply.
>   I just clear other pointer from tcache and fastbin(without small,large).
> 
> 1. bench-malloc-simple-[8-64] looks like better performance than before.
> 2. bench-malloc-simple-[128-4096] and bench-malloc-thread-xx are almost the
> same.
> 
> Attachment is the results of two rounds testing of bench-malloc-simple and
> bench-malloc-thread.

Hi Carlos

If clear pointer from tcache,fast-small-large-unsorted bin.
1. bench-malloc-simple-[8-64] look better than before.
2. bench-malloc-simple-[128-1024] are almost the same.
3. bench-malloc-simple-[2048-4096] are slightly bad.
3. bench-malloc-thread-[6-32] need more than about 10% max_rss than before.
Attachment file is "patch2(tcache,fast,small,large,unsorted) and
bench-malloc-xxx test result.zip"

Taking into account the severity of the issue, the performance of the two
patches, I suggest consider only tcache and fastbin.

Thanks for any other suggestions.

-- 
You are receiving this mail because:
You are on the CC list for the bug.