907727 - (CVE-2013-1737) Accessor user-defined properties on DOM proxies get the wrong "this" object: the expando object

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: 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....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to properly forward gets to the expando object for DOM proxies.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note: no testcase in the patch because the security situation worries me.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Conservatively setting this to "high".

Comment 5

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

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83d003b30be4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83d003b30be4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/638f571cd032
https://hg.mozilla.org/releases/mozilla-beta/rev/7f23a0018f11

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for esr17 approval as well (or prepare a branch-specific patch if need be).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/8236a3be778d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's something branch drivers need to decide...

Comment 14

[Alex Keybl [:akeybl]]

(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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f1dcd2d985f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7f1dcd2d985f

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

That warning has nothing to do with this bug...

Comment 19

[Daniel Veditz [:dveditz]]

(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.