Closed Bug 1315543 (CVE-2016-9896) Opened 3 years ago Closed 3 years ago

dom::Navigator::NotifyVRDisplaysUpdated UAF

Categories

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

52 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: filipesw, Assigned: kip)

References

Details

(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [adv-main50.1+])

Attachments

(6 files, 4 obsolete files)

1.90 KB, text/plain
Details
6.34 KB, text/plain
Details
12 bytes, text/xml
Details
5.86 KB, patch
daoshengmu
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
6.13 KB, patch
Details | Diff | Splinter Review
7.00 KB, patch
Details | Diff | Splinter Review
Attached file report.txt
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=??
Attached file windbg.txt
Attached file external.xml
Blocks: webvr
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM
Product: Firefox → Core
Yeah, that's no good at all.
Blocks: 1182048
Flags: needinfo?(vladimir)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jet, is there anyone who can take a look while Kip's on PTO?
Flags: needinfo?(bugs)
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)
Flags: needinfo?(dmu)
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
Flags: needinfo?(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/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
Flags: needinfo?(abillings)
Target Milestone: --- → mozilla53
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
Attachment #8813920 - Flags: approval-mozilla-aurora?
(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.
Flags: sec-bounty?
Group: dom-core-security → core-security-release
(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)
Flags: sec-bounty? → sec-bounty+
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.
Flags: needinfo?(kgilbert)
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?
Attachment #8813920 - Flags: sec-approval?
Attachment #8813920 - Flags: sec-approval+
Attachment #8813920 - Flags: approval-mozilla-aurora?
Attachment #8813920 - Flags: approval-mozilla-aurora+
Keywords: regression
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2717a47346

ni? kip for the other approval requests that are still needed.
Flags: needinfo?(kgilbert)
Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Per comment 18
Flags: needinfo?(kgilbert)
Attachment #8813920 - Flags: approval-mozilla-release?
Attachment #8813920 - Flags: approval-mozilla-beta?
Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Sec-high, approved for all branches
Attachment #8813920 - Flags: approval-mozilla-release?
Attachment #8813920 - Flags: approval-mozilla-release+
Attachment #8813920 - Flags: approval-mozilla-beta?
Attachment #8813920 - Flags: approval-mozilla-beta+
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
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
Whiteboard: [adv-main50.1+]
Alias: CVE-2016-9896
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.