478438 - Can't access allAccess properties of cross-origin XPCNativeWrappers

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[moz_bug_r_a4]

Attachment: testcase

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-01-21+07%3A00&enddate=2009-01-22+03%3A00

This seems to be a regression from bug 472674 (bug 472792).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

This is a regression, but I'm not sure that I really want to fix it. There isn't any need to use XPCNativeWrapper cross-origin anymore, since XOWs behave like them when accessed this way. The fix isn't too hard if people think this is worth fixing, though.

(I should note that at least a few of the people I'm CCing don't think that the XPCNativeWrapper constructor should be available to content at all.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah.  I have no problems with XPCNativeWrapper in content not working "correctly" as long as there's no security hole, myself.

Comment 3

[moz_bug_r_a4]

This affects some of Greasemonkey user scripts.

e.g. http://userscripts.org/scripts/show/41106

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  How does that script end up with a cross-origin XPCNativeWrapper?

Comment 5

[moz_bug_r_a4]

41106.user.js:
window.GMAILTO = {};
window.GMAILTO.popup = "";
window.GMAILTO.mailto = function (e) {
  ...
  var url = "https://mail.google.com/a/" + ...
  ...
  if (window.GMAILTO.popup.close) {
    window.GMAILTO.popup.close ();
  }
  window.GMAILTO.popup = window.open (url, "gmailPopup", ...);
  if (window.focus) {
    window.GMAILTO.popup.window.focus ();
  }

window.GMAILTO.popup is a cross-origin XPCNativeWrapper, thus script can't
access window.GMAILTO.popup.close.  (At first, script can access
window.GMAILTO.popup.window.focus since newly opened window is still
about:blank.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, so greasemonkey scripts automatically get XPCNativeWrappered?  I guess that makes sense...

In that case, we should fix this.

Comment 7

[moz_bug_r_a4]

In Greasemonkey user scripts, window and document are explicit
XPCNativeWrappers provided by Greasemonkey.  And in an event handler, event
object automatically gets wrapped in an explicit XPCNativeWrapper.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Attachment: WIP patch with mochitest

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 364602 [details] [diff] [review]
WIP patch with mochitest

Implemented what we chatted about yesterday, to the best of my recollection.

Not quite sure about the double EnsureLegalActivity check in XPC_WN_NewResolve.

Also, CheckPropertyAccess should receive the flat native JS object, right?

I'd like to test write-only properties, too, but I could use help thinking of one/some.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 364602 [details] [diff] [review]
WIP patch with mochitest

>diff --git a/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp b/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp
>+    NS_NOTREACHED("Blake Kaplan owes Ben Newman $5.");

I'm not sure if this is a binding contract if I r+ this. But I take the challenge anyway!

>diff --git a/js/src/xpconnect/src/XPCNativeWrapper.cpp b/js/src/xpconnect/src/XPCNativeWrapper.cpp
>-EnsureLegalActivity(JSContext *cx, JSObject *obj)
>+EnsureLegalActivity(JSContext *cx, JSObject *obj,
>+                    jsval id=JSVAL_VOID, PRUint32 accessType=0)

Nit: spaces around the = sign.

>+      JSObject* flatObj;
>+      if (!JSVAL_IS_VOID(id) &&
>+          (accessType & (nsIXPCSecurityManager::ACCESS_GET_PROPERTY |
>+                         nsIXPCSecurityManager::ACCESS_SET_PROPERTY)) &&
>+          (flatObj = wn->GetFlatJSObject()) &&
>+          NS_SUCCEEDED(ssm->CheckPropertyAccess(cx, flatObj,
>+            STOBJ_GET_CLASS(flatObj)->name, id, accessType)))

Nit: The STOBJ_GET_CLASS should move over under the cx, but you don't want to ThrowException below if CheckPropertyAccess fails (since it sets a better exception for you). So instead, move the CheckPropertyAccess out of the condition and into the consequent.

(Then, you could do something like

rv = ssm->CheckPropertyAccess(...);
return NS_SUCCEEDED(rv);

>+  PRUint32 accessType =
>+    aIsSet ? (PRUint32)nsIXPCSecurityManager::ACCESS_SET_PROPERTY
>+           : (PRUint32)nsIXPCSecurityManager::ACCESS_GET_PROPERTY;

I wondered aloud on IRC whether it might be worth making some local aliases for these constants.

>-  if (!EnsureLegalActivity(cx, obj)) {
>+  if (!EnsureLegalActivity(cx, obj, id, nsIXPCSecurityManager::ACCESS_GET_PROPERTY) &&
>+      !EnsureLegalActivity(cx, obj, id, nsIXPCSecurityManager::ACCESS_SET_PROPERTY)) {

You can predicate the final parameter on |flags & JSRESOLVE_ASSIGNING| and not call EnsureLegalActivity twice.

>diff --git a/js/src/xpconnect/src/XPCWrapper.cpp b/js/src/xpconnect/src/XPCWrapper.cpp
>--- a/js/src/xpconnect/src/XPCWrapper.cpp
>+++ b/js/src/xpconnect/src/XPCWrapper.cpp
>@@ -542,16 +542,18 @@ XPCWrapper::ResolveNativeProperty(JSCont
>+    JS_SetReservedSlot(cx, JSVAL_TO_OBJECT(v), eAllAccessSlot, JSVAL_TRUE);

Seems like this is as good a place as any to comment why we're doing this.

>diff --git a/js/src/xpconnect/src/XPCWrapper.h b/js/src/xpconnect/src/XPCWrapper.h
>+  typedef enum FunctionObjectSlot {

The typedef is redundant in C++, right?

>+    eXOWWrappedFunctionSlot = 0,
>+    eAllAccessSlot

Given that an arbitrary renumbering of this would cause me to owe you $5 (as JS_SetReservedSlot would start failing), I think it's worth explicitly specifying |eAllAccessSlot = 1|.

>diff --git a/js/src/xpconnect/tests/mochitest/test_bug478438.html b/js/src/xpconnect/tests/mochitest/test_bug478438.html

Can use use window.location as a "write-only" property (it's write only cross origin, but not same origin)?

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Attachment: candidate patch, with mochitest

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 364631 [details] [diff] [review]
candidate patch, with mochitest

>diff --git a/js/src/xpconnect/src/XPCNativeWrapper.cpp b/js/src/xpconnect/src/XPCNativeWrapper.cpp
>+  // on |obj|.  Don't care about calling EnsureLegalActivity because arbitrary
>+  // properties can be defined on XPCNativeWrappers.
>+  return XPC_NW_RewrapIfDeepWrapper(cx, obj, *vp, vp);

Ugh, I realized that __defineProperty__ makes this untrue. I'll fix to call EnsureLegalActivity with the id and XPCWrapper::sSecMgrSetProp.

r+sr=mrbkap, thanks!

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

Attachment: fixing bit-rot (6b6128b32170) and calling EnsureLegalActivity in XPC_NW_AddProperty

(In reply to comment #12)
> Ugh, I realized that __defineProperty__ makes this untrue. I'll fix to call
> EnsureLegalActivity with the id and XPCWrapper::sSecMgrSetProp.

I can fix that as easily as you can, or was there a reason you wanted to do it?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> I can fix that as easily as you can, or was there a reason you wanted to do it?

I meant "fix it on checkin." For changes that small, you don't really need to attach a new patch, but since you have, there's no harm. One nit: the overhanging && should go on the previous line instead of the next one.

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

Attachment: setting iwin.location = "example.org" (rather than "about:blank") to avoid intermittent mochitest failures

(In reply to comment #14)
> (In reply to comment #13)
> > I can fix that as easily as you can, or was there a reason you wanted to do it?
> 
> I meant "fix it on checkin." For changes that small, you don't really need to
> attach a new patch, but since you have, there's no harm. One nit: the
> overhanging && should go on the previous line instead of the next one.

OK, got it. One more minor fix before checkin.

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c8b9eca59375

Comment 17

[(not currently active) Ted Mielczarek]

So this test's output confuses the tinderbox error parser:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236248443.1236252828.8249.gz

The "Error: " bits in the passing output get picked up as errors and show up in the brief log:
*** 42216 INFO TEST-PASS | /tests/js/src/xpconnect/tests/mochitest/test_bug478438.html | unable to resolve/get restricted property iwin.alert: Error: Permission denied for <http://localhost:8888> to get property Window.alert from <http://example.com>.
*** 42217 INFO TEST-PASS | /tests/js/src/xpconnect/tests/mochitest/test_bug478438.html | unable to call restricted method iwin.alert: Error: Permission denied for <http://localhost:8888> to get property Window.alert from <http://example.com>.

Can we tweak the test so that's not in the output when the test passes? Otherwise this will make it significantly harder for people to diagnose test failures. For reference, the regex that tinderbox uses is here:
http://mxr.mozilla.org/mozilla/source/webtools/tinderbox/ep_unittest.pl#31

Comment 18

[Markus Stange [:mstange]]

I've pushed http://hg.mozilla.org/mozilla-central/rev/2072d9ddce60 to prevent printing exceptions when the test passes.

Comment 19

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #18)
> I've pushed http://hg.mozilla.org/mozilla-central/rev/2072d9ddce60 to prevent
> printing exceptions when the test passes.

+1, nicely done

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: followup patch to fix build warning: remove 'typedef' keyword

This bug's checkin triggered a new build warning (at least on Linux):

> mozilla/js/src/xpconnect/src/XPCWrapper.h:153: warning: ‘typedef’ was ignored in this declaration

We get at least 13 identical copies of this warning -- one per file that includes XPCWrapper.h. (I say "at least" because that's how many copies I hit in a build that I interrupted.)

The attached followup patch fixes this warning by simply removing the unnecessary 'typedef' keyword.  (The keyword would be meaningful if there were a name *after* the enum declaration, before the semicolon -- but currently there's no name there, so the 'typedef' keyword doesn't accomplish anything.)

Comment 21

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 375638 [details] [diff] [review]
followup patch to fix build warning: remove 'typedef' keyword

The other option is to move the FunctionObjectSlot type name between the '}' and before the ';', but your change better matches other code in js/src/xpconnect.

Comment 22

[Daniel Holbert [:dholbert]]

followup patch pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/46a2cbd99142

Comment 23

[Ben Newman (:bnewman) (:benjamn)]

Attachment: backport to 1.9.1

Pushed to tryserver: http://hg.mozilla.org/try/rev/fd60a1ce0ce1

This patch differs slightly from the mozilla-central landed patch because there was some bit-rot and because I wanted to incorporate mstange and dholbert's improvements.

The version I pushed to tryserver actually neglected dholbert's fixup (s/typedef enum/enum/), but the attached patch incorporates that change.  If there are any compilation errors that seem to be related to the unnecessary typedef, they should be safe to ignore.

Comment 24

[Ben Newman (:bnewman) (:benjamn)]

Attachment: previous patch did not actually reflect dholbert's change after all; this one does

Pretend I was talking about this patch in my previous comment.

Comment 25

[Johnny Stenback (:jst)]

Fixed on 1.9.1.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6de44aee1041

Comment 26

[Aakash Desai [:aakashd]]

verified FIXED on builds using the attached testcase:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090604042555

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre (.NET CLR 3.5.30729) ID:20090604045218