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

RESOLVED FIXED

Status

()

defect
--
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: erikvvold, Assigned: mrbkap)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(2 attachments)

Reporter

Description

9 years ago
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]]")
blocking2.0: --- → ?
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.
Assignee: nobody → mrbkap

Comment 2

9 years ago
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.
XrayWrapper is what replaced XPCNativeWrapper.  It provides the same guarantees, etc.
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?
Reporter

Comment 5

9 years ago
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.
Reporter

Comment 6

9 years ago
(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?
No, I meant what I said.  Is your wrappedContentWin from comment 0 exposing expando properties?
Reporter

Comment 8

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

Comment 9

9 years ago
(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?
> The XrayWrapped window is exposing expando properties to content yes. 

That sounds like a bug...  Blake?
Reporter

Comment 11

9 years ago
(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.
(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.
Hmm.

What happens if you do, instead:

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

and skip the __proto__ line?  Does it work then?
Or is the problem that the same-origin sandbox ends up unwrapping when it gets?  Blake?
(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.
Yeah, that definitely sounds like a bug to me....

Comment 17

9 years ago
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);
"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.
I observe no benefit by setting wantXrays to false (nor to true).

Comment 20

9 years ago
So the source of the bug is that the sandbox is not inheriting the properties of wrappedContentWin when sandboxPrototype is set?
(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.
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
blocking2.0: ? → betaN+
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"
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).
Anthony, this bug is blocking betaN.  So yes, it will be fixed before 4.0 final.
Whiteboard: hardblocker
The problem here is a silly bug in the wrapping code. window is always special :-/
Posted patch FixSplinter Review
I don't know if it's worth pulling the check out into a separate function.
Attachment #501473 - Flags: review?(gal)

Updated

9 years ago
Attachment #501473 - Flags: review?(gal) → review+
Whiteboard: hardblocker → [hardblocker]
Fix landed:

http://hg.mozilla.org/mozilla-central/rev/01ea0a38827a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 501473 [details] [diff] [review]
Fix

>+            windowfoo = "windowbar";
JS strict warning: assignment to undeclared variable ;-)
You need to log in before you can comment on or make changes to this bug.