Bug 25620 (CVE-2020-6096) - Signed comparison vulnerability in the ARMv7 memcpy() (CVE-2020-6096)
Summary: Signed comparison vulnerability in the ARMv7 memcpy() (CVE-2020-6096)
Status: NEW
Alias: CVE-2020-6096
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.3.1
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-02 19:55 UTC by regiwils
Modified: 2020-05-13 14:48 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-03-16 00:00:00
carlos: security+


Attachments
TALOS-2020-1019 security advisory (2.42 KB, text/plain)
2020-03-02 19:55 UTC, regiwils
Details

Note You need to log in before you can comment on or make changes to this bug.
Description regiwils 2020-03-02 19:55:54 UTC
Created attachment 12334 [details]
TALOS-2020-1019  security advisory

### Summary

An exploitable signed comparison vulnerability exists in the ARMv7 memcpy() implementation of GNU glibc. Calling memcpy() (on ARMv7 targets that utilize the GNU glibc implementation) with a negative value for the 'num' parameter results in a signed comparison vulnerability.

If an attacker underflows the 'num' parameter to memcpy(), this vulnerability could lead to undefined behavior such as writing to out-of-bounds memory and potentially remote code execution. Furthermore, this memcpy() implementation allows for program execution to continue in scenarios where a segmentation fault or crash should have occurred. The dangers occur in that subsequent execution and iterations of this code will be executed with this corrupted data.

### Tested Versions

GNU glibc 2.30.9000
Comment 1 joseph@codesourcery.com 2020-03-02 21:57:30 UTC
Note that the glibc 2.30 NEWS file contains the entry:

* Memory allocation functions malloc, calloc, realloc, reallocarray, valloc,
  pvalloc, memalign, and posix_memalign fail now with total object size
  larger than PTRDIFF_MAX.  This is to avoid potential undefined behavior with
  pointer subtraction within the allocated object, where results might
  overflow the ptrdiff_t type.

Thus any size specified with the high bit set exceeds the size of the 
object being copied, so resulting in undefined behavior, so I don't see a 
security vulnerability here in 2.30 and later, unless you're claiming it's 
still valid to allocate over half the address space in a single allocation 
with mmap.  (Of course memcpy is also invalid with overlapping copies, but 
there's a de facto GNU extension to allow the source and destination 
pointers to be equal.)
Comment 2 Yves Younan 2020-03-16 18:59:10 UTC
Joseph, the bug is in memcpy, not malloc.
Comment 3 Carlos O'Donell 2020-03-16 20:19:36 UTC
Has CVE-2020-6096 been assigned to this defect? It shows up the TALOS report attached to this issue. It shows up as reserved by MITRE. Please keep in mind this bug tracker is public.

In general the signed comparison issue looks like it could be used to copy less data than expected, and that's behaviour that could lead to data structures not having their intended value and so effect program control flow. I'm setting security+ because of this issue.
Comment 4 Adhemerval Zanella 2020-03-16 20:32:31 UTC
(In reply to Yves Younan from comment #2)
> Joseph, the bug is in memcpy, not malloc.

My understanding from Joseph's reply is that using a value higher than PTRDIFF_MAX for memcpy length is undefined behavior because either for automatic objects or by using memory allocation functions the allocation will fail. 

However, I still think it is an issue because we explicit omit mmap from the allocation functions that might fail (if I recall correctly the reason was that some programs do try to allocate more than PTRDIFF_MAX with mmap). It will be still subject to the undefined behavior with pointer subtraction when operating with the object itself, so maybe we should extend the same failure handling to mmap.
Comment 5 regiwils 2020-03-26 14:24:48 UTC
(In reply to Carlos O'Donell from comment #3)
> CVE-2020-6096 is assigned to this issue and will reflect as reserved by MITRE until the issue is publicly disclosed and publishing request is issued to MITRE.
Comment 6 Carlos O'Donell 2020-03-28 02:36:50 UTC
(In reply to regiwils from comment #5)
> (In reply to Carlos O'Donell from comment #3)
> > CVE-2020-6096 is assigned to this issue and will reflect as reserved by MITRE until the issue is publicly disclosed and publishing request is issued to MITRE.

This is a public issue tracker.

Please review our security process:
https://sourceware.org/glibc/wiki/Security%20Process
Comment 7 regiwils 2020-03-30 13:08:03 UTC
Hello Carlos,

Please feel free to provide updates for the issue via this email thread since your bug tracker is public.  Also, upon reviewing your security process if you'd like to assign CVE to this issue, we can reject the CVE we have reserved.

Regina Wilson
Analyst.Business Operations
 
regiwils@cisco.com
 
 
 

 

On 3/27/20, 10:36 PM, "carlos at redhat dot com" <sourceware-bugzilla@sourceware.org> wrote:

    https://sourceware.org/bugzilla/show_bug.cgi?id=25620
    
    Carlos O'Donell <carlos at redhat dot com> changed:
    
               What    |Removed                     |Added
    ----------------------------------------------------------------------------
                  Alias|                            |CVE-2020-6096
                Summary|Signed comparison           |Signed comparison
                       |vulnerability in the ARMv7  |vulnerability in the ARMv7
                       |memcpy()                    |memcpy() (CVE-2020-6096)
    
    --- Comment #6 from Carlos O'Donell <carlos at redhat dot com> ---
    (In reply to regiwils from comment #5)
    > (In reply to Carlos O'Donell from comment #3)
    > > CVE-2020-6096 is assigned to this issue and will reflect as reserved by MITRE until the issue is publicly disclosed and publishing request is issued to MITRE.
    
    This is a public issue tracker.
    
    Please review our security process:
    https://sourceware.org/glibc/wiki/Security%20Process
    
    -- 
    You are receiving this mail because:
    You reported the bug.
Comment 8 Richard Earnshaw 2020-04-06 10:27:49 UTC
memcpy is only defined if the regions do not overlap.   If the size of the copy is more than half the address space, this can never be true, so any copy that is mis-interpreted as a negative value must be undefined anyway.
Comment 9 Carlos O'Donell 2020-04-07 13:15:32 UTC
(In reply to Richard Earnshaw from comment #8)
> memcpy is only defined if the regions do not overlap.   If the size of the
> copy is more than half the address space, this can never be true, so any
> copy that is mis-interpreted as a negative value must be undefined anyway.

In many cases the implementation chooses what behaviour happens in the undefined case, and it is always better if we crash early rather than to continue to operate having copied less data than expected by the API. If we change the implementation to operate on unsigned values we will eventually reach an unmapped page (likely) and crash. Crashing is the best outcome in this case since it prevents the attack from continuing.
Comment 10 Wilco 2020-04-07 17:47:40 UTC
(In reply to Carlos O'Donell from comment #9)
> (In reply to Richard Earnshaw from comment #8)
> > memcpy is only defined if the regions do not overlap.   If the size of the
> > copy is more than half the address space, this can never be true, so any
> > copy that is mis-interpreted as a negative value must be undefined anyway.
> 
> In many cases the implementation chooses what behaviour happens in the
> undefined case, and it is always better if we crash early rather than to
> continue to operate having copied less data than expected by the API. If we
> change the implementation to operate on unsigned values we will eventually
> reach an unmapped page (likely) and crash. Crashing is the best outcome in
> this case since it prevents the attack from continuing.

Yes it's better to crash, but I fail to see how it would be a security issue. Assuming we agree values over 2GB are not used in legal programs, then it could only happen if the size gets corrupted by a buffer overflow. If that's the case, an attacker could set it to whatever value they wanted anyway rather than relying on this bug.
Comment 11 Florian Weimer 2020-04-07 18:07:14 UTC
(In reply to Wilco from comment #10)
> Yes it's better to crash, but I fail to see how it would be a security
> issue. Assuming we agree values over 2GB are not used in legal programs,
> then it could only happen if the size gets corrupted by a buffer overflow.
> If that's the case, an attacker could set it to whatever value they wanted
> anyway rather than relying on this bug.

I think the relevant case is when the size is set to -1 (or something else close to zero) statically due to a size computation that went wrong as the result of a logic error, and not some arbitrary attacker-controlled value.

Maybe we can change __memcpy_chk to call __chk_fail if the most significant bit of the dynamic length argument is set, independently of the value of the destination length? Then at least applications built with -D_FORTIFY_SOURCE would benefit from that kind of belt-and-suspenders hardening.
Comment 12 Wilco 2020-04-07 18:50:37 UTC
(In reply to Florian Weimer from comment #11)
> (In reply to Wilco from comment #10)
> > Yes it's better to crash, but I fail to see how it would be a security
> > issue. Assuming we agree values over 2GB are not used in legal programs,
> > then it could only happen if the size gets corrupted by a buffer overflow.
> > If that's the case, an attacker could set it to whatever value they wanted
> > anyway rather than relying on this bug.
> 
> I think the relevant case is when the size is set to -1 (or something else
> close to zero) statically due to a size computation that went wrong as the
> result of a logic error, and not some arbitrary attacker-controlled value.

Compilers warn if memcpy size is constant and over 2GB, so it's unlikely such a glaring mistake would go unnoticed. It would need to be runtime underflow which means you'd likely call malloc to allocate the buffer, and that would fail on -1.

> Maybe we can change __memcpy_chk to call __chk_fail if the most significant
> bit of the dynamic length argument is set, independently of the value of the
> destination length? Then at least applications built with -D_FORTIFY_SOURCE
> would benefit from that kind of belt-and-suspenders hardening.

Yes, and it would be even better to check for overlap, that includes this case and would find quite a few real bugs.
Comment 13 Carlos O'Donell 2020-04-07 20:49:03 UTC
(In reply to Wilco from comment #10)
> (In reply to Carlos O'Donell from comment #9)
> > (In reply to Richard Earnshaw from comment #8)
> > > memcpy is only defined if the regions do not overlap.   If the size of the
> > > copy is more than half the address space, this can never be true, so any
> > > copy that is mis-interpreted as a negative value must be undefined anyway.
> > 
> > In many cases the implementation chooses what behaviour happens in the
> > undefined case, and it is always better if we crash early rather than to
> > continue to operate having copied less data than expected by the API. If we
> > change the implementation to operate on unsigned values we will eventually
> > reach an unmapped page (likely) and crash. Crashing is the best outcome in
> > this case since it prevents the attack from continuing.
> 
> Yes it's better to crash, but I fail to see how it would be a security
> issue. Assuming we agree values over 2GB are not used in legal programs,
> then it could only happen if the size gets corrupted by a buffer overflow.
> If that's the case, an attacker could set it to whatever value they wanted
> anyway rather than relying on this bug.

We assign a security+ in all issues where the reporter filed a CVE (glibc security policy). The security+ indicates the issue is considered "security relevant" and needs further evaluation (we have 17 such glibc bugs open for review today).

The fact that we have a CVE assigned means Red Hat's security had to work through their own analysis here:
https://access.redhat.com/security/cve/CVE-2020-6096#cve-cvss-v3

In the case of low or moderate impact we really need to have other factors to decide to prioritize.

I think we all agree this should crash, and if we can make it crash then that's a QoI improvement.

What is the priority of this fix over other stuff you're working on? That's hard to say, but I would not prioritize this against other bugs and features, and not against the set of "high" or "critical" CVEs since our security teams have evaluated it lower.
Comment 14 joseph@codesourcery.com 2020-04-07 20:58:48 UTC
On Tue, 7 Apr 2020, wdijkstr at arm dot com via Glibc-bugs wrote:

> Yes, and it would be even better to check for overlap, that includes this case
> and would find quite a few real bugs.

I think a de facto part of the ABI is that memcpy of an object to itself 
(source and destination pointers equal) does work; some GCC versions may 
generate that for structure assignment in some cases where it's not know 
if there might be such exact overlap but partial overlap would not be 
valid (which is the standard C rule for structure assignment).  (GCC won't 
let you declare a type occupying half the address space or more, so the 
structure assignment case isn't applicable for such a large memcpy.)
Comment 15 Szabolcs Nagy 2020-04-08 13:34:12 UTC
note that memmove calls memcpy with overlapping
src and dst (if dst < src).

and on an aarch64 kernel a 32bit arm executable with
mmap(0, -1U/2 + 4097, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, -1, 0);
can succeed.

so the bug can be triggered by code using mmap and memmove correctly.

however i think relying on memcpy crashing on an overflowed
size to mitigate bugs in user code is a weak protection.
(and in general we should not guarantee that memcpy accesses
all pages in the specified range: there may be a memcpy
syscall in the future for large copies with whatever semantics
in case of ub)
Comment 16 Wilco 2020-04-08 14:09:27 UTC
(In reply to Szabolcs Nagy from comment #15)
> note that memmove calls memcpy with overlapping
> src and dst (if dst < src).
> 
> and on an aarch64 kernel a 32bit arm executable with
> mmap(0, -1U/2 + 4097, PROT_READ|PROT_WRITE, MAP_ANON|MAP_PRIVATE, -1, 0);
> can succeed.
> 
> so the bug can be triggered by code using mmap and memmove correctly.
> 
> however i think relying on memcpy crashing on an overflowed
> size to mitigate bugs in user code is a weak protection.
> (and in general we should not guarantee that memcpy accesses
> all pages in the specified range: there may be a memcpy
> syscall in the future for large copies with whatever semantics
> in case of ub)

Still we should have a new test that checks every string function which has a size_t input, using a positive test if the huge mmap succeeds and a negative test checking for a crash. That should flush out any other cases across all targets.
Comment 17 Carlos O'Donell 2020-04-21 14:35:34 UTC
Patch posted by zhuyan (Huawei):
https://sourceware.org/pipermail/libc-alpha/2020-April/112671.html

There is discussion about how much test coverage is needed.
Comment 18 Carlos O'Donell 2020-04-21 14:41:27 UTC
Version 2 for review:
https://sourceware.org/pipermail/libc-alpha/2020-April/112747.html
Comment 19 Wilco 2020-05-01 13:03:20 UTC
(In reply to Carlos O'Donell from comment #17)
> Patch posted by zhuyan (Huawei):
> https://sourceware.org/pipermail/libc-alpha/2020-April/112671.html
> 
> There is discussion about how much test coverage is needed.

Enough to be 100% sure the issue is really fixed. See  https://sourceware.org/pipermail/libc-alpha/2020-May/113521.html
Comment 20 regiwils 2020-05-01 13:28:39 UTC
Is this issue confirmed for public disclosure release?
Comment 21 Carlos O'Donell 2020-05-01 13:35:26 UTC
(In reply to regiwils from comment #20)
> Is this issue confirmed for public disclosure release?

I'm not sure what you mean by this question.

This is a public bug tracker for glibc.

The CVE is public in the MITRE CVE database:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-6096

There is a public bug for this within Red Hat's tracker:
https://access.redhat.com/security/cve/cve-2020-6096
https://bugzilla.redhat.com/show_bug.cgi?id=1820331

The CVE is thus already publicly disclosed.
Comment 22 regiwils 2020-05-01 13:42:09 UTC
Thank you confirming
Comment 23 regiwils 2020-05-01 15:51:25 UTC
Will libc release notes be updated as well?
Comment 24 Carlos O'Donell 2020-05-01 20:28:03 UTC
(In reply to regiwils from comment #23)
> Will libc release notes be updated as well?

When a fix is committed there will be a release note added for CVE-2020-6096 in the master branch top-level "NEWS" file under the "Security related changes:" section.

The NEWS file information will go out with the release announcement email for the release. The next time-boxed release is planned for August 1st, 2020 and will be glibc 2.32.

As of today there is no committed fix for CVE-2020-6096.
Comment 25 cvs-commit@gcc.gnu.org 2020-05-12 17:03:21 UTC
The master branch has been updated by Florian Weimer <fw@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=eec0f4218cda936a6ab8f543e90b96b196df3fc2

commit eec0f4218cda936a6ab8f543e90b96b196df3fc2
Author: Florian Weimer <fweimer@redhat.com>
Date:   Tue May 12 19:02:08 2020 +0200

    string: Add string/tst-memmove-overflow, a test case for bug 25620
    
    Reviewed-by: Carlos O'Donell <carlos@redhat.com>
    Tested-by: Carlos O'Donell <carlos@redhat.com>
Comment 26 cvs-commit@gcc.gnu.org 2020-05-13 14:48:46 UTC
The master branch has been updated by Florian Weimer <fw@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=eca1b233322914d9013f3ee4aabecaadc9245abd

commit eca1b233322914d9013f3ee4aabecaadc9245abd
Author: Florian Weimer <fweimer@redhat.com>
Date:   Wed May 13 16:45:29 2020 +0200

    arm: XFAIL string/tst-memmove-overflow due to bug 25620
    
    Also reduce the amount of output in case of a large-scale mismatch in
    the copied data.
    
    Reviewed-by: Carlos O'Donell <carlos@redhat.com>