Skip to content

Loading…

through GM_apiLeakCheck of GM_* api from keyboard events via an iframe #1494

Closed
teramako opened this Issue · 14 comments

3 participants

@teramako

When an UserScript listen keyboard events and then do some operation,
normally cannot execute GM_* apis from content's created event, but events via javascript URL of iframe.src can do.

Test code is here: https://gist.github.com/1565506

UserScript auhors need validate either aEvent.isTrusted or not by oneself ?

@teramako

fixed the title (s/though/through/)

@teramako

I reported to Mozilla Bug 716262

@arantius
Collaborator

Confirmed.


Repro steps:

Install the .user.js in the gist, edit it to

// @include        http://fiddle.jshell.net/_display/

Then go to http://jsfiddle.net/nuBgA/6/ .


This is a work around to bug #1001 . And/or a security hole.

@Ventero

After investigating a bit, it looks like the situation has changed quite a bit by now. It looks like any page-generated event passes GM_apiLeakCheck when it isn't explicitly handled by the content scope.

To give an example: Assuming GM_openInTab leaked into the content scope somehow (which nowadays seems to be only possible by having the script explicitly assign unsafeWindow.GM_openInTab = GM_openInTab or something like unsafeWindow.sandbox = this - the old trick of waiting for a content function to be called and then walking up the function.caller chain doesn't seem to work anymore, as the .caller that would cross into the sandbox seems to be null). Then the following passes the API leak check:

var div = document.createElement("div");
div.addEventListener("click", sandbox.GM_openInTab, false);
div.click();

but the following doesn't (due to appearance of the content page in the call stack):

var div = document.createElement("div");
div.addEventListener("click"; function(){sandbox.GM_openInTab("foo");}, false);
div.click();

Since the argument received by GM_openInTab in the first case is an Xray-wrapped event, I don't think there's any way to pass an (effectively) arbitray argument to the API function by using .valueOf trickery or something similar.

So while this seems to make it possible to circumvent the API leak check, it's only possible to call the API functions with a very limited set of arguments. This of course is far from ideal, but at least it shouldn't be a huge security issue. However, I have no idea how we could prevent this.

But on the bright side, it looks like #1001 is finally fixed?

This was tested with Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 and here's a gist containing the example content page and userscript.

@arantius
Collaborator

Since the argument received by GM_openInTab in the first case is an Xray-wrapped event, I don't think there's any way to pass an (effectively) arbitray argument to the API function by using .valueOf trickery or something similar.

Interesting findings! I can't think of an GM_ API that would legitimately take an Event as its first argument, so maybe we just block those too?

@Ventero

Maybe just block all calls containing any Xray wrapped argument, as those are clearly coming from outside the sandbox.

@arantius arantius added a commit to arantius/greasemonkey that referenced this issue
@arantius arantius Block any (Xray/XPC) wrapped arguments in apiLeakCheck().
Refs #1494
16db421
@arantius
Collaborator

I couldn't find a good way to check for xray-wrapped-ness. Please take a look at the commit I pushed for this.

@Ventero

Your check should definitely work, but there might be false positives (then again, I can't think of a GM-API function where an object with a wrappedJSObject property would show up in a legitimate use of that API). To make this check a bit more strict, something like XPCNativeWrapper.unwrap(arg) == arg.wrappedJSObject could be used.

What's confusing me a bit is the fact that in the second test case from the gist I posted earlier, the argument is not XrayWrapped - I always thought that any object passing through a sandbox boundary would be wrapped. But since that case is caught by the stack check, this shouldn't be an issue.

@Ventero

Mh, nevermind that. XPCNativeWrapper.unwrap(arg) == arg.wrappedJSObject still triggers the same false positives.

@Ventero

Sorry, disregard the last message. XPCNativeWrapper.unwrap(arg) == arg.wrappedJSObject definitely is a more strict check. When passed an object like {wrappedJSObject: "foo"}, this check returns false, whereas a simple check for arg.wrappedJSObject passes. Not sure about something like var k = {}; k.wrappedJSObject = k; though. In my understanding, XPCNativeWrapper.unwrap(k) should just return k, so that the check would pass. But tests show that it doesn't - which I don't quite understand.

(And I'm terribly sorry for the bugspam)

@arantius
Collaborator

Don't be sorry. I would definitely prefer a better test. I just don't know for sure how to do it yet.

@Ventero

There's an (undocumented) Components.utils.isXrayWrapper, which seems to be exactly what we want. But I'm not sure if it's a good idea to rely on an undocumented function. So maybe something like the following?

if(Cu.isXrayWrapper && Cu.isXrayWrapper(arg) ||
    XPCNativeWrapper.unwrap(arg) == arg.wrappedJSObject) {
  throw ...;
  return false; 
}
@arantius arantius modified the milestone: 1.16, 1.15
@arantius arantius modified the milestone: 1.17, 1.16
@arantius arantius modified the milestone: 2.2, 2.1
@arantius arantius modified the milestone: 2.3, 2.4
@arantius arantius modified the milestone: 2.4, 2.5
@arantius
Collaborator

Is this obsolete since expanded principal sandboxes? Leaving in this milestone to check.

@arantius
Collaborator

Well, yes. This is obsolete because there is no more GM_apiLeakCheck(). After I edit the several year old test case to run again, the plain sendEvent() works -- it generates a click event, and the click event is handled by the script. That seems pretty expected.

I've always held that Greasemonkey is for adding functionality and for fixing broken sites. But never for expressly violating the wishes of the site owner. If you can't trust the site owner not to generate events, and you react to those events in dangerous ways, then you shouldn't be doing that.

@arantius arantius closed this
@arantius arantius removed this from the 3.2 milestone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.