1838197 - AddressSanitizer: stack-buffer-underflow with WRITE of size 260840 (changeable) in NSS from_montgomery

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: nss_from_montgomery.patch

When running new RTCPeerConnection() and fill memory to exhaust the process virtual memory, it able to crash with SIGSEGV / SI_KERNEL|0x03445c5f05bd4a5e (address can change) on libfreeblpriv3.so|from_montgomery .

I observe the root cause is when MP_CHECKOK(mp_init(&result)); initialize the variable using mp_init_size then on if ((DIGITS(mp) = s_mp_alloc(prec, sizeof(mp_digit))) == NULL) when s_mp_alloc(prec, sizeof(mp_digit) allocation fail, it then set DIGITS(mp)) to NULL and return MP_MEM.

After that the MP_CHECKOK will goto CLEANUP then execute mp_clear(&tmp); that crashes on SIGSEGV / SI_KERNEL

To trigger the s_mp_alloc failure scenario on AddressSanitizer, I added new modified mp_init function as mp_init_test, so only MP_CHECKOK(mp_init(&result)); use the modified function, it change code from:

if ((DIGITS(mp) = s_mp_alloc(prec, sizeof(mp_digit))) == NULL)
     return MP_MEM;

To following:

DIGITS(mp) = NULL;
return MP_MEM;

The full code change is on attached nss_from_montgomery.patch, after the patch was applied, then compile Firefox with AddressSanitizer, afterwards on Firefox startup it will crash with AddressSanitizer: stack-buffer-underflow with WRITE of size 26xxx

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: asan-stack-buffer-underflow-write-261536.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_from_montgomery1.txt

Comment 3

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_from_montgomery2.txt

Comment 5

[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 6

[Benjamin Beurdouche [:beurdouche]]

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

Comment 7

[John Schanck [:jschanck]]

Now that Bug 1615555 has landed, ecp_256_32.c is all dead code. We could fix it on ESR, but I think it's minor enough to WONTFIX.

Comment 8

[Daniel Veditz [:dveditz]]

Since this is a security bug it's better to track the resolution as "FIXED" even though the patch was not in this bug. If bug 1615555 was specifically fixing this issue we could call it a DUPLICATE, but that isn't appropriate here. It's worth noting that although fixing this bug might be a "by-product" of landing bug 1615555 landing, it's not an "accidental" one: the main impetus for adopting the HACL* versions is correctness

WONTFIX for ESR sounds right. Adopting rewrites is usually out of scope for ESR, and the narrow conditions of this issue make it unlikely to be worth exploiting against the ESR population.