Closed Bug 1068218 (CVE-2014-1581) Opened 5 years ago Closed 5 years ago
Utils Use-After-Free, crash [@ mozilla::dom::Element::Update State(bool) ] (ZDI-CAN-2531)
ZDI reported the following to the security@ alias: ZDI-CAN-2531: Mozilla Firefox DirectionalityUtils Use-After-Free Remote Code Execution Vulnerability -- CVSS ----------------------------------------- 6.8, AV:N/AC:M/Au:N/C:P/I:P/A:P -- ABSTRACT ------------------------------------- HP's Zero Day Initiative has identified a vulnerability affecting the following products: Mozilla Firefox -- VULNERABILITY DETAILS ------------------------ Verified against Firefox 32.0 Stable on Windows 8.1 as well as the nightly build 35.0 a1. Note that this might require a couple of tries to trigger properly. Debug info against the night build: ``` (df8.a2c): Access violation - code c0000005 (first chance) First chance exceptions are reported before any exception handling. This exception may be expected and handled. eax=5a5a5a5a ebx=0f223c80 ecx=0f9c1560 edx=0f9c1560 esi=0f9c1560 edi=00000002 eip=60536caf esp=00eee09c ebp=00eee0bc iopl=0 nv up ei pl nz na po nc cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00210202 xul!mozilla::dom::Element::UpdateState+0xf: 60536caf 8b80a8010000 mov eax,dword ptr [eax+1A8h] ds:0023:5a5a5c02=???????? [... Rest of stack deleted, can be retrieved if needed --dveditz] -- CREDIT --------------------------------------- This vulnerability was discovered by: regenrecht working with HP's Zero Day Initiative
I can easily reproduce the crash at UpdateState(bool), looks like it's reading
an object that's really deleted memory, 0x5a5a5c02 is a pretty consistent address in Nightly bp-3a676261-3e80-401a-81b2-b8b0a2140916 bp-7418b142-ede9-42c5-9293-78b652140916 I did get one crash in nsINode::UnsetProperty() which had mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection on the stack instead of mozilla::nsTextNodeDirectionalityMap::SetNodeDirection but obviously related. bp-965f2db7-087e-43f8-8b8f-f720f2140916 [Unrelated side note: was quite nice testing this with e10s enabled since only the tab crashed instead of the whole browser (as we wanted). Yay e10s.]
Before the testcase crashes, I get ###!!! ASSERTION: we shouldn't be treating surrogate codepoints as characters: '!IS_SURROGATE(aCh)', which is from a patch by jfkthame that I happen to have in my patch queue. Preventing that assertion stops the crash, but I'm not sure whether it really addresses the root cause. Jonathan, WDYT?
Attachment #8490748 - Flags: feedback?(jfkthame)
Well, it's true that we shouldn't generally be trying to read *character* properties of individual surrogate codepoints, as that's not meaningful. But I don't see why that should lead to a crash... GetDirectionFromChar() just calls GetBidiCat(ch), which looks up GetCharProps2(ch), which ought to safely return a valid (if not meaningful) nsCharProps2 record. So I'm guessing that this patch doesn't really fix the root cause of the crash, it just changes behavior sufficiently that this particular testcase no longer hits it. The more basic problem seems to be that we're doing something that uses an invalid (deleted?) mozilla::dom::Element. We need to figure out how we end up in that situation...
Assigning to Simon since he wrote a patch.
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar OK, after debugging I see that the mis-handling of surrogates is (at least partly) the root cause of the crash. I'll try to explain why: Text nodes with dir=auto and strong directional characters maintain a map of all the other content nodes whose directionality is determined by the direction of their text content. When there is a change to the contents of the text node, we examine the new content to see if it has strong directional characters. As an optimization, if we are appending text to a node after the first strong directional character, we don't look at the added text because it's not going to change anything. However in the testcase here, the original text containing a lone surrogate is assessed as LTR with the first strong character at offset 0, so we create the map. Then the low surrogate is added at offset 1, and the whole text is now a single neutral character. When text changes from LTR to non-directional, we should clear the map, but because the offset is after the original strong "character", the optimization above kicks in and the map never gets cleared. So in short I think this is the correct fix!
Attachment #8490748 - Flags: review?(ehsan.akhgari)
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar Review of attachment 8490748 [details] [diff] [review]: ----------------------------------------------------------------- OK, you talked me into it! Thanks for the debugging/explanation.
Attachment #8490748 - Flags: feedback?(jfkthame) → feedback+
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar Review of attachment 8490748 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch!
Attachment #8490748 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar [Security approval request comment] How easily could an exploit be constructed based on the patch? If somebody made the POC in the report, somebody else could do the same more easily with the patch as a pointer towards an issue with lone surrogates. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? All supported branches If not all supported branches, which bug introduced the flaw? Bug 548206 or one of its followups Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Creating backports will be trivial and the same risk as the trunk patch. How likely is this patch to cause regressions; how much testing does it need? Very low risk of regressions. I want to create automatic tests that cover different variations and permutations of the scenario in the testcase, but I don't know how well automatic tests will work for this since the testcase as-is takes some time to crash.
Attachment #8490748 - Flags: sec-approval?
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar sec-approval+ for trunk. Please make and nominate Aurora, Beta, and ESR31 patches.
Attachment #8490748 - Flags: sec-approval? → sec-approval+
Comment on attachment 8490748 [details] [diff] [review] Don't pass lone surrogates to GetDirectionFromChar Approval Request Comment [Feature/regressing bug #]: Bug 548206 or one of its followups [User impact if declined]: sec-crit [Describe test coverage new/current, TBPL]: I'll check in testcases after the bug is opened [Risks and why]: No risks known [String/UUID change made/needed]: None
Attachment #8490748 - Flags: approval-mozilla-esr31?
Attachment #8490748 - Flags: approval-mozilla-esr31+
Attachment #8490748 - Flags: approval-mozilla-beta?
Attachment #8490748 - Flags: approval-mozilla-beta+
Attachment #8490748 - Flags: approval-mozilla-aurora?
Attachment #8490748 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a9009a5c1d (The patch applies cleanly as-is to the branches, but I'll let it bake a few days before checking in)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
https://hg.mozilla.org/releases/mozilla-aurora/rev/029d9dfe6292 https://hg.mozilla.org/releases/mozilla-beta/rev/389dd23d771c https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/dc61f92b855e https://hg.mozilla.org/releases/mozilla-esr31/rev/1fa747e7e955 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/47aa9a09ccb5
Confirmed crash on Fx31.1.0esr. Verified fixed on Fx31.1.1esr.
Reproduced the original issue using the following builds: - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-16-13-31-40-mozilla-central/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-16-00-40-01-mozilla-aurora/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0b4/win32/en-US/ OS's used for each channel: - Win 8.1 x64 (VM) - Passed - Ubuntu 13.10 x64 (VM) - Passed - OSX 10.9.5 - Passed (couldn't test with aurora and beta due to the v2 signing) FX 35: - Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-01-03-02-05-mozilla-central/ FX 34: - Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-01-00-40-05-mozilla-aurora/ FX 33: - Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0b8/ Test Cases used for each channel: - opened the poc in a new tab for a few minutes - opened the poc in several tabs/windows - opened the poc in several private windows/tabs - opened the poc in several e10s windows/tabs (on m-c only)
You need to log in before you can comment on or make changes to this bug.