Bug 1315543 (CVE-2016-9896)

dom::Navigator::NotifyVRDisplaysUpdated UAF

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: Filipe Gomes, Assigned: kip)

Tracking

({csectype-uaf, regression, sec-high})

52 Branch
mozilla53
csectype-uaf, regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed, firefox-esr45 unaffected)

Details

(Whiteboard: [adv-main50.1+])

Attachments

(6 attachments, 4 obsolete attachments)

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
(Reporter)

Description

8 months ago
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=??
(Reporter)

Comment 1

8 months ago
Created attachment 8807971 [details]
windbg.txt
(Reporter)

Comment 2

8 months ago
Created attachment 8807972 [details]
external.xml
Blocks: 1036600
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM
Product: Firefox → Core

Comment 3

8 months ago
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/vr/ipc/VRManagerChild.h#155 looks bad.
Yeah, that's no good at all.
Blocks: 1182048
Flags: needinfo?(vladimir)
Status: UNCONFIRMED → NEW
Ever confirmed: true
status-firefox49: --- → unaffected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
Keywords: csectype-uaf, sec-high
Jet, is there anyone who can take a look while Kip's on PTO?
Flags: needinfo?(bugs)

Comment 6

7 months ago
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)
(Assignee)

Comment 9

7 months ago
(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)
(Assignee)

Comment 10

7 months ago
I have reproduced the crash with the testcase attached to this bug and can verify the fix as recommended by Daosheng and Jet above.
(Assignee)

Comment 11

7 months ago
Created attachment 8813920 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated

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+
(Assignee)

Comment 13

7 months ago
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-firefox53: --- → fixed
Flags: needinfo?(abillings)
Target Milestone: --- → mozilla53

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
(Assignee)

Comment 15

7 months ago
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?
(Assignee)

Comment 16

7 months ago
(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)
status-firefox49: unaffected → ?
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 18

7 months ago
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+
status-firefox49: ? → ---
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox53: --- → +
Keywords: regression
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a2717a47346

ni? kip for the other approval requests that are still needed.
status-firefox52: affected → fixed
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?

Updated

7 months ago
tracking-firefox50: ? → +

Comment 22

7 months ago
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+
(Assignee)

Comment 23

7 months ago
Created attachment 8815895 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)
(Assignee)

Comment 24

7 months ago
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.
(Assignee)

Comment 25

7 months ago
(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.
(Assignee)

Comment 26

7 months ago
Created attachment 8815909 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Beta Branch)
Attachment #8815895 - Attachment is obsolete: true
(Assignee)

Comment 27

7 months ago
Created attachment 8815910 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)
(Assignee)

Updated

7 months ago
Attachment #8815902 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-beta/rev/e637d4169309
status-firefox51: affected → fixed
(Assignee)

Comment 29

7 months ago
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
status-firefox51: fixed → affected
(Assignee)

Comment 31

7 months ago
Created attachment 8815918 [details] [diff] [review]
Bug 1315543 - Eliminate UAF in Navigator::NotifyVRDisplaysUpdated (Rebased to Release Branch)

Tested with local build, compiles now
Attachment #8815910 - Attachment is obsolete: true
(Assignee)

Comment 32

7 months ago
Waiting for local build to test the beta patch, will update shortly.
(Assignee)

Comment 33

7 months ago
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
Attachment #8815909 - Attachment is obsolete: true
(Assignee)

Comment 34

7 months ago
Both the release and beta patches are updated now with dependencies fixed.  Tested locally on OSX 10.12.1
https://hg.mozilla.org/releases/mozilla-beta/rev/63121d897767
https://hg.mozilla.org/releases/mozilla-release/rev/5096ba1c0cbc
status-firefox50: affected → fixed
status-firefox51: affected → fixed
Whiteboard: [adv-main50.1+]
Alias: CVE-2016-9896
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.