1287912 - Crash trying to get a nonexistent property off Xray for Image.

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE: (in a debug build, where this is very reliable):

1)  Open scratchpad in browser mode.
2)  Make sure a non-e10s window is open
3)  Evaluate "content.Image['foo']"

EXPECTED RESULTS: undefined

ACTUAL RESULTS: Assert in XrayGetNativeProto because the object we have is a Function, which does not have a DOMJSClass or a DOMIfaceAndProtoJSClass.  In an opt build this ends up reading random memory to get the protoGetter and then calling it and then trying to JS_WrapObject the result, all of which may or may not crash...

Comment 1

[Andrew McCreight [:mccr8]]

Marking this moderate because it seems like this requires that chrome JS does something bad. Though maybe accessing an invalid property is not so rare.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think this is actually sec-high to sec-critical.  Consider chrome JS that is doing a walk over the DOM using firstChild/nextSibling.  Now consider a malicious page that defines nextSibling on one of its nodes to return Image.

Comment 3

[Andrew McCreight [:mccr8]]

Wouldn't that require that it is iterating over without using Xrays? But yeah, it does seem like there could be a bunch of ways to trigger this, and we certainly can't audit all of our browser JS...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Wouldn't that require that it is iterating over without using Xrays? 

Oh, good point.  We need to end up with the Xray for Image, but you're right that a normal DOM walk won't land there...

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Peter Van der Beken [:peterv]]

Bz, want to review?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8807707 [details] [diff] [review]
v1

>+      protop.set(protoAndIfaceCache.EntrySlotMustExist(nativePropertyHooks->mPrototypeID));

This doesn't look right.  This will claim that Image.__proto__ is the canonical HTMLImageElement.prototype (ah, and the test checks this), but it should be the canonical Function.prototype.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 9

[Peter Van der Beken [:peterv]]

Sigh, totally misread the section on .prototype in the spec.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8808119 [details] [diff] [review]
v2

r=me

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8808119 [details] [diff] [review]
v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not very easily, it also requires finding some chrome code that ends up running content script. Talking it over with bz we think there's a chance debugger code could be vulnerable, but that would require the user to be using the debugger on the exploit code, so not a trivial exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

There's a crashtest included in the patch. The patch itself is pretty clear too, however an exploit is not trivial.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

Bug 787070.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch should mostly just apply to branches.

How likely is this patch to cause regressions; how much testing does it need?

Not very likely to cause regressions, as it actually adds code to handle with a case that crashes currently.

Comment 12

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

sec-approval for checkin on 11/22, a week into the new cycle. This seems relatively safe to take.

We'll want branch patches, including ESR45, at that time.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6f55d3d0f40784ba1463ed9250eceb04faeff2

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8808119 [details] [diff] [review]
v2

Approval Request Comment
[Feature/Bug causing the regression]: Bug 787070.
[User impact if declined]: Crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: The automated test runs.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: Just adding code to deal with a case that crashes currently.
[String changes made/needed]: None.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v2 (esr_45 version)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Crashes.
Fix Landed on Version: trunk.
Risk to taking this patch (and alternatives if risky): Just adding code to deal with a case that crashes currently.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8808119 [details] [diff] [review]
v2

Sec-high, ok to uplift to aurora and beta. This should make it into the beta 7 build next Monday.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8816534 [details] [diff] [review]
v2 (esr_45 version)

This should make it into ESR 45.6.0, going to build tomorrow.

Comment 18

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-esr45/rev/778f65148b4009b087c6e7789ffe486a5349922c

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a6f55d3d0f40784ba1463ed9250eceb04faeff2
https://hg.mozilla.org/releases/mozilla-aurora/rev/811d1517145c

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

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

(In reply to Carsten Book [:Tomcat] from comment #18)
>  https://hg.mozilla.org/releases/mozilla-esr45/rev/
> 778f65148b4009b087c6e7789ffe486a5349922c

Argh, this is in ESR 45.6 but was not checked into the "release" branch so it isn't in 50.1. We're not supposed to ship a security bug in ESR and then not fix it in the mainline release going out at the same time. We're supposed to keep them in synch so we don't 0day users. I can't write an advisory for this for ESR45 since 50.1 is unfixed.

Liz? Ritu? This needs to be in 50.1.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

My mistake (from landing this yesterday). We should land it for 50.1 too.

Comment 23

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

Hello Tomcat, Wes, could we land this patch on m-r default branch so it's included in 50.1.0 build2? Thanks!

Comment 24

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

Comment on attachment 8808119 [details] [diff] [review]
v2

Approved for landing to m-r for 50.1.0 (hope it doesn't need rebasing)

Comment 25

[Justin Wood (:Callek)]

callek@Centaurus:~/mozilla/hg/mozilla-central$ hg graft -r aae6eef37ab91d972dde6a592eb31d02bf5bfdab
grafting 371441:aae6eef37ab9 "Bug 1287912. r=bz a=lizzard"
merging dom/bindings/BindingUtils.h

callek@Centaurus:~/mozilla/hg/mozilla-central$ hg out -vr . release
comparing with release
searching for changes
changeset:   371522:8612c3320053
tag:         tip
parent:      371412:1a8bbd6f7e21
user:        Peter Van der Beken <[e-mail]>
date:        Wed Nov 23 16:58:59 2016 -0500
files:       dom/bindings/BindingUtils.h dom/bindings/test/chrome.ini dom/bindings/test/test_bug1287912.html
description:
Bug 1287912. r=bz a=lizzard

MozReview-Commit-ID: 6oHD1x4kiE


callek@Centaurus:~/mozilla/hg/mozilla-central$ hg push -r . release
pushing to ssh://hg.mozilla.org/releases/mozilla-release
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 3 changes to 3 files (+1 heads)
remote: recorded push in pushlog
remote: 
remote: View your change here:
remote:   https://hg.mozilla.org/releases/mozilla-release/rev/8612c3320053b796678921f8f23358e3e9df997e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=8612c3320053b796678921f8f23358e3e9df997e
remote: recorded changegroup in replication log in 0.009s