720619 - (CVE-2012-4193) Lack of security check for [[DefaultValue]]

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

This is a regression between Firefox 6.0.2 and Firefox 7.  This seems to be a regression from bug 646129.  It seems that we unwrap security wrappers without doing security check in defaultValue().

Security problems:
* It's possible to get cross-origin window's location.href.  (This is related to bug 593602.)
* If a target page has custom window.valueOf/toString methods, it's possible to call those methods.
* It's possible to call valueOf/toString methods on a COW even if valueOf/toString are not readable.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - location

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - window

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - COW

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v1

By calling enter, we do any security checks/other required setup for the wrappers. Note that because some wrappers (i.e. COWs) don't throw on invalid property accesses but simply don't return the functions, we now have to throw ourselves if they don't successfully convert the object to a primitive value.

I'm not entirely sure about the JSDVG_IGNORE_STACK, should it be SEARCH_STACK?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: mochitests

Comment 6

[Luke Wagner [:luke]]

Blake: would an alternative be to override SecurityWrapper::defaultValue?  It makes me wonder, in general, if SecurityWrapper is really just an unnecessary alternative to CHECKED calls in Wrapper.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 591080 [details] [diff] [review]
Proposed fix v1

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

This stuff is not particularly my strong suit, as evidenced by this bug existing at all.  But here goes.

I'm a bit confused why this method, being outside the wrapper's compartment, doesn't just naturally hit all the barriers that prevent access to these properties and all.  Is there something we could do to make that happen?

::: js/src/jswrapper.cpp
@@ +342,4 @@
>      *vp = ObjectValue(*wrappedObject(wrapper));
> +    if (hint == JSTYPE_VOID) {
> +        jsid id = toString;
> +        CHECKED(ToPrimitive(cx, vp), GET);

ToPrimitive doesn't necessarily just use toString; it can also use valueOf as well.  So this is at least insufficient.

It seems to me that perhaps there should be a new type of action for [[DefaultValue]], and enter() implementations should reject such actions unless they were written to handle them.  The hook's not a property-get, a property-set, or a call; it's really (most likely) the union of two of those, isn't it?

@@ +346,5 @@
> +    }
> +
> +    const jsid valueOf = ATOM_TO_JSID(cx->runtime->atomState.valueOfAtom);
> +    jsid id = (hint == JSTYPE_STRING) ? toString : valueOf;
> +    CHECKED(ToPrimitive(cx, hint, vp), GET);

Same here.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

Instead of doing all of the work myself, I realized that DefaultValue is generic enough that it will work on a proxy without any additional coaxing.

Comment 9

[Daniel Veditz [:dveditz]]

Not going to make Fx10 next week, but we should get this into Fx11 even if it means landing early in Beta because it doesn't get Aurora approval before the merge.

Comment 10

[moz_bug_r_a4]

I forgot to play with NoWaiverWrapper.  I'll attach an arbitrary code execution testcase.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v3

This fixes all of the testcases here. It's basically the same as v2, except more. Basically, the regular DefaultValue implementation does the right thing for us (since toString and valueOf should work through wrappers, if they're even accessible).

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 591445 [details] [diff] [review]
Proposed fix v3

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

This would cause cross-compartment objects with non-default convert hooks to not work correctly.

[jwalden@wheres-wally src]$ # current, correct behavior
[jwalden@wheres-wally src]$ dbg/js
js> var g = newGlobal("new-compartment");
js> g.evaluate("Date.prototype.toString = function() { return 'PASS'; }; Date.prototype.valueOf = function() { return 'FAIL'; };")
js> g.evaluate("var d = new Date");
js> g.evaluate("print(d + '');")
PASS
js> print(g.d + '')
PASS
js> 
[jwalden@wheres-wally src]$ # change the proxy_Convert in ObjectProxyClass to JS_ConvertStub...
[jwalden@wheres-wally src]$ make -s -C dbg -j8
jsproxy.cpp
/home/jwalden/moz/slots/js/src/jsproxy.cpp:1222:1: warning: unused function 'proxy_Convert' [-Wunused-function]
proxy_Convert(JSContext *cx, JSObject *proxy, JSType hint, Value *vp)
^
1 warning generated.
[jwalden@wheres-wally src]$ dbg/js
js> var g = newGlobal("new-compartment");
js> g.evaluate("Date.prototype.toString = function() { return 'PASS'; }; Date.prototype.valueOf = function() { return 'FAIL'; };")
js> g.evaluate("var d = new Date");
js> g.evaluate("print(d + '');")
PASS
js> print(g.d + '')
FAIL

I didn't have time to do a full browser build to test, but I bet http://whereswalden.com/files/temp/date.html also doesn't pass with this patch.

I'm pretty sure I wrote a shell test (shell-only, tho, since it uses newGlobal) which would have caught this -- ecma_5/Date/defaultvalue.js.  Did you run shell tests with this patch?

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 591113 [details] [diff] [review]
mochitests

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

Feel free to add the webpage-based Date test I linked in the previous comment as well, if you want.  Although I tend to think it's not necessary given the shell tests.

Do we have any shell-available security-wrapper functionality, so that there could be a shell test as well?  Since this was a JS issue, it'd be better if the test were a regular JS test, as the ability to smoketest using JS tests only, usually with few false positives, is very helpful.

::: js/xpconnect/tests/chrome/test_bug720619.xul
@@ +19,5 @@
> +  <script type="application/javascript"><![CDATA[
> +
> +  /** Test for Bug 720619 **/
> +  const Cu = Components.utils;
> +  function checkThrows(s, sandbox, exception) {

I'd name this exceptionRe or something indicating regexp-ness for readability.

@@ +30,5 @@
> +  }
> +
> +  var sandbox = new Cu.Sandbox("about:blank");
> +  sandbox.v = {
> +    __exposedProps__: {},

Hmm, so you're testing only when there are no properties exposed?  For completeness, I think you should probably test the cases where toString only is exposed, valueOf only is exposed, neither is exposed, and both are exposed.

@@ +35,5 @@
> +    valueOf: function() { return "v"; },
> +    toString: function() { return "s"; }
> +  };
> +  checkThrows("v + ''", sandbox, /convert/);
> +  checkThrows("'' + v", sandbox, /convert/);

So those exercise ToPrimitive with no hint.

@@ +36,5 @@
> +    toString: function() { return "s"; }
> +  };
> +  checkThrows("v + ''", sandbox, /convert/);
> +  checkThrows("'' + v", sandbox, /convert/);
> +  checkThrows("String(v)", sandbox, /convert/);

And that exercises ToPrimitive with hint String.

@@ +37,5 @@
> +  };
> +  checkThrows("v + ''", sandbox, /convert/);
> +  checkThrows("'' + v", sandbox, /convert/);
> +  checkThrows("String(v)", sandbox, /convert/);
> +

So how about covering all the bases and exercising ToPrimitive with hint Number?  "v - 0" should do the trick, I think.

::: js/xpconnect/tests/mochitest/test_bug720619.html
@@ +21,5 @@
> +
> +/** Test for Bug 720619 **/
> +SimpleTest.waitForExplicitFinish();
> +
> +function checkThrows(f, exception) {

Rename this too.

@@ +39,5 @@
> +
> +    var win = $('ifr').contentWindow;
> +    checkThrows(function() {win + '';}, /Permission denied/);
> +    checkThrows(function() {'' + win;}, /Permission denied/);
> +    checkThrows(function() {String(win);}, /Permission denied/);

You should probably add the similar |loc - 0| and |win - 0| cases here as well.

Comment 16

[Daniel Veditz [:dveditz]]

Now that MWC is over can this bug get some love?

Comment 17

[moz_bug_r_a4]

Testcase 4 no longer works due to the fix in bug 723808.  I'll attach a new testcase.

Comment 19

[Alex Keybl [:akeybl]]

Let's wait for this to be fixed on mainline prior to tracking for ESR.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Jeff, Blake is anyone active on finishing up the patch?

Comment 21

[Johnny Stenback (:jst)]

I spoke to Blake about this bug last week and he said we're now in a good state to fix what's left here very easily. Blake left on vacation right after that discussion, so hasn't had a chance to get to this, but he will once he's back mid next week.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Update (finally): I have a patch that seems to work. I'll test it more (and write more tests) tomorrow and get it in here for review.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v4

As the comment added in the patch says, this is sort of odd, but I don't see a way around it because of custom defaultValue hooks.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Mochitest

The content mochitest here exercises bug 751858.

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 621047 [details] [diff] [review]
Proposed fix v4

r=bholley. Sorry for the delay. :-(

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 621049 [details] [diff] [review]
Mochitest

I didn't look at this very closely, but it seems fine.

Comment 27

[Bobby Holley (:bholley)]

Eddy is refactoring this code with direct proxies. We should ensure that these land smoothly with each other.

Comment 28

[Bobby Holley (:bholley)]

Attachment: Fix rebased to trunk. v1 r=bholley

So, I tried to grab this patch to see if it would fix another problem I have, but the above patch is busted for trunk, now that the first bit of direct proxies landed. I've rebased it, and am flagging eddy for an additional review on the interaction with direct proxies (also to get this on his radar).

Comment 29

[Bobby Holley (:bholley)]

Attachment: Fix XPConnect mochitests. v1

So, it turns out that this change kind of sucks. It broke 2 XPConnect mochitests, because you can no longer implicitly toString() chrome objects. But you _can_ call toString() on them. I'm not well-versed in the ways of ES5, but it seems like we could do better here. I'm going to push to try to run the full Mochitest suite, but I'm not optimistic about the result, given that 2 XPConnect tests alone were broken (and that's usually a reasonable benchmark).

What do you think, Blake?

Comment 30

[Bobby Holley (:bholley)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=43bab33d6534

Comment 31

[Bobby Holley (:bholley)]

As predicted, this is bright orange. So I think we need another solution here, blake. :-(

Comment 32

[Bobby Holley (:bholley)]

Given that he understands this stuff well, and that Blake is presumably swamped, I've convinced Eddy to take this bug.

Comment 33

[Eddy Bruel [:ejpbruel]]

I think I figured out the problem with Blake's fix. Assuming that a puncture is understood to be the forwarding of a trap on a wrapper to the corresponding trap on the wrapped object, Wrapper:defaultValue does indeed perform a puncture, *except* when the wrapped object uses JS_ConvertStub as its defaultValue trap. In that case, ToPrimitive falls back to the DefaultValue function, which attempts to obtain a value using the toString/valueOf methods on the object.

In other words, I think we can make this work by only calling enter/leave for the defaultValue trap if the wrapped object does *not* use JS_ConvertStub as its defaultValue trap.

Comment 34

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed fix

Here's my proposed fix. It's currently running on try (https://tbpl.mozilla.org/?tree=Try&rev=42c03c529295).

Let's see how much this breaks. In the meantime, thoughts and/or feedback would be welcome.

Comment 35

[Eddy Bruel [:ejpbruel]]

Ok, so obviously this isn't smart enough. The entire tree still turns orange. I'd like to know *why* most of the tests are failing. Are they doing something they're not supposed to be doing? Or are we disallowing something we're supposed to allow?

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

The problem is that XPConnect installs a custom convert hook on all objects and location objects never permit PUNCTURE access, therefore the reduced testcase should be: |location + ''| (which shouldn't throw, but will).

bholley, do you see a solution here better than simply adding a new type of security flag CONVERT, as opposed to PUNCTURE, where we can do more correct checking for location objects?

Comment 37

[Eddy Bruel [:ejpbruel]]

(In reply to Blake Kaplan (:mrbkap) from comment #36)
> The problem is that XPConnect installs a custom convert hook on all objects
> and location objects never permit PUNCTURE access, therefore the reduced
> testcase should be: |location + ''| (which shouldn't throw, but will).
> 
> bholley, do you see a solution here better than simply adding a new type of
> security flag CONVERT, as opposed to PUNCTURE, where we can do more correct
> checking for location objects?

I'd like to think about a solution as well. Where and why does xpconnect does this?

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Oops, clearly you're allowed to answer my question :-)

See XPC_WN_{Shared,Helper}_Convert in js/xpconnect/src/XPCWrappedNativeJSOps.cpp for the gory details. Basically, there's some ugly XPConnect stuff that needs to be handled in a couple of the cases, so XPConnect always overrides it. For the string case, XPConnect simply calls ToString on the underlying object.

Comment 39

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #36)
> bholley, do you see a solution here better than simply adding a new type of
> security flag CONVERT, as opposed to PUNCTURE, where we can do more correct
> checking for location objects?

That seems reasonable to me, though I've never really looked at the guts of the _Convert hooks. Maybe eddy can come up with something more clever.

Our number of wrapper Actions is also becoming somewhat unwieldy. It would be nice to refactor that stuff somehow (even just using a switch statement with a default MOZ_ASSERT to make sure we never miss handling one). Though really, we pretty much want the same Convert policy for any filtering wrapper AFAICT...

Comment 40

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #39)
> (In reply to Blake Kaplan (:mrbkap) from comment #36)
> > bholley, do you see a solution here better than simply adding a new type of
> > security flag CONVERT, as opposed to PUNCTURE, where we can do more correct
> > checking for location objects?
> 
> That seems reasonable to me, though I've never really looked at the guts of
> the _Convert hooks. Maybe eddy can come up with something more clever.
> 
> Our number of wrapper Actions is also becoming somewhat unwieldy. It would
> be nice to refactor that stuff somehow (even just using a switch statement
> with a default MOZ_ASSERT to make sure we never miss handling one). Though
> really, we pretty much want the same Convert policy for any filtering
> wrapper AFAICT...

First of all, the reason why we have a PUNCTURE action is mainly for cloning. When cloning a wrapped object, the clone algorithm needs to asking permission for a PUNCTURE, which is essentially the same as asking for permission to unwrap the wrapped object.

To my knowledge, none of the convert hooks have any side effects, so from a security point of view, our only concern is that we might leak information about a wrapped object by getting its default value. In a sense, a CONVERT action is a weaker form of PUNCTURE; it doesn't allows us to obtain the wrapped object itself, but does allow us to obtain a string representation of it.

A major subtlety, however, is that we can also obtain a string representation of a wrapped object by calling its toString/valueOf method, and this fallback approach is used by the convert trap of Wrapper when the wrapped object doesn't has a convert trap it cannot forward to (i.e JS_ConvertStub).

So, even if we use a separate action type, like CONVERT, to govern our policy for the convert trap, if the wrapped object is another Wrapper, we should only disallow calling the convert trap if we know that it is not going to use the above fallback approach. Otherwise, we would end up disallowing something that would be allowed had we just done a GET and CALL to invoke the toString/valueOf method directly.

This is what my patch was supposed to solve, but I'm afraid the situation is even more complicated than that. If I understood Blake correctly, the presence of JS_ConvertStub alone is sufficient, but not necessary to imply that the fallback approach will be taken. Some wrappers in XPConnect implement a custom convert trap, that nevertheless ends up calling toString/valueOf in several cases. So neither CONVERT of PUNCTURE is an appropriate action in this case, and we cannot call toString/valueOf directly either, because the convert hook might do special stuff like maintaining object maps on top of these calls.

So what we really need, in my opinion, is a way to tell whether or not the convert trap will return a value that could not have been obtained by calling toString/valueOf directly.

For instance, we could change the convert hook's signature so that it can report whether it did anything special or not. In Wrapper::defaultValue, we would then call the convert hook first, and only check if the call was allowed *afterwards*. If it was not, and the wrapper did something special, do not allow the result of the call to escape the wrapper. Obviously, this is only safe if the convert hook doesn't have any side effects. Mainly for this reason, Blake didn't like this approach.

Alternatively, we could leave the signature of the convert hook unchanged, and compare the result of calling the convert hook with that of calling toString/valueOf. If the results differ, *and* the call to the convert hook was allowed, do not allow the result of the call to escape the wrapper. Again, this is only safe if the convert hooks don't have any side effects.

Another approach might be to call the convert hook twice (or split it into two functions). The first call would allow the wrapped object to report back what approach its convert hook will take, based on its current state, but would otherwise not have any side effects.

Based on the result of the first call, we would then decide whether to do an access check for the convert hook. If the convert hook simply calls toString/valueOf + some bookkeeping, we don't have to any access checks. In all other cases, we do. The second call would then perform the actual conversion. The main drawback here is that we would have to change the signature and behavior for every convert hook.

That's all I got so far. However, Ihave to say that if it turns out that the convert trap doesn't have any side effects, this whole bug seems like massive overkill to me. XPConnect already seems to handle stuff like illegal conversions and not leaking information via its custom convert traps, so the need for a policy based access mechanism for this hook is not clear to me. Maybe I'm wrong. If that's the case, please explain to me why :-)

Comment 41

[Bobby Holley (:bholley)]

It seems to me that if the problem is XPConnect's use of a custom convert stub, we should fix the problem by refactoring that code so that it can tell us what it's going to do. Then, when we get the CONVERT wrapper action in XPConnect, we check for the custom XPConnect convert stub, and if so, call into the relevant code to ask what it's about to do.

Comment 42

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #41)
> It seems to me that if the problem is XPConnect's use of a custom convert
> stub, we should fix the problem by refactoring that code so that it can tell
> us what it's going to do. Then, when we get the CONVERT wrapper action in
> XPConnect, we check for the custom XPConnect convert stub, and if so, call
> into the relevant code to ask what it's about to do.

And how do you propose that we refactor the convert stub so that it can tell us what it is going to do before it is going to do it? I gave a couple of suggestions, but would like to hear your thoughts on the matter.

Comment 43

[Bobby Holley (:bholley)]

(In reply to Eddy Bruel [:ejpbruel] from comment #42)
> And how do you propose that we refactor the convert stub so that it can tell
> us what it is going to do before it is going to do it? I gave a couple of
> suggestions, but would like to hear your thoughts on the matter.

I'm not proposing changing the signature of JS_ConvertStub (which was my interpretation of your suggestions in comment 40). I'm saying we specifically check if the stub is XPC_WN_Shared_Convert in the policy trap. If it is, then we either verify that it has no side effects, or introduce a separate helper function that will tell us when XPC_WN_Shared_Convert will and will not have side-effects.

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Eddy, any thoughts on comment 43?

Comment 45

[Eddy Bruel [:ejpbruel]]

(In reply to David Bolter [:davidb] from comment #44)
> Hi Eddy, any thoughts on comment 43?

It's a terrible hack, but I don't have any better solution. In my opinion, if this were an ideal world, XPConnect shouldn't override the DefaultValue trap to begin with, so this problem wouldn't even exist. But seeing how that's not realistic, I'd say that bholley's suggestion is probably the way to go.

I can push a patch with the proposed fix to try early next week.

Comment 46

[Eddy Bruel [:ejpbruel]]

Did some brainstorming with bholley today, and decided to try another approach.

The latest idea is to simply always forward the convert trap to toString/valueOf for wrappers. In theory, this should work for everything except the Date object, for which we can write some special case code. This isn't very neat, since, as I understand it, the Date object is the reason we have the defaultValue trap in the first place, but if it works, its at least cleaner than the previous proposal.

Also note that that the DefaultValue algorithm calls toString/valueOf on the wrappee, not on the wrapper. Either this has to be changed, or we need to roll our own.

Comment 47

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed fix

Currently running on try:
https://tbpl.mozilla.org/?tree=Try&rev=a46789935f72

Comment 48

[Eddy Bruel [:ejpbruel]]

Well, that didn't work either. All mochitests are still orange.

The last attempt was a rather blind one. We assumed it would work, but I don't really understand why it doesn't. Understanding that might help in coming up with a solution that *does* work.

Almost all errors seem to be similar in nature:
9505 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug331959.html | Exception caught in waitForEvent: Permission denied to access object, at: http://mochi.test:8888/tests/SimpleTest/SimpleTest.js (528)

bholley, blake, any ideas on how this could be caused by the changes in defaultValue introduced by my latest patch?

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Eddy, that last patch doesn't seem to implement comment 46. Did you attach the wrong patch/push the wrong patch to try?

Comment 50

[Eddy Bruel [:ejpbruel]]

(In reply to Blake Kaplan (:mrbkap) from comment #49)
> Eddy, that last patch doesn't seem to implement comment 46. Did you attach
> the wrong patch/push the wrong patch to try?

The idea as described in comment 46 wouldn't have worked because, according to waldo, some plugins rely on custom defaultValue traps as well. Recall that our backup plan, which isn't mentioned in the comment, was to only forward the convert trap to toString/valueOf for wrappers if a PUNCTURE failed, which is what this patch intended to do.

The first two lines in defaultValue are probably redundant; they are meant to check whether calling IndirectProxyHandler::defaultValue could potentially cause a puncture. If the convert trap of the wrapped object is JS_ConvertStub, we don't have to check if a PUNCTURE is allowed, otherwise we do.

The next few lines are used to perform the PUNCTURE. If succesful, we call IndirectProxyHandler::defaultValue after all, and proceed as we would have before. If *not* succesful, we fall back to calling js::DefaultValue, which never calls the convert trap: it either calls toString or valueOf to produce the value of the object, which is exactly the behavior we want.

Afaict, this is the behavior we wanted. Am I missing something obvious?

Comment 51

[Bobby Holley (:bholley)]

Did you remember to clear the pending exception set by the failed PUNCTURE?

Comment 52

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #51)
> Did you remember to clear the pending exception set by the failed PUNCTURE?

Ah! I wasn't aware that a failed PUNCTURE would cause an exception to be set at that point. I assume these  are set by the implementation of enter in XPConnect? How does one clear a pending exception in SpiderMonkey?

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

JS_ClearPendingException.

Comment 54

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

This patch looks like it might do the trick.

Here are the results from try (note that the comments in the patch have been changed):
https://tbpl.mozilla.org/?tree=Try&rev=8dc5027676e

Comment 55

[Bobby Holley (:bholley)]

Comment on attachment 638770 [details] [diff] [review]
Patch to be reviewed

>diff -r 4c2ddc60f360 js/src/jswrapper.cpp

>+bool
>+DirectWrapper::defaultValue(JSContext *cx, JSObject *wrapper_, JSType hint, Value *vp)

If you're going to root wrapper_, might as well do it at the top of the function.

>+{
>+    /*
>+     * IndirectProxtyHandler::defaultValue trap only performs a puncture if the

Typo. And this doesn't make sense to me given what this patch does.

>+     * convert callback of the wrapped object is *not* JS_ConvertStub.

Nit - no stars around the not!

>+     */
>+    if (wrappedObject(wrapper_)->getClass()->convert == JS_ConvertStub)
>+        return IndirectProxyHandler::defaultValue(cx, wrapper_, hint, vp);
>+    bool status;
>+    if (!enter(cx, wrapper_, JSID_VOID, PUNCTURE, &status)) {
>+        /*
>+         * If the puncture failed, instead of throwing an exception, fallback to
>+         * calling the DefaultValue algorithm on wrapper (*not* the wrappee),
>+         * which tries to obtain a default value by calling toString/valueOf.
>+         */
>+        JS_ClearPendingException(cx);
>+        RootedObject wrapper(cx, wrapper_);
>+        return DefaultValue(cx, wrapper, hint, vp);
>+    }
>+    bool ok = (IndirectProxyHandler::defaultValue(cx, wrapper_, hint, vp));

This call is duplicated. Seems simple enough to avoid by making the enter() call conditional on the convert hook being different than JS_ConvertStub.

We also need this code on IndirectWrapper, right? I've been thinking, do we actually have a use for IndirectWrapper in the current code? I think everybody using it could just use IndirectProxyHandler (i.e., nobody is using the policy enforcement traps). As such, maybe we should just kill it so that it's not a foot-gun with fixes like this?

Also, this needs a big comment explaining what's going on here.

Comment 56

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #55)
> Comment on attachment 638770 [details] [diff] [review]
> Patch to be reviewed
> 
> >diff -r 4c2ddc60f360 js/src/jswrapper.cpp
> 
> >+bool
> >+DirectWrapper::defaultValue(JSContext *cx, JSObject *wrapper_, JSType hint, Value *vp)
> 
> If you're going to root wrapper_, might as well do it at the top of the
> function.
> 
> >+{
> >+    /*
> >+     * IndirectProxtyHandler::defaultValue trap only performs a puncture if the
> 
> Typo. And this doesn't make sense to me given what this patch does.
> 
> >+     * convert callback of the wrapped object is *not* JS_ConvertStub.
> 
> Nit - no stars around the not!
> 
> >+     */
> >+    if (wrappedObject(wrapper_)->getClass()->convert == JS_ConvertStub)
> >+        return IndirectProxyHandler::defaultValue(cx, wrapper_, hint, vp);
> >+    bool status;
> >+    if (!enter(cx, wrapper_, JSID_VOID, PUNCTURE, &status)) {
> >+        /*
> >+         * If the puncture failed, instead of throwing an exception, fallback to
> >+         * calling the DefaultValue algorithm on wrapper (*not* the wrappee),
> >+         * which tries to obtain a default value by calling toString/valueOf.
> >+         */
> >+        JS_ClearPendingException(cx);
> >+        RootedObject wrapper(cx, wrapper_);
> >+        return DefaultValue(cx, wrapper, hint, vp);
> >+    }
> >+    bool ok = (IndirectProxyHandler::defaultValue(cx, wrapper_, hint, vp));
> 
> This call is duplicated. Seems simple enough to avoid by making the enter()
> call conditional on the convert hook being different than JS_ConvertStub.
> 
> We also need this code on IndirectWrapper, right? I've been thinking, do we
> actually have a use for IndirectWrapper in the current code? I think
> everybody using it could just use IndirectProxyHandler (i.e., nobody is
> using the policy enforcement traps). As such, maybe we should just kill it
> so that it's not a foot-gun with fixes like this?
> 
> Also, this needs a big comment explaining what's going on here.

Well, the leave call needs to be conditional too. So I have to either replicate the call or the check. Do you prefer the latter?

If nobody is using IndirectWrapper, we should get rid of it imho. We can always add it back again later.

Comment 57

[Bobby Holley (:bholley)]

I think you can add two levels of conditional and then you only need a single leave call. But either way. Make the leave() conditional, since I think we actually don't need it anymore. File a bug on ripping it and IndirectWrapper out if you get the chance? :-)

Comment 58

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

This patch addresses your earlier comments.

Comment 59

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

D'oh. I forgot to add the same function for AbstractWrapper/IndirectWrapper. This patch correct that oversight.

Comment 60

[Eddy Bruel [:ejpbruel]]

Follow up bug to remove AbstractWrapper:
https://bugzilla.mozilla.org/show_bug.cgi?id=771907

Comment 61

[Eddy Bruel [:ejpbruel]]

Followup bug to remove Wrapper::leave
https://bugzilla.mozilla.org/show_bug.cgi?id=771908

Comment 62

[Bobby Holley (:bholley)]

Comment on attachment 640065 [details] [diff] [review]
Patch to be reviewed

Per IRC discussion, I don't think it's safe to special-case JS_ConvertStub, and this needs tests.

Comment 63

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Tests will be added in a subsequent patch.

Comment 64

[Bobby Holley (:bholley)]

Comment on attachment 640179 [details] [diff] [review]
Patch to be reviewed

>+/*
>+ * Ordinarily, the convert trap would require a PUNCTURE. However, the default
>+ * implementation of convert, JS_ConvertStub, obtains a default value by calling
>+ * the toString/valueOf method on the wrapper, if any. Doing a PUNCTURE in this
>+ * case would be overly conservative. To make matters worse, XPConnect sometimes
>+ * installs a custom convert trap that obtains a default value by calling the
>+ * toString method on the wrapper. Doing a puncture in this case would be overly
>+ * conservative as well. We deal with these anomaly

anomalies

by clearing the pending
>+ * exception and falling back to the DefaultValue algorithm whenever the
>+ * PUNCTURE fails.

I find this comment a bit confusing, and think it could be simplified to:

"The most transparent behavior would be to call DefaultValue on the wrapped object. But this can involve running code on the wrappee, as well as other behavior if the object implements a custom convert stub. So we attempt a PUNCTURE. If it succeeds, we transparently forward the call to the wrappee. If not, we clear the pending exception and call DefaultValue on the wrapper, which usually just ends up invoking .toString() or .toPrimitive()."


>+bool
>+DirectWrapper::defaultValue(JSContext *cx, JSObject *wrapper_, JSType hint,
>+                            Value *vp)
>+{
>+    RootedObject wrapper(cx, wrapper_);
>+
>+    if (!enter(cx, wrapper_, JSID_VOID, PUNCTURE, &status)) {
>+        JS_ClearPendingException(cx);
>+        return DefaultValue(cx, wrapper, hint, vp);
>+    }
>+    ok = IndirectProxyHandler::defaultValue(cx, wrapper_, hint, vp);
>+    leave(cx, wrapper_);

Pass |wrapper| for these two calls.

r=bholley with those changes, and a test (this shouldn't land without a working test, even if we decide not to check it in immediately).

Comment 65

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

Any updates on getting this in?

Comment 66

[Eddy Bruel [:ejpbruel]]

(In reply to Al Billings [:abillings] from comment #65)
> Any updates on getting this in?

I still need to write some xpcshell tests before we can land, but have been preoccupied with other stuff. Sorry. I'll get to it ASAP.

Comment 67

[Eddy Bruel [:ejpbruel]]

Ok so the good news is Blake already wrote a mochitest for this bug a while ago. The bad news is that my patch doesn't make this test green. Tracing reveals that calling the defaultValue trap on a CCW ends up in IndirectWrapper::defaultValue, after which the call to enter fails (as expected). We then fall back to calling DefaultValue on the wrapper, which *should* result in a call to toString. Shouldn't this trigger the exception that the test is looking for? Or is the test testing for the wrong thing?

Comment 68

[Bobby Holley (:bholley)]

(In reply to Eddy Bruel [:ejpbruel] from comment #67)
> Ok so the good news is Blake already wrote a mochitest for this bug a while
> ago. The bad news is that my patch doesn't make this test green. Tracing
> reveals that calling the defaultValue trap on a CCW ends up in
> IndirectWrapper::defaultValue, after which the call to enter fails (as
> expected). We then fall back to calling DefaultValue on the wrapper, which
> *should* result in a call to toString. Shouldn't this trigger the exception
> that the test is looking for? Or is the test testing for the wrong thing?

Well, our new solution opts for various fallbacks and silent failures, right? It's possible that Blake's test was just testing for the kind of aggressive throwing behavior that we had to cut out. Trace through the call and see what ends up happening? Ultimately, we just want to make sure that the wrapper isn't circumvented. We don't so much care if it throws.

Comment 69

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Eddy Bruel [:ejpbruel] from comment #67)
> *should* result in a call to toString. Shouldn't this trigger the exception
> that the test is looking for? Or is the test testing for the wrong thing?

The test is now testing for the wrong thing. It assumes that it should throw because it has an empty __exposedProps__ (and therefore doesn't expose either toString or valueOf) but because of bug 760109, we'll now pick up toString out of the content compartment. So yeah, comment 68 is right on.

Comment 70

[Eddy Bruel [:ejpbruel]]

(In reply to Blake Kaplan (:mrbkap) from comment #69)
> (In reply to Eddy Bruel [:ejpbruel] from comment #67)
> > *should* result in a call to toString. Shouldn't this trigger the exception
> > that the test is looking for? Or is the test testing for the wrong thing?
> 
> The test is now testing for the wrong thing. It assumes that it should throw
> because it has an empty __exposedProps__ (and therefore doesn't expose
> either toString or valueOf) but because of bug 760109, we'll now pick up
> toString out of the content compartment. So yeah, comment 68 is right on.

Let me make sure I understand you correctly: it looks to me as if the chrome test shouldn't throw anymore, since it can now pick up toString out of the content compartment. However, the content test should still throw, since it cannot access toString either (at least, thats the behavior im seeing right now with my patch applied)

If the above is correct, it looks like this patch is ready to go. I ran into one more assertion because DefaultValue was called from the wrong compartment if DirectWrapper::defaultValue is called from CrossCompartmentWrapper::defaultValue (it needs to be the compartment of the wrapper, not the wrappee). Adding a call to AutoCompartment::enter resolved that issue.

Comment 71

[Bobby Holley (:bholley)]

(In reply to Eddy Bruel [:ejpbruel] from comment #70)
> (In reply to Blake Kaplan (:mrbkap) from comment #69)
> > (In reply to Eddy Bruel [:ejpbruel] from comment #67)
> > > *should* result in a call to toString. Shouldn't this trigger the exception
> > > that the test is looking for? Or is the test testing for the wrong thing?
> > 
> > The test is now testing for the wrong thing. It assumes that it should throw
> > because it has an empty __exposedProps__ (and therefore doesn't expose
> > either toString or valueOf) but because of bug 760109, we'll now pick up
> > toString out of the content compartment. So yeah, comment 68 is right on.
> 
> Let me make sure I understand you correctly: it looks to me as if the chrome
> test shouldn't throw anymore, since it can now pick up toString out of the
> content compartment. However, the content test should still throw, since it
> cannot access toString either (at least, thats the behavior im seeing right
> now with my patch applied)

Yes. We only currently do the standard prototype remapping for chrome objects exposed to content.

> If the above is correct, it looks like this patch is ready to go. I ran into
> one more assertion because DefaultValue was called from the wrong
> compartment if DirectWrapper::defaultValue is called from
> CrossCompartmentWrapper::defaultValue (it needs to be the compartment of the
> wrapper, not the wrappee). Adding a call to AutoCompartment::enter resolved
> that issue.

Sounds reasonable, but I think such a change needs review.

Comment 72

[Bobby Holley (:bholley)]

To test things for chrome, try overriding Object.toString() in the chrome compartment and make sure that you see the effects when doing DefaultValue operations in the chrome scope but not in the content scope.

Comment 73

[Bobby Holley (:bholley)]

Eddy, any progress on this? Sounds like we're real close.

Comment 74

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

I'm sorry this took as long as it did. I was preoccupied with landing direct proxies, so I had lost track of this bug.

This patch contains a working solution + tests

Comment 75

[Eddy Bruel [:ejpbruel]]

Blake, I think Bobby is still on vacation, so if you have time, could you take a look at this patch?

I'll be on vacation next week, so I wont be able to land this patch until the week after that. I've pushed the patch to try to make sure there are no more oranges. If you feel like landing it for me, by all means go ahead.

Try link (results still pending at the time of writing)
https://tbpl.mozilla.org/?tree=Try&rev=6f11d8723055

Comment 76

[Bobby Holley (:bholley)]

Comment on attachment 657246 [details] [diff] [review]
Patch to be reviewed

The try push has some oranges, so I'm canceling review.

10649 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_bug720619.xul | undefined
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_badargs.js | an unexpected uncaught JS exception reported through window.onerror - Error:  at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_badargs2.js | an unexpected uncaught JS exception reported through window.onerror - Error:  at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_localfile2.js | an unexpected uncaught JS exception reported through window.onerror - Error:  at :0

Comment 78

[Daniel Veditz [:dveditz]]

Is this patch back-portable, or does it depend on other recent changes? We'd like to make sure it's fixed on the upcoming ESR-17, and ideally on ESR-10 since that's still supported.

Comment 79

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

I don't know what caused those last 3 oranges, but I don't think it was my patch. The first orange was a simple mistake on my part. Here's a new patch, with corresponding try run (all green, from the looks of it):
https://tbpl.mozilla.org/?tree=Try&rev=2b1a27eafe38

Comment 80

[Bobby Holley (:bholley)]

Comment on attachment 660172 [details] [diff] [review]
Patch to be reviewed

Not super happy about the duplication here, but I guess we'll fix that when we merge the wrapper stuff back into the ProxyHandler hierarchy.

r=bholley

Comment 81

[Eddy Bruel [:ejpbruel]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/634a2b9859ab

*flees the scene*

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/634a2b9859ab

Comment 83

[Alex Keybl [:akeybl]]

Please nominate for aurora approval when ready. Wontfixing for FF16 given how old this bug is, and how close we are to release.

Comment 84

[Bobby Holley (:bholley)]

Eddy, what's the holdup on getting this on branch?

Comment 85

[Bobby Holley (:bholley)]

We're chemspilling for this. I'm getting this ready for beta and release now. :-(

Comment 86

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-beta/rev/259f9f2f7380

Comment 88

[Bobby Holley (:bholley)]

Attachment: mozilla-release patch. v1

Added a patch for mozilla-release. Flagging bz and eddy for review.

Comment 89

[Bobby Holley (:bholley)]

Attachment: Squelch exceptions better. v1

Adding a patch to fix mochitest-o failures (applies to both m-b and the patch for m-r). Flagging bz for review.

Comment 90

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 670051 [details] [diff] [review]
mozilla-release patch. v1

r=me

Comment 91

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 670052 [details] [diff] [review]
Squelch exceptions better. v1

r=me

Comment 92

[Bobby Holley (:bholley)]

Bustage fix on m-b: https://hg.mozilla.org/releases/mozilla-beta/rev/542992e8cf80

Comment 93

[Bobby Holley (:bholley)]

Pushed to release:

remote:   https://hg.mozilla.org/releases/mozilla-release/rev/780dd2c6d53a
remote:   https://hg.mozilla.org/releases/mozilla-release/rev/7789ab68a99d

Comment 94

[Devdatta Akhawe [:devd]]

I wonder if Gareth Hayes made this public: http://www.thespanner.co.uk/2012/10/10/firefox-knows-what-your-friends-did-last-summer/

Comment 95

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://bugzilla.mozilla.org/show_bug.cgi?id=799952#c0

Comment 96

[u279076]

QA is trying to verify this is fixed. 

Testcase 1, 2 and 3 work as expected. 
Testcase 4 we assume to be broken as per comment 17. 
Testcase 5 we can't seem to get working. It reports the same thing to error console in Firefox 17b1#1 and 17b1#2. Can we get some explanation of the testcase #5?  Thank you

Comment 97

[Robert Kaiser]

(In reply to Devdatta Akhawe from comment #94)
> I wonder if Gareth Hayes made this public:
> http://www.thespanner.co.uk/2012/10/10/firefox-knows-what-your-friends-did-
> last-summer/

Yes, that's why bug 799952 was filed and why this has become a chemspill manner within hours.

Comment 98

[juan becerra [:juanb]]

I verified this on Linux and Mac with the test cases here.

On a 16.0 build I could see the problem using testcase 1, 2 and 3.
On a 16.0.1 candidate builds I did not see the problem.

On a build prior to the bug fix referenced in comment #17 (Fx10) I could also see the problem in testcase 4 and 5. I did not see these problems in later builds nor in the 16.0.1 candidates.

I'll check WinXP once those builds are out before marking as verified for Fx16.

Comment 99

[juan becerra [:juanb]]

Verified on Windows XP as well. Flipping the status-firefox16 flag.

Comment 101

[u279076]

Verified fixed with all attached testcases in Firefox 17.0b1#2 on Ubuntu 12.04, Mac OSX 10.7, and Windows XP.

Juan, before signing off enabling of Aurora updates on Friday, can you please verify this against Firefox 18.0a2 builds?

Comment 102

[Daniel Veditz [:dveditz]]

Testcase 4 broke due to the fix for bug 723808 in Firefox 11/10.0.3

Testcase 5 broke somewhere else along the way. In Firefox 15 I get
  Error: Exposing chrome JS objects to content without __exposedProps__ is
       insecure and deprecated.
  Error: TypeError: opener.wrappedJSObject is undefined

In both cases the underlying bug is still there, it's just some particular behaviors the exploits were relying on changed.

Comment 103

[Huzaifa Sidhpurwala]

In firefox 10.0.8-ESR, only test cases 5 works and i see the following:

http://huzaifas.fedorapeople.org/Screenshot.png

Comment 105

[Huzaifa Sidhpurwala]

Are we expecting an ESR commit soon?

Atleast it would allow people to backport, if 10.0.9 is going to take some time :)

Comment 107

[Bobby Holley (:bholley)]

(In reply to Huzaifa Sidhpurwala from comment #105)
> Are we expecting an ESR commit soon?
> 
> Atleast it would allow people to backport, if 10.0.9 is going to take some
> time :)

I am preparing an ESR10 commit right now.

Comment 108

[Bobby Holley (:bholley)]

Attachment: ESR10 Part 1 - Run the [[DefaultValue]] algorithm on the wrapper if subsumes fails (esr10). v1

This is the backport of the fix landed elsewhere to ESR10. The PUNCTURE machinery
unfortunately doesn't exist there, but this was back in the day when you could still
call subsume() from the JS engine, so we can work with that. Note also that we don't
necessarily get the perfect object principals here (given that this is pre-CPG), but
they should be at least same-origin with the correct principals, so this should be ok.

bz, given that mrbkap is on PTO, I think you're the best reviewer here.

Comment 109

[Bobby Holley (:bholley)]

Attachment: ESR10 Part 2 - Always call the enter() trap for [[DefaultValue]] (on esr10). v1

This fixes testcase 5.

Comment 110

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 670320 [details] [diff] [review]
ESR10 Part 1 - Run the [[DefaultValue]] algorithm on the wrapper if subsumes fails (esr10). v1

r=me

Comment 111

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 670321 [details] [diff] [review]
ESR10 Part 2 - Always call the enter() trap for [[DefaultValue]] (on esr10). v1

r=me, I guess, though I have no idea what's really going on here, so Eddy's review is what really matters.  ;)

Comment 112

[Andrew McCreight [:mccr8]]

Over IRC, luke said "in that case, the esr10 patch looks good to me in my weak opinion", so I'm counting that as a review.

https://hg.mozilla.org/releases/mozilla-esr10/rev/ebffbd29624c
https://hg.mozilla.org/releases/mozilla-esr10/rev/863bac88c122

Comment 113

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #111)
> r=me, I guess, though I have no idea what's really going on here, so Eddy's
> review is what really matters.  ;)

For posterity, the issue here is that we need to call the enter()/leave() traps when going from chrome to content so that the misnamed CrossOriginWrapper can push/pop a principal.

Comment 114

[Bobby Holley (:bholley)]

Followup bustage fix. r=jmaher on IRC

https://hg.mozilla.org/releases/mozilla-esr10/rev/9806d9532ce3

Comment 115

[u279076]

Verified this fixed for ESR10 using the attached testcases and the just available 10.0.9esr candidates on Windows XP, Mac OSX 10.7, and Ubuntu 12.04.

Comment 116

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #115)
> Verified this fixed for ESR10 using the attached testcases and the just
> available 10.0.9esr candidates on Windows XP, Mac OSX 10.7, and Ubuntu 12.04.

For completeness, I've now verified this in Firefox 18.0b1. Marking this verified fixed.