Last Comment Bug 1315543 - (CVE-2016-9896) dom::Navigator::NotifyVRDisplaysUpdated UAF
(CVE-2016-9896)
: dom::Navigator::NotifyVRDisplaysUpdated UAF
Status: RESOLVED FIXED
[adv-main50.1+]
: csectype-uaf, regression, sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 52 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla53
Assigned To: :kip (Kearwood Gilbert)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: webvr 1182048
  Show dependency treegraph
 
Reported: 2016-11-06 08:47 PST by Filipe Gomes
Modified: 2017-02-09 08:03 PST (History)
14 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
+
fixed
unaffected


Attachments
report.txt (1.90 KB, text/plain)
2016-11-06 08:47 PST, Filipe Gomes
no flags Details
windbg.txt (6.34 KB, text/plain)
2016-11-06 08:49 PST, Filipe Gomes
no flags Details
external.xml (12 bytes, text/xml)
2016-11-06 08:51 PST, Filipe Gomes
no flags Details
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (5.86 KB, patch)
2016-11-23 17:25 PST, :kip (Kearwood Gilbert)
dmu: review+
abillings: approval‑mozilla‑aurora+
rkothari: approval‑mozilla‑beta+
rkothari: approval‑mozilla‑release+
abillings: sec‑approval+
Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch) (5.91 KB, patch)
2016-11-30 14:53 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch) (5.02 KB, patch)
2016-11-30 15:14 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch) (6.57 KB, patch)
2016-11-30 15:29 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch) (6.07 KB, patch)
2016-11-30 15:29 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch) (6.13 KB, patch)
2016-11-30 15:59 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch) (7.00 KB, patch)
2016-11-30 16:32 PST, :kip (Kearwood Gilbert)
no flags Details | Diff | Splinter Review

Description User image Filipe Gomes 2016-11-06 08:47:15 PST
Created attachment 8807970 [details]
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=??
Comment 1 User image Filipe Gomes 2016-11-06 08:49:45 PST
Created attachment 8807971 [details]
windbg.txt
Comment 2 User image Filipe Gomes 2016-11-06 08:51:00 PST
Created attachment 8807972 [details]
external.xml
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-07 11:49:07 PST
Yeah, that's no good at all.
Comment 5 User image Andrew Overholt [:overholt] 2016-11-14 13:32:42 PST
Jet, is there anyone who can take a look while Kip's on PTO?
Comment 6 User image Jet Villegas (:jet) 2016-11-15 00:42:57 PST
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.
Comment 7 User image Daosheng Mu[:daoshengmu] 2016-11-15 08:04:17 PST
(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)
Comment 8 User image Andrew Overholt [:overholt] 2016-11-21 09:55:45 PST
What are the next steps here?
Comment 9 User image :kip (Kearwood Gilbert) 2016-11-22 12:41:56 PST
(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.
Comment 10 User image :kip (Kearwood Gilbert) 2016-11-23 15:12:42 PST
I have reproduced the crash with the testcase attached to this bug and can verify the fix as recommended by Daosheng and Jet above.
Comment 11 User image :kip (Kearwood Gilbert) 2016-11-23 17:25:13 PST
Created attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

This patch eliminates the crash
Comment 12 User image Daosheng Mu[:daoshengmu] 2016-11-23 18:29:37 PST
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.
Comment 13 User image :kip (Kearwood Gilbert) 2016-11-24 13:27:46 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb16059022c621e095d2e013a1f14088e0055714
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated,r=dmu
Comment 14 User image Carsten Book [:Tomcat] 2016-11-25 07:26:38 PST
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
Comment 15 User image :kip (Kearwood Gilbert) 2016-11-25 14:58:05 PST
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
Comment 16 User image :kip (Kearwood Gilbert) 2016-11-25 14:59:04 PST
(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.
Comment 17 User image Al Billings [:abillings] 2016-11-28 10:38:25 PST
(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.
Comment 18 User image :kip (Kearwood Gilbert) 2016-11-28 16:40:44 PST
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.
Comment 19 User image Al Billings [:abillings] 2016-11-29 17:11:38 PST
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?
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2016-11-29 18:58:03 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2717a47346

ni? kip for the other approval requests that are still needed.
Comment 21 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 13:09:14 PST
Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Per comment 18
Comment 22 User image Ritu Kothari (:ritu) 2016-11-30 14:13:03 PST
Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Sec-high, approved for all branches
Comment 23 User image :kip (Kearwood Gilbert) 2016-11-30 14:53:55 PST
Created attachment 8815895 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)
Comment 24 User image :kip (Kearwood Gilbert) 2016-11-30 15:14:16 PST
Created attachment 8815902 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)

In the release branch, "VRDisplay" is called "VRDevice" but the patch is otherwise the same.
Comment 25 User image :kip (Kearwood Gilbert) 2016-11-30 15:18:43 PST
(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.
Comment 26 User image :kip (Kearwood Gilbert) 2016-11-30 15:29:23 PST
Created attachment 8815909 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)
Comment 27 User image :kip (Kearwood Gilbert) 2016-11-30 15:29:54 PST
Created attachment 8815910 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)
Comment 28 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 15:39:28 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/e637d4169309
Comment 29 User image :kip (Kearwood Gilbert) 2016-11-30 15:43:03 PST
I found a build failure in my patches due to dependencies.  Updating now
Comment 30 User image Ryan VanderMeulen [:RyanVM] 2016-11-30 15:50:15 PST
Indeed :(. Backed out from Beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/1930eb067498647c3a291fceaa63a3518102f92d
Comment 31 User image :kip (Kearwood Gilbert) 2016-11-30 15:59:41 PST
Created attachment 8815918 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)

Tested with local build, compiles now
Comment 32 User image :kip (Kearwood Gilbert) 2016-11-30 16:16:02 PST
Waiting for local build to test the beta patch, will update shortly.
Comment 33 User image :kip (Kearwood Gilbert) 2016-11-30 16:32:12 PST
Created attachment 8815940 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)

Tested this one locally on OSX 10.12.1
Comment 34 User image :kip (Kearwood Gilbert) 2016-11-30 16:32:57 PST
Both the release and beta patches are updated now with dependencies fixed.  Tested locally on OSX 10.12.1

Note You need to log in before you can comment on or make changes to this bug.