1839258 - (CVE-2024-6609) AddressSanitizer: global-buffer-overflow on NSS PORT_FreeArena_Util -> PL_ClearArenaPool after calloc and malloc failure

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: asan-global-buffer-overflow-read-8.txt

When running new RTCPeerConnection() and fillMemory using JS code to exhaust the process virtual memory, on Firefox compiled with AddressSanitizer, I able to hit AddressSanitizer: global-buffer-overflow, sometimes it hit SEGV on unknown address, and sometimes "fault was caused by a dereference of a high value address" is included, it hit on NSS PORT_FreeArena_Util -> PL_ClearArenaPool, this one is quite rare to trigger on ASan, compared to Bug 1838197 is very common.

I've investigated for a days, it caused after calloc failure on ec_NewKey at mp_init(&k), the ec_NewKey is returning ecPriv value from calloc failure, then malloc failure on SEC_ASN1EncodeItem is triggered goto ecgn_done; which freed up the ecPriv on PORT_FreeArena(ecPriv->ecParams.arena, PR_TRUE);

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:beurdouche, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Benjamin Beurdouche [:beurdouche]]

John, can you please check priority and severity on this when you have a min ? Thanks !

Comment 3

[John Schanck [:jschanck]]

This will be fixed when we finish upgrading the P-384 and P-521 code. I assume RTCPeerConnection() uses P-256, and that case is already fixed.

Comment 4

[John Schanck [:jschanck]]

Fixed by Bug 1854438 and Bug 1854439 in NSS 101.

Comment 5

[Daniel Veditz [:dveditz]]

Our bounty guidelines for "Memory Corruption triggered by an OOM condition" is $1500
https://www.mozilla.org/en-US/security/client-bug-bounty/

Comment 6

[Simon Friedberger [:simonf]]

Attachment: advisory.txt

Comment 7

[Mike Hommey [:glandium]]

Are you sure this was addressed by bug 1854438 and bug 1854439? I was looking at the issue as reported in comment 0 for potential backports to older versions of NSS, and the code issue that PORT_FreeArena(ecPriv->ecParams.arena, PR_TRUE); is called on an potentially already free'd arena is still there. It might not be triggerable through OOM allocation failures, but can we be absolutely sure that no other code path present or future might lead to that? As a matter of defense in depth, it would seem like we should at least add a if (key) key->ecParams.arena = NULL to ec_NewKey's cleanup.

Comment 8

[John Schanck [:jschanck]]

The core issue was that ec_NewKey was using CHECK_MPI_OK after CHECK_SEC_OK without resetting rv. So if mp_init failed here, the function would return the value rv = SECSuccess that was set here without assigning a value to *privKey.

You are right that the two bugs I linked above did not actually fix the issue. Those were prerequisites for a larger cleanup effort that I did in Bug 1898627, which landed simultaneously.

Comment 9

[Mike Hommey [:glandium]]

(In reply to John Schanck [:jschanck] from comment #8)

The core issue was that ec_NewKey was using CHECK_MPI_OK after CHECK_SEC_OK without resetting rv. So if mp_init failed here, the function would return the value rv = SECSuccess that was set here without assigning a value to *privKey.

You are right that the two bugs I linked above did not actually fix the issue. Those were prerequisites for a larger cleanup effort that I did in Bug 1898627, which landed simultaneously.

But this code path is on the failure case? https://searchfox.org/mozilla-central/rev/0c55d51c0d2a9b672e42ad40ea54f90267f92a8e/security/nss/lib/softoken/pkcs11c.c#5874 and the arena is always freed on cleanup ?

Comment 10

[John Schanck [:jschanck]]

We don't goto ecgn_done on EC_NewKey failure https://searchfox.org/mozilla-central/rev/02a4a649ed75ebaf3fbdf301c3d3137baf6842a1/security/nss/lib/softoken/pkcs11c.c#5834-5842.

Comment 11

[Mike Hommey [:glandium]]

Oh, I see. So a fix for older versions would be to apply the following to the cleanup code path in ec_NewKey, right? https://searchfox.org/nss/rev/ba9330537e6e94971de8b9bc49460891b23afd4f/lib/freebl/ec.c#379-382

Comment 12

[John Schanck [:jschanck]]

Yes, that would work.

Comment 13

[Mike Hommey [:glandium]]

Is it ok to attach the information in comment 11 to public information about the CVE? (I think that would end up on https://security-tracker.debian.org/tracker/CVE-2024-6609)

Comment 14

[John Schanck [:jschanck]]

Fine by me.