319263 - valueOf.call() causes privilege escalation issue in Greasemonkey

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

* Greasemonkey 0.6.4 on fx1.5

Greasemonkey provides some API functions (GM_*) to allow user scripts to do
some certain privileged operations.  GM_*.__proto__.__proto__.__parent__ is
Greasemonkey's component's compilation scope that is a BackstagePass object. 
Accessing __proto__ and __parent__ properties are protected by access check. 
But, valueOf.call() can be used to circumvent the access check.  Once a user
script reaches the BackstagePass, a user script can run arbitrary code with
chrome privileges by using Greasemonkey's non-API functions (e.g. GM_hitch, ge,
etc.).

A user script that calls content-defined function or getter/setter allows
content to access the sandbox and Greasemonkey's API functions on the sandbox. 
In such case, content can combine these two flaws to run arbitrary code with
chrome privileges.

For example, http://userscripts.org/fixes/gmailconversationpreview.user.js and
http://www.squarefree.com/userscripts/valid-xhtml.user.js are exploitable.


* Greasemonkey 0.5.3 on fx1.0.7

Also exploitable. Almost same as 0.6.4. 

Difference: In 0.5.3, Greasemonkey's functions' compilation scope is
nsXULPrototypeScript compilation scope.  When the first time Greasemonkey and
user script are installed, Greasemonkey's non-API functions aren't accessible
until Firefox restarts.  And if XUL cache is disabled, the functions don't
become accessible.  But, in such case, Function constructor can be used to run
arbitrary code.  (The security check for Function constructor has been added on
fx1.5, and should be ported to 1.0.x.)

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1

exploit for http://userscripts.org/fixes/gmailconversationpreview.user.js

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

exploit for http://www.squarefree.com/userscripts/valid-xhtml.user.js

Comment 3

[Jesse Ruderman]

I can confirm that malicious script can get chrome privs using testcase 2. I'm using Firefox 1.5, Greasemonkey 0.6.4, and the current version of valid-xhtml.user.js (which uses unsafeWindow.DOMParser).

Comment 4

[Jesse Ruderman]

I'm guessing this could also be exploited by a malicious Greasemonkey script.  (Malicious Greasemonkey scripts can already use XMLHttpRequest to any domain, but this bug would allow them to install spyware as well.)

Comment 5

[moz_bug_r_a4]

Attachment: testcase 3 - a malicious Greasemonkey script

Yes, a malicious Greasemonkey script can exploit.

Comment 6

[Aaron Boodman]

Ok, so I had already planned to add this function to Greasemonkey and strongly discourage (remove?) unsafeWindow (Thanks to Jesse Ruderman for inventing this):

function GM_executeContentScript(str) {
  wrappedContentWindow.location.href = "javascript:" + str;
}

... which would allow user scripts to execute content scripts without having any link back to Greasemonkey. Unfortunately, it would not be able to return values, but I think that people could simply execute enough script in the content scope so that they wouldn't have to return values.


Another aspect of this problem is that people can abuse * in the @include patterns to get scripts to run on pages the author did not intend. We already have a special .tld operator, which is safer, but people do not use it. Perhaps I can change the pattern matching so that it encourages safer usage.


Finally, if any user script every does .wrappedJSObject they expose themselves, and potentially all other scripts on all websites, to this problem again.

Ideas on how I can nail this down tighter?

- a

Comment 7

[Brendan Eich [:brendan]]

Using javascript: URLs is cool, and they can return results if they call some function that stores a result somewhere safe.  Not sure exactly how that would be safe, but I thought I'd throw it out.

It sounds like mrbkap is all over this bug, btw.  There is another bug to be filed that he found, too.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Note that if your functions only need to do content-like stuff (i.e., they don't need to have chrome principals when they're called), then you might be able to reparent them to the content window when you set up your sandbox for the user script(s).

For example, when setting up your sandbox:
var sandbox = new Components.utils.Sandbox(...);
sandbox.GM_hitch = eval("(" + GM_hitch.toString() + ")", sandbox);
...

I haven't tested this, but if I'm not going insane, then this should give your functions the correct principals and make things happy, unless they need to access things like Components.classes (in which case, you'll need the patch that I've started working on to add access checks to the BackstagePass).

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: partially non-working fix

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 205520 [details] [diff] [review]
partially non-working fix

This is really the wrong way to go. Sticking functions that were directly created in a chrome context into a sandbox is really giving scripts an escape route out of the sandbox.

What's needed is a solution that will allow a (priviledged) evalInSandbox consumer to expose functions on the sandbox which can do priviledged things, but whose __parent__ and __proto__ chains lead only to the sandbox and unpriviledged kin.

I was talking to shaver about this, and we came up with the idea of creating an importFunction method which will take a priviledged function and expose it to the sandbox as described above. Additionally, in order to stop consumers from shooting themselves in the foot, we're going to disallow adding properties directly to the sandbox object in favor of using intialization scripts (e.g., instead of doing box.unsafeWindow = window.wrappedJSObject; you'd do: Components.utils.evalInSandbox("var unsafeWindow = window.wrappedJSObject", box);)

Comments are welcome.

Comment 11

[Aaron Boodman]

I barely follow what you're proposing, so I'm not sure that I can contribute much.

Let me just say that unsafeWindow is not really a priority for Greasemonkey anymore. The next version will have a GM_evaluateContentScript(str) which just takes the string and puts in in location.href = "javascript:...."; which should be completely safe. There's no good way to return a value, which sorta sucks, but I can live with it.

With unsafeWindow removed, all I really want is to be relatively sure that:

* access to DOM via XPCNativeWrappers cannot be hijacked by content
* access to special GM_* functions cannot be hijacked by content
* access to JavaScript globals like Array and Math cannot be hijacked by content

--

basically, I don't want content to know about Greasemonkey or be able to affect Greasemonkey or it's scripts. I realize that accessing content script and this goal are incompatible, which is why I want to provide GM_evaluateContentScript(str) so that there are really clear boundaries.

--

In the future, I'd really like to be able to do something like Opera's magic variables. Where, when content asks for a JS variable, a user script can return it something else. But if the user script returns on object, this gets back to the same security issues, so I think this needs some thought.

Thanks for working on this guys,

- a

Comment 12

[Daniel Veditz [:dveditz]]

Is there an implementation of the idea Blake and shaver had in comment 10?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

The work is taking place in bug 321101.

Comment 14

[Brendan Eich [:brendan]]

Attachment: 1.8 branch patch

This fix avoids writing the loop to find the global object via the parent chain from a given callable object in several places (array_extra and fun_call versus js_ComputeThis called from js_Invoke).  It also adds access control checking to the one true loop (in js_ComputeThis).

Further work is needed to rationalize the engine's checks against the mutating security landscape.  Originally, the engine left all access checks to window or similar DOM object graph articulation point object class getters and setters.  Over the years we've had to add many checks in the engine itself that couldn't be done via existing callbacks such as getters and setters.

Now, because we have built several code-sharing systems (XUL brutal sharing, XBL) on top of the cloned function object stuff originally done for similar reasons, but needed also (without access checks) for nested functions and lambdas, we're questioning the way code sharing via function cloning works.  To make it safer and stronger, we probably want to replace the special definition of "clone" used in SpiderMonkey with "import".

This plays into JS2/ES4 work.  Ever since Netscape 4 and signed script support, SpiderMonkey has featured import and export statements.  In a trusted container, e.g., window w1 loading a signed script, you could say:

export f; // a safe API function f

and in w2, loading a document whose scripts can reference w1, you could say

import w1.f; // or import w1.*

and get f in w2's namespace as a reference to a clone of w1.f.  With this patch plus existing access checks, such a clone should be safe to use.

So if GM could address its chrome window via chromeWindow, it could export XMLHttpRequest and other stuff, and import those names via

import chromeWindow.XMLHttpRequest;

before evaluating the user script.  It would want to delete chromeWindow after importing the stuff it needs into the sandbox.

To handle renaming, we would need to extend import.  Part of the JS2/ECMA Ed. 4 work was syntax for renaming when importing, so we can build on that.  Let me know if any of this doesn't make sense.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 208924 [details] [diff] [review]
1.8 branch patch

Argh, not the right patch.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: 1.8 branch patch

This is the right patch.

/be

Comment 17

[Brendan Eich [:brendan]]

var v = 'global'
function f() {
  function api() { return v }
  export api
  var v = 'local'
  return function () { return arguments.callee.__parent__ }
}
var g = f()
var a = g()
import a.api
print(api())

is a little js shell testcase, showing how to use activation (Call) objects to hold exported properties.  I left off semicolons for fun, and I made evil use of the __parent__ SpiderMonkey extension.  The output printed is 'local', natch.

As in Netscape 4, exporting non-function objects is a bad idea.  Exporting constant primitive type values is ok.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 208926 [details] [diff] [review]
1.8 branch patch

Thanks!

Comment 19

[Mike Shaver (:shaver emeritus)]

Comment on attachment 208926 [details] [diff] [review]
1.8 branch patch

sr=shaver

Comment 20

[Brendan Eich [:brendan]]

Attachment: trunk version of patch

Checking this in soon.

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 208926 [details] [diff] [review]
1.8 branch patch

Testing shows trouble. More in a bit, if my Windows builds cooperate.

/be

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More conservative fix

This patch inserts a security check into the .call and .apply paths. It fixes the PAC version of this bug, and I believe, therefore, it will fix the GreaseMonkey side of this bug.

Sadly, it will involve calling js_ComputeThis twice in these paths, but I'm not too concerned about that, since I don't think that perf here is too critical, and js_ComputeThis will return the same answer when asked twice.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 220565 [details] [diff] [review]
More conservative fix

Comments in person -- don't want OBJ_CHECK_ACCESS for parent from thisp, want to make sure js_ComputeThis doesn't cross a property edge in the object graph that is not allowed.

Also don't need newargv, and shouldn't duplicate store to newargv[-1] that js_ComputeThis does.

/be

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With comments addressed

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 220583 [details] [diff] [review]
With comments addressed

Looks good, thanks.  Safe for 1.8.0.4.

/be

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 220583 [details] [diff] [review]
With comments addressed

a=dveditz for 1.8.0 branch

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Even better fix

This makes the array extras safe too.

The only questions I have about this patch:
-- There is a slight incompatibility with fun_call: namely that currently, we walk up obj's parent chain, whereas with this patch we walk up obj-as-callable's parent chain. Do we care?

-- Why do I need the JSVAL_IS_NULL(v) test in the while loop in js_SafeComputeThis?

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 220734 [details] [diff] [review]
Even better fix

>     /* We call with 3 args (value, index, array), plus room for rval. */
>     origsp = js_AllocStack(cx, 2 + 3 + 1, &mark);
>     if (!origsp)
>         return JS_FALSE;
>+    origsp[0] = OBJECT_TO_JSVAL(callable);
>+    origsp[1] = OBJECT_TO_JSVAL(thisp);
>+
>+    thisp = js_SafeComputeThis(cx, thisp, origsp + 2);
>+    if (!thisp) {
>+        ok = JS_FALSE;
>+        goto out;
>+    }

You have just hoisted the callable and computed thisp stores out of this loop:

>     for (i = 0; i < length; i++) {
>         jsid id;
>         jsval rval2;
> 
>         ok = IndexToExistingId(cx, obj, i, &id);

And the stack at origsp is reused for each call in the loop.  Can we avoid redoing the stores to to origsp[0] and origsp[1]?  If we believe that no native messes with these slots (say for local GC rooting), we can.  But at least jsstr.c does indeed root a string jsval converted or extracted from the thisp parameter at argv[-1], so we need to redo the origsp[1] store of thisp.  Given that, perhaps it's safest to redo the origsp[0] store too, although I know of no native that mutates argv[-2].

>+/* Like js_ComputeThis, but with security checks. */
>+JSObject *
>+js_SafeComputeThis(JSContext *cx, JSObject *thisp, jsval *argv)
>+{
>+    jsid id;
>+    jsval v;
>+    uintN attrs;
>+
>+    JS_ASSERT(thisp == JSVAL_TO_OBJECT(argv[-1]));
>+
>+    /* N.B. This closely mirrors the logic in js_ComputeThis. */
>+    if (thisp && OBJ_GET_CLASS(cx, thisp) != &js_CallClass)
>+        return thisp;

The assertion makes it ok to return early rather than fall into a common argv[-1] store, as js_ComputeThis does.  However, the above early return is missing the necessary OBJ_THIS_OBJECT call.

>+    JS_ASSERT(!JSVAL_IS_PRIMITIVE(argv[-2]));
>+    thisp = OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(argv[-2]));
>+    if (!thisp) {
>+        thisp = cx->globalObject;
>+    } else {
>+        id = ATOM_TO_JSID(cx->runtime->atomState.parentAtom);
>+        for (;;) {
>+            if (!OBJ_CHECK_ACCESS(cx, thisp, id, JSACC_PARENT, &v, &attrs))
>+                return NULL;
>+            if (JSVAL_IS_VOID(v) || JSVAL_IS_NULL(v))
>+                break;

I wrote the JSVAL_IS_VOID(v) test, IIRC, but now I question it.  I'd expect null parent at end of scope chain.  Who is giving undefined back?  Is it an artifact of OBJ_CHECK_ACCESS, or rather, a bug?

Looks great otherwise -- almost there. Thanks for taking this on,

/be

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> The assertion makes it ok to return early rather than fall into a common
> argv[-1] store, as js_ComputeThis does.  However, the above early return is
> missing the necessary OBJ_THIS_OBJECT call.

And (I should have said, just to be clear) since OBJ_THIS_OBJECT will return a different object from its argument for the relevant cases (with objects at least), you do need to store argv[-1].  So mirroring the if/else structure of js_ComputeThis is better, and then you don't need a comment noting well that you are "closely mirror[ing]" it, when you aren't ;-).

/be

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #28)
> that, perhaps it's safest to redo the origsp[0] store too, although I know of
> no native that mutates argv[-2].

I believe the return value is stuck into argv[-2].


> missing the necessary OBJ_THIS_OBJECT call.

This is currently OK, since all codepaths lead to js_ComputeThis, which will come to the same conclusions as js_SafeComputeThis and call OBJ_THIS_OBJECT for us. I'll add a comment to this effect.

> Who is giving undefined back?  Is it an
> artifact of OBJ_CHECK_ACCESS, or rather, a bug?

When you wrote it, I assumed that it was an artifact of OBJ_CHECK_ACCESS. I can't seem to figure out how this is possible, however, given that the only check access hook doesn't modify vp at all. Is it worth filing a bug over?

New patch in a jiffy.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Best fix yet

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 220743 [details] [diff] [review]
Best fix yet

We talked, but for the record: assert JSVAL_IS_OBJECT(v) and then break if JSVAL_IS_PRIMITIVE(v) in js_SafeComputeThis -- we should be free of undefined parent returns from OBJ_CHECK_ACCESS, but why risk it on short patch release cycle notice?

Also might be more explicit and use active voice in the comment in array_extra (rval does mutate origsp[0], no can about it ;-).

/be

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch and trunk.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 220743 [details] [diff] [review]
Best fix yet

approved for 1.8.0 branch, a=dveditz for drivers

Comment 35

[Daniel Veditz [:dveditz]]

Blake landed this on the 1.8.0 branch last night.

Comment 36

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4, didn't see any exploits with the 3 testcases.

Comment 37

[Daniel Veditz [:dveditz]]

Although this is critical for Greasemonkey for mozilla-based products as a whole GM (or even PAC) is an uncommon non-default configuration and thus the overall impact is sg:moderate.

Comment 38

[Aaron Boodman]

Thanks for all your hard work on this, you guys are awesome. Beers on me sometime soon.

Were any of the new theorized APIs implemented, or was the existing stuff just fixed. Should I change anything that I'm doing in GM when this fix goes out?

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #38)
> Were any of the new theorized APIs implemented, or was the existing stuff just
> fixed. Should I change anything that I'm doing in GM when this fix goes out?

The new APIs were sadly not sufficient to protect GreaseMonkey. The fix that this bug finally implemented should require no changes on your part.