1514762 - AddressSanitizer: unknown-crash [@ Length] with READ of size 8 in CC through [@ nsObserverList::AppendStrongObservers]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 65.0a1-20181208094804-https://hg.mozilla.org/mozilla-central/rev/336f58aeb663c01ede2a646d51d5015bf741538d.

For detailed crash information, see attachment.

This is a regular crash but :mccr8 mentioned that it looks really weird and that it would be worth investigating further because we haven't seen this type of crash in our regular crash-stats before. It might well be that the bug will end up not being actionable because we neither have steps to reproduce nor additional stacks that might help.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

nsGlobalWindowOuter::Create calling nsObserverService::AddObserver hints that this could be something about bug 1498370.

Comment 3

[(no longer active)]

Thanks, will take a look.

Comment 4

[(no longer active)]

Hmm, I don't really understand this assertion.  I don't remember ever seeing an ASAN "unknown-crash" before.  Andrew can you read this crash report and understand it by any chance?

Comment 5

[Andrew McCreight [:mccr8]]

I'm not really sure. When I was looking at this before, I was thinking that maybe it was a wild pointer, but it does seem to recognize it as a pointer within a hash table, so that can't be right.

Maybe we ended up calling nsObserverService::AddObserver() while we were already modifying the observer service hash table, so it got corrupted somehow? The allocation stack ends at JITted code so it is hard to say.

Comment 6

[(no longer active)]

Hmm, yeah that's a good theory.  I can write a patch based on that.

Comment 7

[(no longer active)]

Attachment: Bug 1514762 - Delay modifying the observer list until we hit the event loop;

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

This is indeed really odd. calloc happens on hashtable, crash on an array. But I guess hashtable could pass some broken array back and we operate on such later.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #8)
> This is indeed really odd. calloc happens on hashtable, crash on an array.
> But I guess hashtable could pass some broken array back and we operate on
> such later.

It is a hashtable of arrays, so I think the array is inlined into an entry in the hashtable.

It isn't entirely clear what is going on here, but it feels like something that could be akin to a UAF.

Comment 10

[(no longer active)]

Comment on attachment 9032076 [details]
Bug 1514762 - Delay modifying the observer list until we hit the event loop;

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Unclear.  We aren't sure what security bug we are fixing yet.  The patch tells the potential attacker that we may be modifying the observer table in the middle of nsGlobalWindowOuter::Create(), and calling nsGlobalWindowOuter::Create() is certainly something that the web page can control, but it's unclear how it can control doing so within a period of time where we're modifying the observer table.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: beta and central

If not all supported branches, which bug introduced the flaw?: Bug 1498370

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: It should be quite safe to backport, I am not concerned with the patch causing regressions.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9032076 [details]
Bug 1514762 - Delay modifying the observer list until we hit the event loop;

Sec-approval+ for trunk. Please nominate a Beta patch as well.

Comment 12

[(no longer active)]

Comment on attachment 9032076 [details]
Bug 1514762 - Delay modifying the observer list until we hit the event loop;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1498370

User impact if declined: This may be the sign of a UAF bug hiding somewhere, we aren't quite sure.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch itself is very safe, it's only delaying the observer registration by the main thread loop visit.

String changes made/needed: None

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/eddeea139dba1165867c563d0a117b5c0aaaeb18
https://hg.mozilla.org/mozilla-central/rev/eddeea139dba

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032076 [details]
Bug 1514762 - Delay modifying the observer list until we hit the event loop;

[Triage Comment]
Fixes a UAF-looking crash introduced in Fx64. Approved for 65.0b6.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34be2a107bd4