1284690 - (CVE-2016-5281) use-after-free in DOMSVGLength

Status: RESOLVED DUPLICATE of bug 1288228

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

Description

[Brian Carpenter [:geeknik]]

Attachment: Stack Text from WinDBG

While fuzzing the latest Firefox Nightly build (Built from https://hg.mozilla.org/mozilla-central/rev/c9a70b64f2faa264296f0cc90d68a2ee2bac6ac5) with cross_fuzz (http://lcamtuf.coredump.cx/cross_fuzz/), I encountered a null ptr dereference / access violation / crash @ nsINode::GetComposedDoc.

cross_fuzz command line:
file:///d:/cross_fuzz_v3/cross_fuzz_randomized_20110105_seed.html#8088

time to crash:
10-15 minutes

FAULTING_IP: 
xul!nsINode::GetComposedDoc+0 [c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\obj-firefox\dist\include\nsinode.h @ 536]
00007ffc`a0e53af8 f7411800080000  test    dword ptr [rcx+18h],800h

EXCEPTION_RECORD:  ffffffffffffffff -- (.exr 0xffffffffffffffff)
.exr 0xffffffffffffffff
ExceptionAddress: 00007ffca0e53af8 (xul!nsINode::GetComposedDoc)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 0000000000000000
   Parameter[1]: 0000000000000018
Attempt to read from address 0000000000000018

FAULTING_THREAD:  000000000000153c

PROCESS_NAME:  firefox.exe

ERROR_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_CODE: (NTSTATUS) 0xc0000005 - The instruction at 0x%08lx referenced memory at 0x%08lx. The memory could not be %s.

EXCEPTION_PARAMETER1:  0000000000000000

EXCEPTION_PARAMETER2:  0000000000000018

READ_ADDRESS:  0000000000000018 

FOLLOWUP_IP: 
xul!nsINode::GetComposedDoc+0 [c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\obj-firefox\dist\include\nsinode.h @ 536]
00007ffc`a0e53af8 f7411800080000  test    dword ptr [rcx+18h],800h

NTGLOBALFLAG:  70

APPLICATION_VERIFIER_FLAGS:  0

BUGCHECK_STR:  APPLICATION_FAULT_NULL_CLASS_PTR_DEREFERENCE_NULL_POINTER_READ_INVALID_POINTER_READ

PRIMARY_PROBLEM_CLASS:  NULL_CLASS_PTR_DEREFERENCE

DEFAULT_BUCKET_ID:  NULL_CLASS_PTR_DEREFERENCE

LAST_CONTROL_TRANSFER:  from 00007ffca0fb90c9 to 00007ffca0e53af8

FAULTING_SOURCE_CODE:  
No source found for 'c:\builds\moz2_slave\m-cen-w64-ntly-000000000000000\build\src\obj-firefox\dist\include\nsinode.h'


SYMBOL_STACK_INDEX:  0

SYMBOL_NAME:  xul!nsINode::GetComposedDoc+0

FOLLOWUP_NAME:  MachineOwner

MODULE_NAME: xul

IMAGE_NAME:  xul.dll

DEBUG_FLR_IMAGE_TIMESTAMP:  577c1d67

STACK_COMMAND:  ~52s ; kb

FAILURE_BUCKET_ID:  NULL_CLASS_PTR_DEREFERENCE_c0000005_xul.dll!nsINode::GetComposedDoc

BUCKET_ID:  X64_APPLICATION_FAULT_NULL_CLASS_PTR_DEREFERENCE_NULL_POINTER_READ_INVALID_POINTER_READ_xul!nsINode::GetComposedDoc+0

WATSON_STAGEONE_URL:  http://watson.microsoft.com/StageOne/firefox_exe/50_0_0_6030/577c1612/xul_dll/50_0_0_6030/577c1d67/c0000005/00103af8.htm?Retriage=1

Followup: MachineOwner

Comment 1

[Brian Carpenter [:geeknik]]

The last null ptr deref crasher that I reported was fixed pretty quickly when I flipped the security bit, but 2 weeks on and nobody has noticed this one?

Comment 2

[Noah (NoahSUMO/SolidSnake) [:Noah]]

That's because not every crasher you find with your fuzzer is a security exploit. Bug 1283674 wasn't even deemed a security exploit after it was fixed.

I suggest being more patient in these matters. Especially if the crashers are not easy to reproduce or exploitable. After how long you've been contributing to Mozilla (~11 years), I would have thought you learned that by now. :/

There's also the fact that things can slow down during the summer. For example, the devs who know how to fix this could be on PTO. Coupled with the fact that this is filed under the General component further ensures no one will find this obscure bug unless by accident. Triaging bugs correctly is very important to getting them fixed quickly & seen by the right devs. You need to find the right component for nsINode::GetComposedDoc. I will ask around for you. I'll also NI some security devs for their opinion on this.

Comment 3

[Brian Carpenter [:geeknik]]

Ah, yes, my old friend patience. I have been waiting patiently now for 4 years here: https://bugzilla.mozilla.org/show_bug.cgi?id=760811. Correctly triaged, promptly forgotten. So please, no lectures on patience. I don't have to submit any bugs whatsoever, but here I am, not keeping bugs to myself. I assume y'all want these bug reports as opposed to waking up some morning to news of 50M users pwnt by a zero day that you didn't know about? Yes, this is non exploitable but still paves the way for increased safety and stability in Firefox. So grumpy old timer, go have another coffee and maybe a Snickers  bar and we'll move on and continue making Firefox better. =)

PS, correct component found based on other similar crashes.

Comment 4

[Noah (NoahSUMO/SolidSnake) [:Noah]]

Hehehe. Perfect response, Brian. ;) As we both painfully know, being long time bug filers, tons of bugs can go unfixed over the years for various reasons. Some valid & understandable (bug lost in wrong component, understaffed teams, overworked devs, strict deadlines to complete higher priority bugs, etc.), others being a gross oversight (seen but dismissed too quickly or ignored without reply).

I already knew you'd say it was a lecture. ;) But I wanted to remind you the devs are human too and can't know all or see all. It's up to us to use Bugzilla's tools ("need info", security flag) & IRC (#security) to seek out the right devs to raise priority when it's rightly due.

My response was in reaction to your tweet. I've been waiting for a month to get a simple typo bug fixed (Bug 1282867). And 7 years for a useful feature for crash reports (Bug 515181). I know both severely pale in comparison to your security bugs but I just wanted to illustrate how some bugs can't be fixed according to our hopeful timeframes.

To wrap-up: 
I'm no fuzzing expert nor a dev. So I'd like to defer to you & the security devs on what is a serious vulnerability or not. If you thought there was immediate danger, you'd have set the security flag no question. Since you didn't, that led me to believe you thought the risk of this being exploited was low. Even with the critical flag set. Otherwise I'd have fully backed your complaint about the attention this bug was getting. :) Thanks for all the Firefox fuzzing & security bugs you've filed over the years! I never took it for granted. Just wanted to state the reasons to why it's not fixed yet. :)

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Marking this secure because I believe we have a UAF here.

The DOMSVGLength has these two attributes:

  nsSVGLength2* mVal; // kept alive because it belongs to mSVGElement
  RefPtr<nsSVGElement> mSVGElement;

During cycle collection we clear mSVGElement allowing it to be destroyed.  We don't, however, clear mVal.  This leaves a dangling pointer that is sometimes used to call methods.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: debug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38bc8830345a

Unfortunately Brian is not accepting feedback or NI requests, so I can't ask him to retest with this patch.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: debug

Try failures demonstrate that we are triggering CC and the dangling pointer in our automation tests today.

Here's an updated patch to fix the test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a9443b7aa8

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Not sure how easy it would be to exploit this, but calling it sec-critical for now.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Looks like this was introduced in bug 886416 and affects builds as far back as FF31.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

The try build looks good.

Ehsan, this patch cleans up the mVal weak pointer when we unlink the mSVGElement due to cycle collection.  Since mVal references data within the mSVGElement they need to be managed together.

Comment 11

[(no longer active)]

Comment on attachment 8774476 [details] [diff] [review]
Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

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

::: dom/svg/DOMSVGLength.cpp
@@ +26,5 @@
>    sBaseSVGLengthTearOffTable,
>    sAnimSVGLengthTearOffTable;
>  
> +void
> +CleanupTableVal(bool aIsAnimValItem, nsSVGLength2* aVal)

It's a bit weird that this isn't a member function.  Can you please make it so?

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

Updated based on review feedback.

I also added an assert to the destructor which is enough to make our current tests fail.  I think this means our current tests provide adequate coverage here.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8774810 [details] [diff] [review]
Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This bug describes a reliable way to trigger a UAF, but its not clear how easily that can be leveraged into an exploit.  The attacker must arrange for the DOMSVGLength object to be cycle collected and then trigger its use somehow.

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

No additional comments describe the problem.  We are just cleaning up an additional variable in the CC unlink.  We do add some asserts to verify that our variables are in sync.  I don't think this patch immediately points to the problem, but I guess if someone knows our CC infrastructure well they can see what is happening.

Which older supported branches are affected by this flaw?

FF31 to FF50

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

Bug 886416

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I believe it should backport cleanly, but I have not checked yet.

How likely is this patch to cause regressions; how much testing does it need?

This code is triggered in our current existing tests.  We should have adequate coverage.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Sorry but this is too late for 48 and 45.3.0

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8774810 [details] [diff] [review]
Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

We are going to take it in ESR 45.4 when it is time and won't fix in beta, release as we are too close to the release date

Comment 16

[Marcia Knous [:marcia]]

Tracking 49/50+ for this sec high crash.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 8774810 [details] [diff] [review]
Additional cleanup in DOMSVGLength cycle collection unlink. r=ehsan

sec-approval and a=dveditz to land in aurora (49). Holding off on ESR approval until after the merge so this doesn't accidentally slip into 45.3

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

It appears dholbert just landed a similar fix in bug 1288228.

Daniel, can you get that bug uplifted to 49?  This is a security issue since its a UAF situation.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

I'm not quite sure how to handle the flags here, but we should not land the patch from this bug at the moment.

Comment 20

[Carsten Book [:Tomcat]]

(In reply to Ben Kelly [:bkelly] from comment #19)
> I'm not quite sure how to handle the flags here, but we should not land the
> patch from this bug at the moment.

ok so holding the uplift for the moment, let me /us know when we should uplift this

Comment 21

[Daniel Holbert [:dholbert]]

Sorry, I've been out in the woods for a few days; just getting back now.

I'll request 49 uplift approval (which means beta approval now, I think) on the other bug, with some hand-wavy language about stability so as not to reveal the potential sec risk in a public bug.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Marking this secure because I believe we have a UAF here.
[...]
> During cycle collection we clear mSVGElement allowing it to be destroyed. 
> We don't, however, clear mVal.  This leaves a dangling pointer that is
> sometimes used to call methods.

Note: it seems hypothetically like this UAF might happen, BUT -- in practice when I've seen us crash, the dangling pointer "mVal" is always pointing to something that is still alive.  I suspect that might have to be the case here. So I'm skeptical that this can actually lead to a UAF.

In particular:
 - I believe JS can only get access to the half-cleaned-up DOMSVGLength by querying the tearoff table.
 - To make this query, it'd have to be using a pointer to the nsSVGLength2 object (the same one that mVal points to) -- and it'd get that (via JS bindings) by having queried using the nsSVGElement that owns the nsSVGLength2. (This is the element that the nulled-out "mElement" formerly pointed to.)
 - So, if JS has a pointer to mElement, it means mElement is still alive, which means the nsSVGLength2 is still alive.

SO: while it's probably good to assume the worst here, I also think there's a decent chance this isn't actually exploitable.

In any case, I requested Firefox 49 uplift-approval over on bug 1288228. I can request ESR approval, too, though I'm not sure it's necessary.

Comment 23

[Daniel Holbert [:dholbert]]

This should be fixed in the 49beta source now (via landings bug 1284690 comment 32).  So, presumably the next beta49 release should have this fixed.

(And it's already fixed in Firefox 50 & later, via landings in bug 1288228 comment 29.)

Comment 24

[Brian Carpenter [:geeknik]]

Confirmed fixed on my end in Nightly. :thumbsup:

Comment 25

[Daniel Holbert [:dholbert]]

Thanks! I'll dupe this over to bug 1288228 then.

Also, I've just requested esr45 uplift approval over there.

Comment 26

[Daniel Holbert [:dholbert]]

(If it matters for "sec-bounty?"-resolution purposes: this bug here was filed before its (independently-discovered-by-me) dupe-target bug, bug 1288228.  I'm duping forward simply because the other bug is where we ended up investigating thoroughly & landing the patch.  Also: per comment 22, I'm not sure it would've been possible to turn this issue into a UAF.  But I also am not 100% sure it's impossible.)

Comment 27

[Brian Carpenter [:geeknik]]

I know there is talk about how this might/might not be exploitable (I don't have a test case available showing exploitability, so I can't sway you one way or the other at this time), but is it at least serious enough to warrant a CVE assignment, sec advisory, etc? Thanks.

Comment 28

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

If there is an advisory written for this, it will have a CVE. I expect it will be included in advisories.

Comment 29

[Daniel Holbert [:dholbert]]

dveditz, looks like you're the one who marked this bug as "tracking-esr45: 49+". Any chance you can approve the uplift request in the dupe-target bug? (and perhaps transfer the "tracking" status over there)

Comment 30

[Daniel Holbert [:dholbert]]

("the dupe bug" being bug 1288228)

Comment 31

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

I've done the approval.

Comment 32

[Daniel Holbert [:dholbert]]

Thanks. Backported to ESR45 over on bug 1288228.