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

RESOLVED FIXED in Firefox -esr45

Status

()

Core
DOM
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bz, Assigned: peterv)

Tracking

({sec-high})

Trunk
mozilla53
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr4550+ fixed, firefox50 fixed, firefox51+ fixed, firefox52+ fixed, firefox53 fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

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-moderate → sec-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)

Updated

a year ago
Assignee: nobody → peterv
Flags: needinfo?(peterv)
(Assignee)

Comment 5

a year ago
Created attachment 8807707 [details] [diff] [review]
v1
(Assignee)

Comment 6

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

Comment 8

a year ago
Created attachment 8808119 [details] [diff] [review]
v2
Attachment #8807707 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 11

a year ago
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.
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox-esr45: --- → 51+
Whiteboard: [checkin on 11/22]
Attachment #8808119 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6f55d3d0f40784ba1463ed9250eceb04faeff2
Flags: in-testsuite+
Whiteboard: [checkin on 11/22]
(Assignee)

Comment 14

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

Comment 15

a year ago
Created attachment 8816534 [details] [diff] [review]
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.
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+
 https://hg.mozilla.org/releases/mozilla-esr45/rev/778f65148b4009b087c6e7789ffe486a5349922c
status-firefox-esr45: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/3a6f55d3d0f40784ba1463ed9250eceb04faeff2
https://hg.mozilla.org/releases/mozilla-aurora/rev/811d1517145c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/releases/mozilla-beta/rev/aae6eef37ab91d972dde6a592eb31d02bf5bfdab
status-firefox51: affected → fixed
(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)
status-firefox50: affected → fixed
Whiteboard: [adv-main50.1+][adv-esr45.6+]

Updated

11 months ago
Group: dom-core-security → core-security-release

Updated

10 months ago
tracking-firefox-esr45: 51+ → 50+

Updated

10 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.