The default bug view has changed. See this FAQ.
Bug 907727 (CVE-2013-1737)

Accessor user-defined properties on DOM proxies get the wrong "this" object: the expando object

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({sec-moderate})

unspecified
mozilla26
x86
Mac OS X
sec-moderate
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox23 wontfix, firefox24+ verified, firefox25+ verified, firefox26+ verified, firefox-esr1724+ verified, b2g1824+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [still needs a test checked in][adv-main24+][adv-esr1709+])

Attachments

(2 attachments)

Created attachment 793481 [details]
Testcase

If you load the attached testcase, you get an exception when you should get the HTMLOptionElement.  The reason is this callstack:

#11 0x000000010187efd1 in JS_GetPropertyById (cx=0x115e0f640, objArg=0x116190220, idArg={asBits = 4643216832}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at jsapi.cpp:3753
#12 0x0000000106394b20 in mozilla::dom::HTMLSelectElementBinding::DOMProxyHandler::get (this=0x1093d1b40, cx=0x115e0f640, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf9470}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at HTMLSelectElementBinding.cpp:1313
#13 0x0000000101a12889 in js::Proxy::get (cx=0x115e0f640, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf9470}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at jsproxy.cpp:2487

and now we're doing the get on the expando object, and that's what the getter will see.

This is bad, because if you just use a scripted getter you get the expando object as "this"!  Going to mark as security-sensitive for that reason....
Created attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.
Attachment #793522 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Note: no testcase in the patch because the security situation worries me.
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Unclear; I don't know an obvious way this can be exploited.

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

Which older supported branches are affected by this flaw?  I think everything back to b2g18 or so.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I think this patch should apply as-is to the various branches.

How likely is this patch to cause regressions; how much testing does it need?  The main regression risk would be custom getters on Xrays for DOM proxies, which I expect to be rare if they exist at all.
Attachment #793522 - Flags: sec-approval?
Conservatively setting this to "high".
Keywords: sec-high
status-b2g18: --- → affected
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox26: --- → +
tracking-firefox-esr17: --- → ?
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

sec-approval+ for trunk. Approving for aurora and beta after that. I assume Peter still needs to r+ it?
Attachment #793522 - Flags: sec-approval?
Attachment #793522 - Flags: sec-approval+
Attachment #793522 - Flags: approval-mozilla-beta+
Attachment #793522 - Flags: approval-mozilla-aurora+
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding bug in DOM proxy bindings
User impact if declined: Unclear: we have an invariant violation, but I'm not
   sure what can be done with it.
Testing completed: Basic testing of the functionality in webpages
Risk to taking this patch (and alternatives if risky): I think it's low risk. 
   The other option is to just not take it.
String or UUID changes made by this patch: None.
Attachment #793522 - Flags: approval-mozilla-b2g18?
Attachment #793522 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/83d003b30be4
Flags: in-testsuite?
Whiteboard: [still needs a test checked in]
Target Milestone: --- → mozilla26
https://hg.mozilla.org/mozilla-central/rev/83d003b30be4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/638f571cd032
https://hg.mozilla.org/releases/mozilla-beta/rev/7f23a0018f11
status-firefox24: affected → fixed
status-firefox25: affected → fixed
Whiteboard: [still needs a test checked in] → [still needs a test checked in][adv-main24+]
Please nominate for esr17 approval as well (or prepare a branch-specific patch if need be).
tracking-firefox-esr17: ? → 24+
Flags: needinfo?(bzbarsky)
Attachment #793522 - Flags: approval-mozilla-esr17?
Flags: needinfo?(bzbarsky)
Attachment #793522 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
https://hg.mozilla.org/releases/mozilla-esr17/rev/8236a3be778d
status-firefox-esr17: affected → fixed
Whiteboard: [still needs a test checked in][adv-main24+] → [still needs a test checked in][adv-main24+][adv-esr1709+]
Attachment #793522 - Flags: approval-mozilla-b2g18?
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
status-b2g18: affected → wontfix
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
That's something branch drivers need to decide...
Flags: needinfo?(lsblakk)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> b2g18 is frozen for non-leo+ blockers. Please request blocking status if you
> feel that this must land there.

I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.
status-b2g18: wontfix → affected
status-b2g-v1.1hd: wontfix → affected
tracking-b2g18: ? → 24+
Flags: needinfo?(lsblakk)

Updated

4 years ago
Attachment #793522 - Flags: approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f1dcd2d985f
status-b2g18: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7f1dcd2d985f
status-b2g-v1.1hd: affected → fixed
Confirmed bug in m-c 2013-08-22.
Verified fixed in FF24, FF25 and FF26, 2013-09-11.

On FF17.0.9esr only, I see no JS alert box. This seems weird. I do see the following in the error console, which does not appear in FF17.0.8esr:

"Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block."

Is the behavior we desire?
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox26: fixed → verified
That warning has nothing to do with this bug...
(In reply to Andrew McCreight [:mccr8] (mostly PTO through 9/22) from comment #4)
> Conservatively setting this to "high".

if bz doesn't even know for sure if this can be abused (comment 6) that seems too high.
Keywords: sec-high → sec-moderate
Alias: CVE-2013-1737
status-firefox-esr17: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.