Closed Bug 1287912 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

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

People

(Reporter: bzbarsky, Assigned: peterv)

Details

(Keywords: sec-high, Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(2 files, 1 obsolete file)

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...
Flags: needinfo?(peterv)
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.
Keywords: sec-moderate
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.
Flags: needinfo?(continuation)
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...
Flags: needinfo?(continuation)
Keywords: sec-moderatesec-high
> 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...
Assignee: nobody → peterv
Flags: needinfo?(peterv)
Attached patch v1 (obsolete) — Splinter Review
Bz, want to review?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attachment #8807707 - Flags: review-
Attached patch v2Splinter Review
Attachment #8807707 - Attachment is obsolete: true
Sigh, totally misread the section on .prototype in the spec.
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Comment on attachment 8808119 [details] [diff] [review]
v2

r=me
Flags: needinfo?(bzbarsky)
Attachment #8808119 - Flags: review+
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.
Attachment #8808119 - Flags: sec-approval?
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.
Whiteboard: [checkin on 11/22]
Attachment #8808119 - Flags: sec-approval? → sec-approval+
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.
Attachment #8808119 - Flags: approval-mozilla-beta?
Attachment #8808119 - Flags: approval-mozilla-aurora?
[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.
Attachment #8816534 - Flags: approval-mozilla-esr45?
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.
Attachment #8808119 - Flags: approval-mozilla-beta?
Attachment #8808119 - Flags: approval-mozilla-beta+
Attachment #8808119 - Flags: approval-mozilla-aurora?
Attachment #8808119 - Flags: approval-mozilla-aurora+
Comment on attachment 8816534 [details] [diff] [review]
v2 (esr_45 version)

This should make it into ESR 45.6.0, going to build tomorrow.
Attachment #8816534 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
(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.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
My mistake (from landing this yesterday). We should land it for 50.1 too.
Flags: needinfo?(lhenry)
Hello Tomcat, Wes, could we land this patch on m-r default branch so it's included in 50.1.0 build2? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(rkothari)
Flags: needinfo?(cbook)
Comment on attachment 8808119 [details] [diff] [review]
v2

Approved for landing to m-r for 50.1.0 (hope it doesn't need rebasing)
Attachment #8808119 - Flags: approval-mozilla-release+
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 <peterv@propagandism.org>
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
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
Whiteboard: [adv-main50.1+][adv-esr45.6+]
Group: dom-core-security → core-security-release
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.