moz_bug_r_a4 Reporter
	Description • 13 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 4 • 13 years ago

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?

	Luke Wagner [:luke]
	Comment 6 • 13 years ago

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.

	Jeff Walden [:Waldo]
	Comment 7 • 13 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 8 • 13 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 9 • 13 years ago

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.

	moz_bug_r_a4 Reporter
	Comment 10 • 13 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 11 • 13 years ago

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

	Jeff Walden [:Waldo]
	Comment 12 • 13 years ago

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?

	Jeff Walden [:Waldo]
	Comment 13 • 13 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 14 • 13 years ago

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

	moz_bug_r_a4 Reporter
	Comment 15 • 13 years ago

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

	Alex Keybl [:akeybl]
	Comment 16 • 13 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 17 • 13 years ago

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

	Johnny Stenback (:jst)
	Comment 18 • 13 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 19 • 12 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 20 • 12 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 21 • 12 years ago

The content mochitest here exercises bug 751858.

	Bobby Holley (:bholley)
	Comment 22 • 12 years ago

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

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

	Bobby Holley (:bholley)
	Comment 23 • 12 years ago

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

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

	Bobby Holley (:bholley)
	Comment 24 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 25 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 26 • 12 years ago

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?

	Bobby Holley (:bholley)
	Comment 27 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 28 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 29 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 30 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 31 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 32 • 12 years ago

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?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 33 • 12 years ago

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?

	Eddy Bruel [:ejpbruel] Assignee
	Comment 34 • 12 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 35 • 12 years ago

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.

	Bobby Holley (:bholley)
	Comment 36 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 37 • 12 years ago

(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 :-)

	Bobby Holley (:bholley)
	Comment 38 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 39 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 40 • 12 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 41 • 12 years ago

Hi Eddy, any thoughts on comment 43?

	Eddy Bruel [:ejpbruel] Assignee
	Comment 42 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 43 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 44 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 45 • 12 years ago

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?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 46 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 47 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 48 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 49 • 12 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 50 • 12 years ago

JS_ClearPendingException.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 51 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 52 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 53 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 54 • 12 years ago

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? :-)

	Eddy Bruel [:ejpbruel] Assignee
	Comment 55 • 12 years ago

This patch addresses your earlier comments.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 56 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 57 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 58 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 59 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 60 • 12 years ago

Tests will be added in a subsequent patch.

	Bobby Holley (:bholley)
	Comment 61 • 12 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 62 • 12 years ago

Any updates on getting this in?

	Eddy Bruel [:ejpbruel] Assignee
	Comment 63 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 64 • 12 years ago

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?

	Bobby Holley (:bholley)
	Comment 65 • 12 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 66 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 67 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 68 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 69 • 12 years ago

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.

	Bobby Holley (:bholley)
	Comment 70 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 71 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 72 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 73 • 12 years ago

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

	Daniel Veditz [:dveditz]
	Comment 75 • 12 years ago

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.

	Eddy Bruel [:ejpbruel] Assignee
	Comment 76 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 77 • 12 years ago

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

	Eddy Bruel [:ejpbruel] Assignee
	Comment 78 • 12 years ago

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

*flees the scene*

	Ryan VanderMeulen [:RyanVM]
	Comment 79 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 80 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 81 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 82 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 83 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 85 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 86 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 87 • 12 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 88 • 12 years ago

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

r=me

	Bobby Holley (:bholley)
	Comment 89 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 90 • 12 years ago

Pushed to release:

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

	Devdatta Akhawe [:devd]
	Comment 91 • 12 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 92 • 12 years ago

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

	u279076
	Comment 93 • 12 years ago

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

	Robert Kaiser
	Comment 94 • 12 years ago

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

	juan becerra [:juanb]
	Comment 95 • 12 years ago

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.

	juan becerra [:juanb]
	Comment 96 • 12 years ago

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

	u279076
	Comment 97 • 12 years ago

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?

	Daniel Veditz [:dveditz]
	Comment 98 • 12 years ago

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.

	Huzaifa Sidhpurwala
	Comment 99 • 12 years ago

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

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

	Huzaifa Sidhpurwala
	Comment 100 • 12 years ago

Are we expecting an ESR commit soon?

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

	Bobby Holley (:bholley)
	Comment 101 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 102 • 12 years ago

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.

	Bobby Holley (:bholley)
	Comment 103 • 12 years ago

This fixes testcase 5.

	Boris Zbarsky [:bzbarsky]
	Comment 104 • 12 years ago

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

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 105 • 12 years ago

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.  ;)

	Andrew McCreight [:mccr8]
	Comment 106 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 107 • 12 years ago

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

	Bobby Holley (:bholley)
	Comment 108 • 12 years ago

Followup bustage fix. r=jmaher on IRC

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

	u279076
	Comment 109 • 12 years ago

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.

	u279076
	Comment 110 • 12 years ago

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