Bug 780370 (CVE-2012-4184)

ChromeObjectWrapper is not implemented as intended

RESOLVED FIXED in Firefox 16

Status

()

Core
Security
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla17
x86
Windows XP
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ wontfix, firefox16+ fixed, firefox17+ fixed, firefox-esr1016+ fixed)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Comment 2

5 years ago
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.
Keywords: sec-high
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.
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.
status-firefox-esr10: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox15: --- → +
tracking-firefox16: --- → +
tracking-firefox17: --- → +
I have patches for this. Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=3d029a5f6c1a
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.
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.
Attachment #654780 - Flags: review?(mrbkap)
Created attachment 654781 [details] [diff] [review]
Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2
Attachment #654781 - Flags: review?(mrbkap)

Updated

5 years ago
tracking-firefox-esr10: ? → ---
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.
Attachment #654780 - Flags: review?(mrbkap) → review+
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.
Attachment #654781 - Flags: review?(mrbkap) → review+
(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.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20a3b0eeb776
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c766aa050f0f
Blocks: 768101
Updating tracking flags, we're look at 16 for this nom for beta/ESR when ready.
status-firefox15: affected → wontfix
tracking-firefox-esr10: --- → 16+
(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
Assignee: nobody → bobbyholley+bmo
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Does the patch apply to ESR?
Keywords: verifyme
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.
Attachment #654781 - Flags: approval-mozilla-esr10?
Attachment #654781 - Flags: approval-mozilla-beta?
Attachment #654781 - Flags: approval-mozilla-esr10?
Attachment #654781 - Flags: approval-mozilla-esr10+
Attachment #654781 - Flags: approval-mozilla-beta?
Attachment #654781 - Flags: approval-mozilla-beta+
Pushed to m-b:
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/647876f95fea
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d9a8bb8a5533
status-firefox16: affected → fixed
Pushed to mozilla-esr10:
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/f61385d44144
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/b9ee5ec9d172
status-firefox-esr10: affected → fixed
Whiteboard: [advisory-tracking+]
Alias: CVE-2012-4184
Group: core-security
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.