612025 - window in a user script is no longer a XPCNativeWrapper object in FF4 nightly for Greasemonkey

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Erik Vold]

When a user script did alert(window); before recent FF4 nightly changes that would alert "[object XPCNativeWrapper [object Window]]" now it alerts "[object Window]".

Why is this if it's not a bug? and should GM continue to force a XPCNativeWrapper object for window or not?

When I try to do new XPCNativeWrapper(wrappedContentWin) where alert(wrappedContentWin) returns "[object XrayWrapper [object Window]]" it doesn't change (ie: alert(ew XPCNativeWrapper(wrappedContentWin)); returns "[object XrayWrapper [object Window]]")

Comment 1

[Johnny Stenback (:jst)]

Blake, lots of things changed with the compartments landing, but this sounds like it was all intentional. If so, please mark this invalid/wontfix, or say what we need to fix here and whether you think we need to fix it for a specific version etc.

Comment 2

[Anthony Lieuallen]

It's very hard to call this "working as intended".

As it stands, Greasemonkey in Firefox 3.x is secure, and in Firefox 4.x presents a wide gaping security hole.  What can we do to make sure that user scripts, don't expose themselves to content, because the "window" object gets unwrapped?

And by the way, what is XrayWrapper?  I can't find any documentation anywhere.

Comment 3

[Boris Zbarsky [:bzbarsky]]

XrayWrapper is what replaced XPCNativeWrapper.  It provides the same guarantees, etc.

Comment 4

[Johnny Stenback (:jst)]

Anthony, are you claiming Firefox 4 presents a gaping security hole because of the lack of visible wrappers here, or can you actually show an exploit due to the change in wrappers (visible or not) here?

Comment 5

[Erik Vold]

Is there any documentation on XrayWrapper atm or not? if we are able to read some documentation then we'd have a much better understanding if it's a problem for us or not.

Comment 6

[Erik Vold]

(In reply to comment #3)
> XrayWrapper is what replaced XPCNativeWrapper.  It provides the same guarantees, etc.

This isn't exactly so imo, because a 'guarantee' of the XPCNativeWrapper was that window.foo would always be undefined for example, and therefore it would be safe to add a "foo" expando property and that expando would not be readable from content.

I assume you meant that the XrayWrapper provides the guarantee to safe access to the IDL-defined methods of the window? and what else did you have in mind?

Comment 7

[Boris Zbarsky [:bzbarsky]]

No, I meant what I said.  Is your wrappedContentWin from comment 0 exposing expando properties?

Comment 8

[Erik Vold]

(In reply to comment #4)
> Anthony, are you claiming Firefox 4 presents a gaping security hole because of
> the lack of visible wrappers here, or can you actually show an exploit due to
> the change in wrappers (visible or not) here?

Take this poorly written user script for example:

window.foo = function(url) {
  setTimeout(function(){
GM_xmlhttpRequest({
  method: "GET",
  url: url,
  onload: function(response) {
    alert(response.responseText);
  }
});
  }, 0);
}

If you put javascript:window.foo("bank.com"); in the location bar then

Pre XrayWrapper this would error: Error: window.foo is not a function

With the Xraywrapper a hacker will get your money.

Comment 9

[Erik Vold]

(In reply to comment #7)
> No, I meant what I said.  Is your wrappedContentWin from comment 0 exposing
> expando properties?

The XrayWrapped window is exposing expando properties to content yes. 

If you meant what you said then what do you have to say about my comment about the 'guarantee' of a XPCNativeWrapper being that window.foo would always be undefined?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> The XrayWrapped window is exposing expando properties to content yes. 

That sounds like a bug...  Blake?

Comment 11

[Erik Vold]

(In reply to comment #10)
> > The XrayWrapped window is exposing expando properties to content yes. 
> 
> That sounds like a bug...  Blake?

Hmm I take that back actually, it looks like sandbox.__proto__ = wrappedContentWin; is converting the XrayWrapper to a Window, and expando properties on the XrayWrapper seem to be alright, but expandos on the Window are not. So we need a way to keep the window a XrayWrapper I suppose.

Comment 12

[Anthony Lieuallen]

(In reply to comment #7)
> No, I meant what I said.  Is your wrappedContentWin from comment 0 exposing
> expando properties?

Kinda.  It's weird.  I have this test case:
  https://github.com/greasemonkey/greasemonkey/issues#issue/1192/comment/549404
And in Firefox 4.0b7, it exposes expando properties, whereas they are not in Firefox 3.6.12.

To put it another way, this code:

    alert('wrappedContentWin: '+wrappedContentWin)
    alert('wrappedContentWin.window: '+wrappedContentWin.window)
    sandbox = new Components.utils.Sandbox(wrappedContentWin);
    sandbox.__proto__ = wrappedContentWin;
    Components.utils.evalInSandbox('alert(window)', sandbox, maxJSVersion);

Is giving, in 4.0b7:

    wrappedContentWin: [object XrayWrapper [object Window]]
    wrappedContentWin.window: [object XrayWrapper [object Window]]
    [object Window]

Where, of course, I expect that last line to still be a wrapper.  But it is not, and it will expose expandos.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.

What happens if you do, instead:

  sandbox = new Components.utils.Sandbox(wrappedContentWin, 
                    { sandboxPrototype: wrappedContentWin });

and skip the __proto__ line?  Does it work then?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Or is the problem that the same-origin sandbox ends up unwrapping when it gets?  Blake?

Comment 15

[Anthony Lieuallen]

(In reply to comment #13)
> What happens if you do, instead:
> ... { sandboxPrototype: wrappedContentWin }) ...

No change.  Also no change when I do no prototype at all, and instead:

    alert('wrappedContentWin: '+wrappedContentWin)
    alert('wrappedContentWin.window: '+wrappedContentWin.window)
    sandbox = new Components.utils.Sandbox(wrappedContentWin);
    sandbox.window = wrappedContentWin;
    Components.utils.evalInSandbox('window.alert(window)', sandbox, maxJSVersion);

The code run in the sandbox gets an un-wrapped window with expandos intact, even though the chrome code sees a wrapped window with no expandos.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, that definitely sounds like a bug to me....

Comment 17

[Mike Medley]

It appears that by default the sandbox constructor gives code outside the sandbox "X-ray vision"(aka access to expando properties): https://developer.mozilla.org/en/Components.utils.Sandbox#Optional_parameter

So apparently the correct code would be:

    var sandbox = Components.utils.Sandbox(wrappedContentWin, { sandboxPrototype: wrappedContentWin, wantXrays: false });
    Components.utils.evalInSandbox('window.alert(window)', sandbox,
maxJSVersion);

Comment 18

[Boris Zbarsky [:bzbarsky]]

"x-ray vision" is LACK of access to expandos.  It's guaranteed access to the underlying DOM object.  Aka what XrayWrapper does and what XPCNativeWrapper used to do.

Comment 19

[Anthony Lieuallen]

I observe no benefit by setting wantXrays to false (nor to true).

Comment 20

[Mike Medley]

So the source of the bug is that the sandbox is not inheriting the properties of wrappedContentWin when sandboxPrototype is set?

Comment 21

[Anthony Lieuallen]

(In reply to comment #20)
> So the source of the bug is that the sandbox is not inheriting the properties
> of wrappedContentWin when sandboxPrototype is set?

I don't know what the source is.  The behavior I'm seeing so far is that when assigning a "window" type object into a sandbox (as the prototype, or a property) it is stripped of its wrapper.  Other objects I've tried (document) do not seem exhibit this broken behavior, they keep their wrapper.  But here's a fun quirk:

    var sandbox = Components.utils.Sandbox(wrappedContentWin);
    sandbox.window = wrappedContentWin;
    sandbox.document = wrappedContentWin.document;
    Components.utils.evalInSandbox('window.alert(document+"\\n")', sandbox, maxJSVersion);
    Components.utils.evalInSandbox('window.alert(document)', sandbox, maxJSVersion);

gives:
    
    [object XrayWrapper [object HTMLDocument]]
    [object HTMLDocument]
    
Substituting "window" for "document" in both alert lines above always gives [object Window].  The "window" object definitely sees expandos.  The document object does not.

Comment 22

[Anthony Lieuallen]

Attachment: extremely reduced greasemonkey test case for revealed expandos

This .XPI is an extremely reduced version of Greasemonkey, down to 55 total lines of code, which exhibits the buggy behavior described above.  It creates a Components.utils.sandbox with a prototype of a wrapped content window.  Expandos in the window are revealed.

To test, install this XPI then navigate to http://www.google.com/ -- code run in the sandbox will look for "window.google" which is defined in the content scope, but because of the wrapper should not be accessible in the sandbox.

When I run this in "Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12", I get 
  window.google is a(n):
  undefined
As expected.  But when I run it in "Mozilla/5.0 (Windows NT 6.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7"
  window.google is a(n):
  object

Comment 23

[Anthony Lieuallen]

Just now, bug confirmed (with my test case above) on "Mozilla/5.0 (Windows NT 6.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7" but confirmed that the bug does NOT happen on "Mozilla/5.0 (Windows NT 6.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6"

Comment 24

[Anthony Lieuallen]

Just tested on

  Mozilla/5.0 (Windows NT 6.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

Still seeing the unexpected expando-revealing behavior, with my earlier attached XPI.  I'd appreciate if someone could confirm that this is indeed unexpected behavior.  And if so, will this be fixed before 4.0 final?

Not only is this a bug that means user scripts' scope is polluted with anything that might be defined in the content scope, I've just confirmed that the much worse reverse is also true: user scripts that assign to 'window.foo' create (or overwrite) a foo in the content scope.  This creates the potential for much havoc and broken pages (think jQuery overwritten with different incompatible versions, etc.; although I haven't tested for this, I just expect it).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Anthony, this bug is blocking betaN.  So yes, it will be fixed before 4.0 final.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

The problem here is a silly bug in the wrapping code. window is always special :-/

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

I don't know if it's worth pulling the check out into a separate function.

Comment 28

[Johnny Stenback (:jst)]

Fix landed:

http://hg.mozilla.org/mozilla-central/rev/01ea0a38827a

Comment 29

[[e-mail]]

Comment on attachment 501473 [details] [diff] [review]
Fix

>+            windowfoo = "windowbar";
JS strict warning: assignment to undeclared variable ;-)