Last Comment Bug 780370 - (CVE-2012-4184) ChromeObjectWrapper is not implemented as intended
(CVE-2012-4184)
: ChromeObjectWrapper is not implemented as intended
Status: RESOLVED FIXED
[advisory-tracking+]
: sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla17
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks: CVE-2012-3993
  Show dependency treegraph
 
Reported: 2012-08-04 02:42 PDT by moz_bug_r_a4
Modified: 2014-01-10 10:40 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
fixed
16+
fixed


Attachments
testcase 1 - test (688 bytes, text/html)
2012-08-04 02:44 PDT, moz_bug_r_a4
no flags Details
testcase 2 - XrayWrapper pollution (392 bytes, text/html)
2012-08-04 02:46 PDT, moz_bug_r_a4
no flags Details
Part 1 - Clarify the compartment situation in ChromeObjectWrapper. v1 (2.36 KB, patch)
2012-08-23 13:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2 (6.22 KB, patch)
2012-08-23 13:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2012-08-04 02:42:50 PDT
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 2012-08-04 02:44:48 PDT
Created attachment 648979 [details]
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 2012-08-04 02:46:38 PDT
Created attachment 648980 [details]
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) (busy with Stylo) 2012-08-08 11:04:59 PDT
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 2012-08-21 11:50:33 PDT
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) (busy with Stylo) 2012-08-21 14:53:49 PDT
I have patches for this. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=3d029a5f6c1a
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-08-23 13:48:52 PDT
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) (busy with Stylo) 2012-08-23 13:49:39 PDT
Created attachment 654780 [details] [diff] [review]
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) (busy with Stylo) 2012-08-23 13:49:55 PDT
Created attachment 654781 [details] [diff] [review]
Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2
Comment 9 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-08-23 17:45:29 PDT
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) (PTO until Jan. 2, 2017) 2012-08-23 18:00:41 PDT
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) (busy with Stylo) 2012-08-23 18:19:20 PDT
(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 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-24 09:54:15 PDT
Updating tracking flags, we're look at 16 for this nom for beta/ESR when ready.
Comment 15 Al Billings [:abillings] 2012-08-30 13:21:32 PDT
Does the patch apply to ESR?
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-09-04 13:05:25 PDT
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) (busy with Stylo) 2012-09-10 15:47:32 PDT
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) (busy with Stylo) 2012-09-10 16:34:11 PDT
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] 2014-01-10 10:40:39 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.