780370 - (CVE-2012-4184) ChromeObjectWrapper is not implemented as intended

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

After bug 760109 landing, content still can access properties of a standard prototype from a chrome scope if __exposedProps__ doesn't exist.

67 ChromeObjectWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver,
68                          jsid id, js::Value *vp)
69 {
70     // Try the lookup on the base wrapper.
71     if (!ChromeObjectWrapperBase::get(cx, wrapper, receiver, id, vp))
72         return false;
73 
74     // If we found something or have no proto, we're done.
75     JSObject *wrapperProto = JS_GetPrototype(wrapper);
76     if (!vp->isUndefined() || !wrapperProto)
77         return true;

ChromeObjectWrapperBase::get lookups the chrome object's prototype chain and can find something on a standard prototype from the chrome scope.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - test

After bug 760109 landing:
o.s1: chrome
o.s2: content
p.s1: content
p.s2: content

Before bug 760109 landing:
o.s1: chrome
o.s2: undefined
p.s1: chrome
p.s2: undefined

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - XrayWrapper pollution

This does a similar thing to what the testcase in bug 768101 comment 0 does.

This works on fx14-17 but does not work on fx10.

Comment 3

[Bobby Holley (:bholley)]

Ah, crap.

The code isn't broken per se, but the issue that moz_bug_r_a4 pointed out means that I was mistaken to suggest it as a solution to bug 768101. It will do the right thing once __exposedProps__ starts being mandatory, but that's only going to happen this cycle.

In order to get the behavior I was imagining, we'll need bug 778085, which offloads the duties of ChromeObjectWrapper into the JS engine and lets us remove ChromeObjectWrapper.{cpp,h} entirely. Those patches will be landing on m-i tomorrow probably once Eddy and I work out a few details.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this bug for 15, though we are going to final beta now, but if there is a chemspill we can look at taking this along.  Also tracking ? for ESR until this lands and we can approve for the appropriate ESR at the time.  This is the rest of the work started/landed in bug 760109.

Comment 5

[Bobby Holley (:bholley)]

I have patches for this. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=3d029a5f6c1a

Comment 6

[Bobby Holley (:bholley)]

These are green except for one bug, which I've fixed and verified it fixes the oranges in that try push. Uploading patches and flagging blake for review.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Clarify the compartment situation in ChromeObjectWrapper. v1

Not sure what I was thinking before. We should always be in the compartment of the wrapper here.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 654780 [details] [diff] [review]
Part 1 - Clarify the compartment situation in ChromeObjectWrapper. v1

Review of attachment 654780 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +57,1 @@
>      {

Nit: the braces go away here.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 654781 [details] [diff] [review]
Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2

Review of attachment 654781 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/tests/unit/test_bug780370.js
@@ +12,5 @@
> +function run_test()
> +{
> +  var sb = Cu.Sandbox("http://www.example.com");
> +  sb.obj = { foo: 42, __exposedProps__: { hasOwnProperty: 'r' } };
> +  do_check_eq(Cu.evalInSandbox('typeof obj.foo', sb), 'undefined', "COW works as expected");

Is it worth adding an Object.prototype.bar in the sandbox and ensuring that the Object.prototype being used is the sandbox's?

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +69,5 @@
>  
>      // Try the prototype if that failed.
>      JS_ASSERT(js::IsObjectInContextCompartment(wrapper, cx));
>      JSPropertyDescriptor desc;
> +    memset(&desc, 0, sizeof(desc));

Why are these memsets needed (here and below).

@@ +84,5 @@
>                           jsid id, js::Value *vp)
>  {
> +    // Start with a call to getPropertyDescriptor. We unfortunately need to do
> +    // this because the call signature of ::get doesn't give us any way to
> +    // determine the object upon which the property was found.

Yeah, this is pretty ugly. One way to fix it would be to expose the guts of BaseProxyHandler::get as a protected function that takes a JSPropertyDescriptor and gets the value using it. For now, I think this is fine, since the double protochain walk isn't detectable (and should be fast enough anyway). If we end up allowing __exposedProps__ on objects on the proto chain though, it might be worth it to clean this up.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #10)

> Is it worth adding an Object.prototype.bar in the sandbox and ensuring that
> the Object.prototype being used is the sandbox's?

Added.

> 
> Why are these memsets needed (here and below).

Discussed IRL.

> Yeah, this is pretty ugly. One way to fix it would be to expose the guts of
> BaseProxyHandler::get as a protected function that takes a
> JSPropertyDescriptor and gets the value using it. For now, I think this is
> fine, since the double protochain walk isn't detectable (and should be fast
> enough anyway). If we end up allowing __exposedProps__ on objects on the
> proto chain though, it might be worth it to clean this up.

I'll file a bug when I get back from vacation.

Comment 12

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20a3b0eeb776
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c766aa050f0f

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Updating tracking flags, we're look at 16 for this nom for beta/ESR when ready.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (:bholley) (on vacation Aug 24 - Sept 3) from comment #12)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20a3b0eeb776
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c766aa050f0f

https://hg.mozilla.org/mozilla-central/rev/20a3b0eeb776
https://hg.mozilla.org/mozilla-central/rev/c766aa050f0f

Comment 15

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

Does the patch apply to ESR?

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 654781 [details] [diff] [review]
Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2

Back from vacation. Looks like this stuck on m-c (which has now also branched to m-a), so it's time to get this onto beta and esr10.

This patch isn't super risky. The bulk of the work happened over in bug 760109, which has now landed everywhere. This just tweaks it a bit. No string or IID changes.

Comment 17

[Bobby Holley (:bholley)]

Pushed to m-b:
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/647876f95fea
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d9a8bb8a5533

Comment 18

[Bobby Holley (:bholley)]

Pushed to mozilla-esr10:
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/f61385d44144
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/b9ee5ec9d172

Comment 19

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old