Closed Bug 1514762 Opened 1 year ago Closed 1 year ago
Sanitizer: unknown-crash [@ Length] with READ of size 8 in CC through [@ ns Observer List::Append Strong Observers]
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.
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.
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.
(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?
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+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
You need to log in before you can comment on or make changes to this bug.