1068218 - (CVE-2014-1581) DirectionalityUtils Use-After-Free, crash [@ mozilla::dom::Element::UpdateState(bool) ] (ZDI-CAN-2531)

Status: VERIFIED FIXED

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

Description

[Daniel Veditz [:dveditz]]

Attachment: POC (will crash affected versions)

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

Comment 1

[Daniel Veditz [:dveditz]]

I can easily reproduce the crash at UpdateState(bool), looks like it's reading

Comment 2

[Daniel Veditz [:dveditz]]

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.]

Comment 3

[Simon Montagu :smontagu]

Attachment: Don't pass lone surrogates to GetDirectionFromChar

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?

Comment 4

[Jonathan Kew [: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...

Comment 5

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

Assigning to Simon since he wrote a patch.

Comment 6

[Simon Montagu :smontagu]

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!

Comment 7

[Jonathan Kew [:jfkthame]]

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.

Comment 8

[(no longer active)]

Comment on attachment 8490748 [details] [diff] [review]
Don't pass lone surrogates to GetDirectionFromChar

Review of attachment 8490748 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch!

Comment 9

[Simon Montagu :smontagu]

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.

Comment 10

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

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.

Comment 11

[Simon Montagu :smontagu]

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

Comment 12

[Simon Montagu :smontagu]

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)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81a9009a5c1d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/dc61f92b855e

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on Fx31.1.0esr.
Verified fixed on Fx31.1.1esr.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

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)