1492083 - AddressSanitizer: heap-use-after-free [@ JS_GetCompartmentPrincipals] with READ of size 8

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 64.0a1-20180916100118-https://hg.mozilla.org/mozilla-central/rev/7ed950e60f3c1f8a47c117c04124d31e94a66e32.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

The stack here looks a little weird: The alloc/free stacks match and clearly fit together, but I am unsure how the "use" stack relates to them. However, all 3 stacks go through JS/DOM and there might be something wrapped here that has been freed.

Assuming that the stack is ok, the bug could be in nsRFPService. Tom, could you take a look at this and help determine if this looks reasonable? If necessary, we might need someone from DOM to help look into it. Thanks!

Comment 4

[Tom Ritter [:tjr]]

I presume it's possible and correct, and some race condition with my crypto done there. Adding the NSS folks!

Comment 5

[Bobby Holley (:bholley)]

This looks related to Jan's stuff.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #5)
> This looks related to Jan's stuff.

I'll take a closer look tomorrow, but this is JS_GetCompartmentPrincipals and that should be mostly unaffected by my changes. I'm just trying to kill it but it should still work as before.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I am also not at all sure how the "use" stack relates to the other two.  We're remapping wrappers, trying to get a compartment's principals.  But our compartment pointer seems to be pointing into memory allocated by nsCryptoHash?

Do we have any idea of what the testcase here looks like, or whether this is reproducible?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)
> I am also not at all sure how the "use" stack relates to the other two. 
> We're remapping wrappers, trying to get a compartment's principals.  But our
> compartment pointer seems to be pointing into memory allocated by
> nsCryptoHash?

I think this could happen if the "use" involves some wild pointer that just happens to point into a previously freed block of memory. The odds seem astronomically low given a 64-bit address space, but I've seen things like that before.

> Do we have any idea of what the testcase here looks like, or whether this is
> reproducible?

No, this is just from somebody browsing the web with an ASan browser. The person who found the crash is on this bug but they'd have probably posted something if they knew what caused it.

Comment 9

[Jan de Mooij [:jandem]]

I don't know what's going on, but some analysis:

1) The use
----------
jsfriendapi.cpp:171 is this line in JS_GetCompartmentPrincipals:

  MOZ_RELEASE_ASSERT(compartment->realms().length() == 1);

Here we should emit a single load to read the length from the Vector-of-Realms stored in JS::Compartment and apparently that load is where we UAF.

sizeof(JS::Compartment) is 96 in an x64 opt build, that matches the size of the data that was freed by NSS:

  0x12421a999150 is located 48 bytes inside of 96-byte region [0x12421a999120,0x12421a999180)

And indeed the Vector's length field is stored at offset 48 in JS::Compartment AFAICT.

So it looks like we have 2 allocations of 96 bytes, the JS::Compartment + the NSS PK11Context, at the same address. That seems reasonable for a UAF (although note that we use a separate jemalloc arena for SpiderMonkey now, but probably not in ASan builds).

2) The NSS malloc
-----------------
In pk11_CreateNewContextInSlot we malloc sizeof(PK11Context). Looking at PK11ContextStr, 96 bytes probably makes sense (a bunch of wasted space for padding due to mixing bools/pointers).

3) The NSS free
---------------
This is HASH_DESTROY, apparently this line:

    (*context->hashobj->destroy)(context->hash_context, PR_TRUE);

If I'm reading the code correctly, this should be a HASH_AlgSHA256 hash, and our destroy function pointer is then PK11_DestroyContext. This matches 2).

------------

So assuming the NSS code is behaving correctly (it looks like a scoped/short-lived malloc/free), it looks like in 1) we have a JS::Compartment* pointer that's either complete nonsense or it was freed by someone. Probably the latter but it's hard to say more I'm afraid :/

Comment 10

[Jan de Mooij [:jandem]]

Oh and the JS::Compartment* is the key in another compartment's wrapper map. I wonder if we swept a compartment while another compartment still had it in its wrapper map...

Comment 11

[Christian Holler (:decoder)]

Note that it is perfectly possible with ASan to get mismatched use vs. free/alloc stacks.

Here is a possible scenario for this case:

1. Something frees the data in compartment->realms(), ASan marks the memory as freed and retains it.

2. A lot of memory is being used, forcing ASan to return the retained memory back for use (the size of retained memory is controlled by quarantine_size).

3. NSS gets the memory, allocates it, uses it for its purposes, frees it again.

4. compartment->realms() is accessed, leading to the trace we see here.


In this case, there is probably not a lot we can do here, because the alloc/free stacks a mismatched and we would have no clue what the real free location is for compartment->realms() unless we can guess it somehow.

This would be the second case where this happens, and we could increase quarantine_size to make it less likely to happen.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Oh and the JS::Compartment* is the key in another compartment's wrapper map.
> I wonder if we swept a compartment while another compartment still had it in
> its wrapper map...

Jon, do you know if we have known bugs in this area?

It might make sense to expose js::RecomputeWrappers to the JS shell so we can fuzz it. I'll look into that today.

Comment 13

[Jan de Mooij [:jandem]]

Bug 1492406 has a patch to expose js::RecomputeWrappers to the JS shell for fuzzing. I don't expect it to find anything related to this bug, but it can't hurt to try.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #12)
Not that I know of, but it's possible.

Comment 15

[Christian Holler (:decoder)]

Resolving this as incomplete as the stacks are likely mismatched and won't help us further to figure out what was going on at the time of the crash.