Closed Bug 1315543 (CVE-2016-9896) Opened 3 years ago Closed 3 years ago
VRDisplays Updated UAF
1.90 KB, text/plain
6.34 KB, text/plain
12 bytes, text/xml
5.86 KB, patch
|Details | Diff | Splinter Review|
6.13 KB, patch
|Details | Diff | Splinter Review|
7.00 KB, patch
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.40 Build ID: 20160314222922 Steps to reproduce: Saved an xml file containing only "<svg></svg>" to external.xml in the directory root together with the attached html, this external file is loaded inside the attached html with a object tag, the node containing it's own window reference by issuing a call to navigator.getVRDisplays followed by the tag removal causes the usage of the freed navigator. Tested with a Nightly build 32bit version: Name Firefox Version 52.0a1 Build ID 20161007030207 A more detailed windbg output is attached aswell. Actual results: (bf8.d60): Access violation - code c0000005 (first chance) First chance exceptions are reported before any exception handling. This exception may be expected and handled. eax=e5e5e5d5 ebx=00000000 ecx=e5e5e5d5 edx=1506d5b4 esi=142eb2c8 edi=17c081f0 eip=687f9216 esp=003aecc8 ebp=003aecd4 iopl=0 nv up ei pl nz na po nc cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00210202 xul!nsGlobalWindow::UpdateVRDisplays+0x8: 687f9216 80795e00 cmp byte ptr [ecx+5Eh],0 ds:002b:e5e5e633=??
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM
Product: Firefox → Core
Yeah, that's no good at all.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jet, is there anyone who can take a look while Kip's on PTO?
Kip gets back on 11/21 but perhaps Daosheng can have a look in the meantime. I think we could manage these dom::Navigator pointers like we do for ServiceWorkers (i.e, no raw pointer arrays) and instead access the dom::Navigator* via checked InnerWindow references.
Flags: needinfo?(bugs) → needinfo?(dmu)
(In reply to Jet Villegas (:jet) from comment #6) > Kip gets back on 11/21 but perhaps Daosheng can have a look in the meantime. > I think we could manage these dom::Navigator pointers like we do for > ServiceWorkers (i.e, no raw pointer arrays) and instead access the > dom::Navigator* via checked InnerWindow references. It looks like we can send the global window's raw pointer or window id (https://dxr.mozilla.org/mozilla-central/rev/5e76768327660437bf3486554ad318e4b70276e1/dom/base/Navigator.cpp#1631) to VRManagerChild and be stored to an array. Then, we access Navigator via InnerWindow() (https://dxr.mozilla.org/mozilla-central/rev/5e76768327660437bf3486554ad318e4b70276e1/dom/workers/ServiceWorkerClient.cpp#110)
What are the next steps here?
Flags: needinfo?(vladimir) → needinfo?(kgilbert)
(In reply to Andrew Overholt [:overholt] from comment #8) > What are the next steps here? I am back from PTO now and can take the bug. I'll apply the changes as suggested by Daosheng and Jet.
Assignee: nobody → kgilbert
I have reproduced the crash with the testcase attached to this bug and can verify the fix as recommended by Daosheng and Jet above.
This patch eliminates the crash
Attachment #8813920 - Flags: review?(dmu)
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated Review of attachment 8813920 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #8813920 - Flags: review?(dmu) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb16059022c621e095d2e013a1f14088e0055714 Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated,r=dmu
https://hg.mozilla.org/mozilla-central/rev/bb16059022c6 shouldn't this needed sec-approval per https://wiki.mozilla.org/Security/Bug_Approval_Process ? Also since 52 is marked as affected i guess we need a aurora approval request
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated Approval Request Comment [Feature/regressing bug #]: Bug 1182048 - Make webvr and e10s work together [User impact if declined]: UAF Vulnerability [Describe test coverage new/current, TreeHerder]: Reproduced UAF using test case attached to bug, no longer occurs with patch. Manually tested WebVR functionality on an Oculus DK2 to ensure that it does not regress with this patch. [Risks and why]: Low risk, as is a localized change that should affect only WebVR sites. [String/UUID change made/needed]: none
(In reply to Carsten Book [:Tomcat] from comment #14) > https://hg.mozilla.org/mozilla-central/rev/bb16059022c6 > > shouldn't this needed sec-approval per > https://wiki.mozilla.org/Security/Bug_Approval_Process ? > > Also since 52 is marked as affected i guess we need a aurora approval request Thanks for notifying me of the bug approval process. I have added an aurora approval request to the attachment details. Please advise if I may have missed any other steps.
(In reply to Carsten Book [:Tomcat] from comment #14) > shouldn't this needed sec-approval per > https://wiki.mozilla.org/Security/Bug_Approval_Process ? Yes, this violated security rules and should have gotten sec-approval. We need the sec-approval? to be set on the patch and the questions in the template answered by the dev.
Flags: needinfo?(abillings) → needinfo?(kgilbert)
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated [Security approval request comment] How easily could an exploit be constructed based on the patch? - The patch adds additional validation, which provides clues about what the pre-conditions of a crash in the content process would require. It would take significantly more work than using the details in the patch to do more than cause the content process to crash when visiting a maliciously formed page. The patch provides no details on how someone could cause remote code execution or otherwise leak information. Remote code execution would require a specific address of the heap to be filled with payload instructions. The patch provides no details on how to deploy a payload or at what address it would need to be loaded. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The check-in comment mentions “UAF” which indicates that the patch is security related. The validation added in the patch indicates the object that will be freed; however, it does not describe any method of causing the UAF. Which older supported branches are affected by this flaw? The vulnerable code landed with Bug 1182048 in Firefox 46 If not all supported branches, which bug introduced the flaw? This impacts all supported branches. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch attached to this bug could be backported with few changes. The risk is moderate, as the changes are relatively isolated but there are no automated tests to test for WebVR related regressions. (See Bug 1229464) How likely is this patch to cause regressions; how much testing does it need? Regressions caused by this patch are most likely to affect WebVR, which is enabled by default only on Nightly and Aurora/Dev Edition. Any regressions affecting non-webvr sites would be found immediately during the first vsync callback that hits the affected function.
Attachment #8813920 - Flags: sec-approval?
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated We'll need a Beta patch nominated. Is ESR45 unaffected for sure?
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2717a47346 ni? kip for the other approval requests that are still needed.
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated Per comment 18
Comment on attachment 8813920 [details] [diff] [review] Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated Sec-high, approved for all branches
In the release branch, "VRDisplay" is called "VRDevice" but the patch is otherwise the same.
(In reply to Ritu Kothari (:ritu) from comment #22) > Comment on attachment 8813920 [details] [diff] [review] > Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated > > Sec-high, approved for all branches I have added patches that are rebased for beta and release branches. Would setting checkin-needed be appropriate here? Please advise if there are any other steps I may have missed, thanks all.
Attachment #8815902 - Attachment is obsolete: true
I found a build failure in my patches due to dependencies. Updating now
Indeed :(. Backed out from Beta for bustage. https://hg.mozilla.org/releases/mozilla-beta/rev/1930eb067498647c3a291fceaa63a3518102f92d
Tested with local build, compiles now
Attachment #8815910 - Attachment is obsolete: true
Waiting for local build to test the beta patch, will update shortly.
Tested this one locally on OSX 10.12.1
Attachment #8815909 - Attachment is obsolete: true
Both the release and beta patches are updated now with dependencies fixed. Tested locally on OSX 10.12.1
You need to log in before you can comment on or make changes to this bug.