Closed Bug 1284690 (CVE-2016-5281) Opened 4 years ago Closed 4 years ago
use-after-free in DOMSVGLength
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: 0000000000000000 Parameter: 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
Severity: normal → critical
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
Version: unspecified → Trunk
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
Attachment #8774347 - Attachment is patch: true
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
Attachment #8774347 - Attachment is obsolete: true
Not sure how easy it would be to exploit this, but calling it sec-critical for now.
Looks like this was introduced in bug 886416 and affects builds as far back as FF31.
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 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?
Attachment #8774476 - Flags: review?(ehsan) → review+
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 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.
Sorry but this is too late for 48 and 45.3.0
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 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
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.
I'm not quite sure how to handle the flags here, but we should not land the patch from this bug at the moment.
Whiteboard: [fixed in bug 1288228]
(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
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.
(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.
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.)
Confirmed fixed on my end in Nightly. :thumbsup:
Thanks! I'll dupe this over to bug 1288228 then. Also, I've just requested esr45 uplift approval over there.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1288228
(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.)
No longer depends on: 1288228
Attachment #8774810 - Attachment is obsolete: true
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.
Flags: sec-bounty? → sec-bounty+
If there is an advisory written for this, it will have a CVE. I expect it will be included in advisories.
Flags: sec-bounty+ → sec-bounty?
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)
("the dupe bug" being bug 1288228)
I've done the approval.
Flags: needinfo?(dveditz) → needinfo?(dholbert)
Thanks. Backported to ESR45 over on bug 1288228.
Whiteboard: [fixed in bug 1288228] → [fixed in bug 1288228][adv-main49+][adv-esr45.4+]
Summary: null ptr deref / access violation / crash [@ nsINode::GetComposedDoc+0] → use-after-free in DOMSVGLength
You need to log in before you can comment on or make changes to this bug.