Brian Carpenter [:geeknik] Reporter
	Description • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 6 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 10 • 8 years ago

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.

	(no longer active)
	Comment 11 • 8 years ago

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?

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 8 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 14 • 8 years ago

Sorry but this is too late for 48 and 45.3.0

	Gerry Chang [:gchang]
	Comment 15 • 8 years ago

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

	Marcia Knous [:marcia]
	Comment 16 • 8 years ago

Tracking 49/50+ for this sec high crash.

	Daniel Veditz [:dveditz]
	Comment 17 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 18 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 19 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago

(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

	Daniel Holbert [:dholbert] Assignee
	Comment 21 • 8 years ago

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.

	Daniel Holbert [:dholbert] Assignee
	Comment 22 • 8 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 23 • 8 years ago

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

	Brian Carpenter [:geeknik] Reporter
	Comment 24 • 8 years ago

Confirmed fixed on my end in Nightly. :thumbsup:

	Daniel Holbert [:dholbert] Assignee
	Comment 25 • 8 years ago

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

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

	Daniel Holbert [:dholbert] Assignee
	Comment 26 • 8 years ago

(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.)

	Brian Carpenter [:geeknik] Reporter
	Comment 27 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 28 • 8 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 29 • 8 years ago

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)

	Daniel Holbert [:dholbert] Assignee
	Comment 30 • 8 years ago

("the dupe bug" being bug 1288228)

	Al Billings [:abillings - ex-MoCo]
	Comment 31 • 8 years ago

I've done the approval.

	Daniel Holbert [:dholbert] Assignee
	Comment 32 • 8 years ago

Thanks. Backported to ESR45 over on bug 1288228.