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

RESOLVED FIXED

Status

()

Core
XPConnect
--
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Erik Vold, Assigned: mrbkap)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker])

Attachments

(2 attachments)

(Reporter)

Description

7 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

7 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

7 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

7 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

7 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

7 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

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

Comment 12

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

Comment 15

7 years ago
(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

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

Comment 19

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

Comment 20

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

Comment 21

7 years ago
(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

7 years ago
Created attachment 493277 [details]
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
blocking2.0: ? → betaN+

Comment 23

7 years ago
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

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

Updated

7 years ago
Whiteboard: hardblocker
(Assignee)

Comment 26

7 years ago
The problem here is a silly bug in the wrapping code. window is always special :-/
(Assignee)

Comment 27

7 years ago
Created attachment 501473 [details] [diff] [review]
Fix

I don't know if it's worth pulling the check out into a separate function.
Attachment #501473 - Flags: review?(gal)

Updated

7 years ago
Attachment #501473 - Flags: review?(gal) → review+

Updated

7 years ago
Whiteboard: hardblocker → [hardblocker]
Fix landed:

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

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