1315543 - (CVE-2016-9896) dom::Navigator::NotifyVRDisplaysUpdated UAF

Status: RESOLVED FIXED

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

Description

[lipe]

Attachment: 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

[lipe]

Attachment: windbg.txt

Comment 2

[lipe]

Attachment: external.xml

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/vr/ipc/VRManagerChild.h#155 looks bad.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that's no good at all.

Comment 5

[Andrew Overholt [:overholt]]

Jet, is there anyone who can take a look while Kip's on PTO?

Comment 6

[Jet Villegas (inactive)]

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

[Daosheng Mu[:daoshengmu]]

(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

[Andrew Overholt [:overholt]]

What are the next steps here?

Comment 9

[:kip (Kearwood Gilbert)]

(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

[:kip (Kearwood Gilbert)]

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

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

This patch eliminates the crash

Comment 12

[Daosheng Mu[:daoshengmu]]

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

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb16059022c621e095d2e013a1f14088e0055714
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated,r=dmu

Comment 14

[Carsten Book [:Tomcat]]

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

[:kip (Kearwood Gilbert)]

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

[:kip (Kearwood Gilbert)]

(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

[Al Billings [:abillings - ex-MoCo]]

(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

[:kip (Kearwood Gilbert)]

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

[Al Billings [:abillings - ex-MoCo]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2717a47346

ni? kip for the other approval requests that are still needed.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Per comment 18

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

Sec-high, approved for all branches

Comment 23

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)

Comment 24

[:kip (Kearwood Gilbert)]

Attachment: 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

[:kip (Kearwood Gilbert)]

(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

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)

Comment 27

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e637d4169309

Comment 29

[:kip (Kearwood Gilbert)]

I found a build failure in my patches due to dependencies.  Updating now

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Indeed :(. Backed out from Beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/1930eb067498647c3a291fceaa63a3518102f92d

Comment 31

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)

Tested with local build, compiles now

Comment 32

[:kip (Kearwood Gilbert)]

Waiting for local build to test the beta patch, will update shortly.

Comment 33

[:kip (Kearwood Gilbert)]

Attachment: Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)

Tested this one locally on OSX 10.12.1

Comment 34

[:kip (Kearwood Gilbert)]

Both the release and beta patches are updated now with dependencies fixed.  Tested locally on OSX 10.12.1

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63121d897767
https://hg.mozilla.org/releases/mozilla-release/rev/5096ba1c0cbc