1895032 - Deallocator mismatch in SECITEM_ArenaDupItem()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[mozillabugs]

SECITEM_ArenaDupItem() (security/nss/lib/util/secitem.c) calls the incorrect deallocator on lines 258-61 (below) if lines 253-55 fail to allocate to->data. Lines 258-61 unconditionally call PORT_Free(to), which is incorrect if to was allocated from an arena, as by lines 244-46.

Code from FIREFOX_124_0_2_RELEASE:

235: SECItem *
236: SECITEM_ArenaDupItem(PLArenaPool *arena, const SECItem *from)
237: {
238:     SECItem *to;
239: 
240:     if (from == NULL) {
241:         return (NULL);
242:     }
243: 
244:     if (arena != NULL) {
245:         to = (SECItem *)PORT_ArenaAlloc(arena, sizeof(SECItem));
246:     } else {
247:         to = (SECItem *)PORT_Alloc(sizeof(SECItem));
248:     }
249:     if (to == NULL) {
250:         return (NULL);
251:     }
252: 
253:     if (arena != NULL) {
254:         to->data = (unsigned char *)PORT_ArenaAlloc(arena, from->len);
255:     } else {
256:         to->data = (unsigned char *)PORT_Alloc(from->len);
257:     }
258:     if (to->data == NULL) {
259:         PORT_Free(to);
260:         return (NULL);
261:     }
262: 
263:     to->len = from->len;
264:     to->type = from->type;
265:     if (to->len) {
266:         PORT_Memcpy(to->data, from->data, to->len);
267:     }
268: 
269:     return (to);
270: }

Here is a debugger-based POC:

1. Start FF with -profilemanager and attach a debugger to it.
2. Set a BP on line 254, but disable it.
3. Click "start".
4. Load, e.g., https://www.nytimes.com
5. While it's loading, enable the BP.
6. When the BP fires, record the value in from->len.
7. Now replace from->len with 0x80000000, which guarantees that PORT_ArenaAlloc() will fail.
8. Step line 254.
9. Replace from->len with the original value you recorded in step 6.
10. Step through line 259.
11. Proceed and watch FF crash, possibly in arena_run_reg_dalloc() with a "double free?" RELEASE_ASSERT, and possibly elsewhere with an access violation.

This bug has existed at least since the earliest version of NSS (3.10) on https://ftp.mozilla.org/pub/security/nss/releases/ . Presumably it's an inheritance from Netscape.

Comment 1

[John Schanck [:jschanck]]

SECItem_ArenaDupItem is not used very often (searchfox). But it is used in CERT_DecodeGeneralName which is called from cert_GetCertificateEmailAddresses by way of CERT_DecodeAltNameExtension. So maybe this is the cause of Bug 1748105? I'm surprised that PORT_ArenaAlloc would fail (other than with a safe OOM crash) in Firefox though.

Comment 2

[John Schanck [:jschanck]]

Attachment: Bug 1895032 - remove redundant AllocItem implementation. r=#nss-reviewers

Comment 3

[mozillabugs]

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

...So maybe this is the cause of Bug 1748105?

Please CC me on that bug if you can.

Comment 4

[John Schanck [:jschanck]]

It is possible to trigger this without causing an OOM crash because of this condition in PORT_ArenaAlloc.

That said, I spent some time constructing malformed certificates / certificates with very large directoryName SANs / etc, and I was not able to reproduce Bug 1748105 along this path. So I'm leaning towards this double-free being unrelated to Bug 1748105. I've CC'd you anyway.

Comment 5

[mozillabugs]

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

It is possible to trigger this without causing an OOM crash because of this condition in PORT_ArenaAlloc.

That said, I spent some time constructing malformed certificates / certificates with very large directoryName SANs / etc, and I was not able to reproduce Bug 1748105 along this path. So I'm leaning towards this double-free being unrelated to Bug 1748105. I've CC'd you anyway.

Thanks for the followup. I looked at https://bugzilla.mozilla.org/show_bug.cgi?id=1748105 and have an idea, expanding on your intuition, that might explain it. See that bug for details.

Comment 6

[John Schanck [:jschanck]]

:glandium, do you know whether Firefox will use an infallible allocator here? We're trying to decide how severe this really is.

Comment 7

[Mike Hommey [:glandium] (PTO. Back on Nov 25)]

Firefox only uses infallible allocations for operator new and moz_xmalloc/moz_xcalloc/etc..

Comment 8

[mozillabugs]

But PORT_ArenaAlloc() is fallible, at least for large allocations and apparently for all allocations.

PORT_ArenaAlloc() uses PL_ARENA_ALLOCATE(), which uses PR_MALLOC() if it can't just allocate from an arena. PR_MALLOC() uses malloc(), which calls PageMalloc(), which calls MozJemalloc::malloc(), which calls BaseAllocator::malloc(), which calls arena_t::Malloc(), which calls one of arena_t::MallocSmall(), arena_t::MallocLarge(), or arena_t::MallocHuge(), depending on the requested size. These all look fallible. Tracing arena_t::MallocHuge() eventually results in a call to the Windows API VirtualAlloc(). Faking its return value as NULL eventually causes PORT_ArenaAlloc() to return NULL. You can also emulate this by using the debugger to doctor from->len to some large value on line 254, then iterating that line. Eventually it returns NULL, rather than crashing with an OOM like moz_xmalloc().

This path is similar to the one taken by fallible nsTArray, which uses MozJemallocPHC::realloc(), which calls PageRealloc(), which calls BaseAllocator::realloc(), which calls arena_t::Ralloc() or arena_t::Malloc(), the latter of which joins the flow above.

Comment 9

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/f9b22115dc97be76e388dc9d0dca946dde955e64

Comment 10

[Frederik Braun [:freddy]]

Good find. We're awarding a bounty for this as a sec-moderate. We're happy to revisit that decision if you can come up with a test case that allows to trigger this more reliably.

Comment 11

[Mike Hommey [:glandium] (PTO. Back on Nov 25)]

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

https://hg.mozilla.org/projects/nss/rev/dbd189b826b80eb0ff99d7769e16482624434682

This doesn't seem to be a fix for this issue.

Comment 12

[John Schanck [:jschanck]]

Thanks, updated my earlier comment. The correct patch is
https://hg.mozilla.org/projects/nss/rev/f9b22115dc97be76e388dc9d0dca946dde955e64

Comment 13

[Donal Meehan [:dmeehan]]

Setting esr115 to Fixed for 115.13esr.
Bug 1905160 was uplifted to esr115

Comment 14

[Simon Friedberger [:simonf]]

Attachment: advisory.txt