Bug 25620 (CVE-2020-6096)

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.)

Joseph, the bug is in memcpy, not malloc.

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.

(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.

(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.

(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

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.

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 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.

(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.

(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.

(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.

(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.

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.)

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)

(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.

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.

Version 2 for review:
https://sourceware.org/pipermail/libc-alpha/2020-April/112747.html

(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

Is this issue confirmed for public disclosure release?

(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.

Thank you confirming

Will libc release notes be updated as well?

(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.

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>

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>

Fixed for glibc 2.32 with:

commit beea361050728138b82c57dda0c4810402d342b9
Author: Alexander Anisimov <a.anisimov@omprussia.ru>
Date:   Wed Jul 8 14:18:31 2020 +0200

    arm: CVE-2020-6096: Fix multiarch memcpy for negative length [BZ #25620]
    
    Unsigned branch instructions could be used for r2 to fix the wrong
    behavior when a negative length is passed to memcpy.
    This commit fixes the armv7 version.

commit 79a4fa341b8a89cb03f84564fd72abaa1a2db394
Author: Evgeny Eremin <e.eremin@omprussia.ru>
Date:   Wed Jul 8 14:18:19 2020 +0200

    arm: CVE-2020-6096: fix memcpy and memmove for negative length [BZ #25620]
    
    Unsigned branch instructions could be used for r2 to fix the wrong
    behavior when a negative length is passed to memcpy and memmove.
    This commit fixes the generic arm implementation of memcpy amd memmove.

The master branch has been updated by Aurelien Jarno <aurel32@sourceware.org>:

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

commit 7b5f02dc2a9278cd068a58a3db3644e24707be49
Author: Aurelien Jarno <aurelien@aurel32.net>
Date:   Mon Jul 13 22:37:41 2020 +0200

    arm: remove string/tst-memmove-overflow XFAIL
    
    The arm string/tst-memmove-overflow XFAIL has been added in commit
    eca1b233322 ("arm: XFAIL string/tst-memmove-overflow due to bug 25620")
    as a way to reproduce the reported bug.
    
    Now that this bug has been fixed in commits 79a4fa341b8 ("arm:
    CVE-2020-6096: fix memcpy and memmove for negative length [BZ #25620]")
    and beea3610507 ("arm: CVE-2020-6096: Fix multiarch memcpy for negative
    length [BZ #25620]"), let's remove the XFAIL.
    
    Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Is this publicly released?

(In reply to regiwils from comment #29)
> Is this publicly released?

The version of glibc 2.32 will contain the fix for CVE-202-6096, and will be released on August 3rd. The current git sources contain a fix for CVE-202-6096 and a regression test.