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?
with Safe-Linking support, the memory block pointer can be resolved by REVEAL_PTR(&p).
(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.
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; } ---
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.
(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.
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 ;-)
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; }
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; > }
(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.
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.
Created attachment 12540 [details] 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.