Closed Bug 1514762 Opened 1 year ago Closed 1 year ago

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

Categories

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

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: decoder, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main65+])

Attachments

(2 files)

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.
nsGlobalWindowOuter::Create calling nsObserverService::AddObserver hints that this could be something about bug 1498370.
Flags: needinfo?(ehsan)
Thanks, will take a look.
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?
Flags: needinfo?(ehsan) → needinfo?(continuation)
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.
Flags: needinfo?(continuation) → needinfo?(ehsan)
Hmm, yeah that's a good theory.  I can write a patch based on that.
Flags: needinfo?(ehsan)
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.
Assignee: nobody → ehsan
Group: core-security → dom-core-security
(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 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.
Attachment #9032076 - Flags: sec-approval?
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.
Attachment #9032076 - Flags: sec-approval? → sec-approval+
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
Attachment #9032076 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/integration/autoland/rev/eddeea139dba1165867c563d0a117b5c0aaaeb18
https://hg.mozilla.org/mozilla-central/rev/eddeea139dba
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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.
Attachment #9032076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.