63027 - Need ability to run Javascript code with restricted privileges

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Ben Bucksch (:BenB)]

For bug 53080, we need an option to sandbox certain JS code, i.e. remove rights
from part of the code.

jband proposaed an implementation, which he attached to that bug
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20793>.

jband, can you please explain us, how you defined "Sandbox", i.e. what
"sandboxed" code is allowed to do? Or is this configurable, i.e. are the rights
variable?

For starters like me, can you please explain (if possible), why you think, your
implementation is secure?

Comment 1

[John Bandhauer]

Attachment: a better fix

Comment 2

[John Bandhauer]

Attachment: patch to nsSample.js as a quicky test (not for checkin)

Comment 3

[John Bandhauer]

Attachment: a (perhaps) better diff

Comment 4

[John Bandhauer]

This latest patch tries to be more serious about avoiding data leakage by using 
fresh JSContexts to make the sandbox object and to run the eval.

I've been running under the assumption that we'd like to make it so that zero 
data leakage occurs from the component into the sandbox unless the component 
explicitly allows it. Inasmuch as there is fun stuff one can do by following 
__parent__ and __proto__ chains to get back to a global, I decided to play with 
creating new contexts to force the new objects (including standard objects, etc) 
to have no links back to the caller. 

Perhaps this is overkill for the autoproxy usage. We started by wanting to limit 
principals and I've stretched it to limiting access to *any* data in the trusted 
scope. This might be futile given that any call into the sandbox from the 
trusted code which includes object parameters is going to open up a path through 
which the global data in the global scope can be sniffed by the code in the 
sandbox. But, this does not allow the *direct* calls into native land that we 
would have without this eval-with-principals code.

However, even *with* this stricter multi-JSContext patch, If the trusted code 
has any methods that do anything dangerous and the trusted code passes any 
object into the sandbox, then the code in the sandbox can call the trusted 
methods that do dangerous things; e.g.

var someObject = {};
function doSomethingBad() {
    // only trusted code can do this!
    dump(Components.classes+"\n");
}

var sandbox = new Sandbox();
var url = "http://foo.bar/baz.js";
evalInSandbox(sandbox, "function bad(o){o.__parent__.doSomethingBad()};", url);
sandbox.bad(someObject);

// this succeeds in dumping "[xpconnect wrapped nsIXPCComponents_Classes]'

Is there any way around this?

Assuming that there isn't, then JS components running sandboxes had better be 
sure to be careful to *either* call no sandboxed methods with object params 
(impractical!) OR be careful to not have any methods that would be dangerous if 
called by the sandboxed code OR have a very high level of trust of the code in 
the sandbox.

Given all that, I'm wondering how much we really gain by the separate 
JSContexts. Heck, there is even some doubt that the sandbox itself is needed - 
we might be well enough off to just do an 'evalWithCodebasePrincipals(text,url)' 
and just let the eval'd script use the global object.

I like the sandbox, but I'd like to hear others' opinions on if it helps enough 
to be worth the trouble.

Comment 5

[Scott Gifford (old account)]

Added self to CC.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I wonder if the scope chain backtracking can be helped by some of the
auto-cloning mechanics, but I'm not sure how right now, and it doesn't help
__proto__.

I think we'd need to hook into js_CheckAccess to catch it all properly, though,
and I'm not sure it's worth it.  (We'd have to annotate everything created by
code operating under evalWithSandbox, and flag it as sandboxed, then check that
flag in js_CheckAccess.)

evalWithCodebasePrincipals seems like it might be a better solution here, since
it doesn't overstate its capabilities in a way that's potentially dangerously
misleading. (Not to say that I don't think evalInSandbox would be nice, it just
seems like a lot of work and head-scratching right now, and I'm sure you have
better things to be doing. =) )

Comment 7

[John Bandhauer]

Shaver, I appreciate your comments. But I don't understand exactly what course 
of action you are suggesting. Are you saying you like one or the other of my 
patches, but suggesting that I give it a less secure sounding name? Or that some 
other code should be written?

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sorry about that.  I think you should use your latest diff (though I haven't
really reviewed it, just read it quickly), and call it something like
evalWithPrincipals, because that seems to better describe exactly what the call
does.

Doing away with the new-context stuff seems reasonable as well: just fiddle the
principals, and tell people (through the API name) that it's all you're doing.

Comment 9

[Ben Bucksch (:BenB)]

Mass-change: Do not remove nominations (even if Milestone passed). Readding
mozilla0.8 nomination.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Tuesday, huh?  Let's get it done.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So I've slimmed it down to just do the principal munging, basically, and it
seems to successfully deny access to things.  If people try to evalAsCodebase
during component registration, as the nsSample test does, they will get smacked
down by the assertion god:

###!!! ASSERTION: Factory creation failed: 'NS_SUCCEEDED(rv)', file
/src/mozilla/xpcom/mozilla/xpcom/components/nsNativeComponentLoader.cpp, line 157
###!!! Break: at file
/src/mozilla/xpcom/mozilla/xpcom/components/nsNativeComponentLoader.cpp, line 157
###!!! ASSERTION: re-entrant call to service manager created service twice!:
'!mServices->Get(&key)', file
/src/mozilla/xpcom/mozilla/xpcom/components/nsServiceManager.cpp, line 353
###!!! Break: at file
/src/mozilla/xpcom/mozilla/xpcom/components/nsServiceManager.cpp, line 353
###!!! ASSERTION: re-entrant call to service manager created service twice!:
'!mServices->Get(&key)', file
/src/mozilla/xpcom/mozilla/xpcom/components/nsServiceManager.cpp, line 353
###!!! Break: at file
/src/mozilla/xpcom/mozilla/xpcom/components/nsServiceManager.cpp, line 353

I'll attach my diff anyway, because I don't think this is necessarily a problem
with this code, and the PAC guys won't likely be racing with registration this
way.  I'm away from the computer all day Tuesday, so someone else will have to
check it in if r= and sr= are happy enough.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: new diff: just the principals, ma'am

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Turns out that I'm just disturbing some code that doesn't expect to be
constructing objects at registration time.  Can't say I blame it, so ``don't do
that, then''.

(PAC's not in for 0.8.1, so this doesn't have to be either.  Lowering chofmann's
blood pressure with a milestone switch.)

Comment 14

[John Bandhauer]

shaver: You were the one who objected to my use of the word 'Sandbox'. Yet, you
retained the name and then created a 'global' object inside the original
JSContext. How is this not in the parent chain and exposing who knows what?

Also, since you don't construct a context for the eval you might want to think
about why you have this code... :) 

+  if (!retval && JS_IsExceptionPending(cx)) {
+    jsval e;
+    if (JS_GetPendingException(cx, &e))
+      JS_SetPendingException(cx, e);
+  }

I'm pretty doubtful about the safety of this stuff. I *thought* the goal was to
produce something that might standup to an attacker.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I attached the wrong diff -- in another of my 8 trees, I have one that's not
quite as sloppy. =/  (Though it still had the exception silliness.)  I'm hacking
on it now to fix some brendan comments as well.

I don't think we need a new context, and it's OK to create the global in the
current context, as long as neither __parent__ nor __proto__ leak out.  You need
to be careful what you put in the sandbox object, to avoid a __proto__ or
__parent__ leak, but I think that:

  var sandbox = new Sandbox();
  sandbox.someObj = new sandbox.Object();
  sandbox.someObj.foo = "bar";

will work just fine.  If we want, js_SandboxClass.setProperty can help by
(recursively) nulling out the parent link on objects that refer to an object
with a mismatched principal, or throwing an exception for mismatched proto links.

I'll work on that after I get back from shopping and post a new diff that will
hopefully not disappoint as much.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: A diff that doesn't make me look so dumb.

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I need to work up a set of tests for this, but I think that, as long as people
are careful about what they put in the sandbox, they're going to be fine.  (Note
that the parameter order changed to be (source, object, codebase), to match eval
better.)

Careful might well mean something like:

  var sb = new Sandbox();
  sb.someObj = new sb.Object();
  sb.someObj.foo = "bar";
  evalWithCodebase(nastySource, sb, "http://www.nasty.com");

or

  var sb = new Sandbox();
  evalWithCodebase("someObj = { foo: 'bar' }", sb, "http://www.nasty.com");
  evalWithCodebase(nastySource, sb, "http://www.nasty.com");

(The latter form seems less prone to caller error, and it makes it harder for
people to put leaky objects in place, which seems virtuous as well.)

Ideas for test cases are welcome, and I'll try to put something together today
that will drive those tests.

For now, I'm not going to recursively check or change __proto__ or __parent__
links for things assigned into the sandbox (which I would have to do when the
sandbox was about to be used for evaluation, and which is a
possibly-quite-expensive operation).

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I've been remiss in not posting tests, and will remain remiss for the rest of
the day, but I'm very interested in people's comments on this.

Comment 19

[John Bandhauer]

I don't know whether or not you can get away with just resetting the proto of 
Object and having nothing about the original global leak through. I do know a 
couple of things: 1) If you don't do this before init'ing xpconnect then 
xpconnect is going to build objects using the old chains. 2) There is code in 
caps and elsewhere that gets the global from the context using 
JS_GetGlobalObject. This may or may not burn you with this strategy.

My scheme of constructing JSContexts was messy, but free of this classs of 
unexpected 'leaks'. Maybe what you are doing is better. I don't know. When I 
was fooling with this stuff I saw a lot of unexpected stuff going on in object 
creation that linked objects back to the old chains. I was justifying the cost 
of the JSContext contruction/destruction with the assumption that creating 
sandboxes and evaling code in them is done infrequently.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: rewire proto before initting XPConnect, and add global-object switcheroo.

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Thanks, John.  New patch rewires proto and parent before initializing XPConnect
and defining our functions, and also uses the sandbox as global during eval.

As far as frequency goes, with PAC we'll evalWithCodebase for every URL we load,
including images.  I don't know if that's ``frequent'' or not, but it's a data
point.

Of course, if you have someone hostile controlling your proxy settings, you're
having a pretty bad day.  Are any of the PAC people on this bug?  Can they
explain the threat model to me?  Otherwise, I'm going to get jband drunk and
land this as is -- people can file bugs if they need something else later.

Comment 22

[John Bandhauer]

I'll look at your patch soon.

I thought the model was to eval the script like once. And then just call into 
the function (via xpconnect) on each image and page load. But I could sure be 
wrong. If they really do eval many times then I certainly agree that my 
suggested scheme of creating and destorying JSContexts each time would be very 
bad.

Comment 23

[Brendan Eich [:brendan]]

Shaver: make a new context for each eval -- it's safe and sure.  If you set up
its global to be the sandbox first, you don't need to JS_SetParent(..., NULL). 
We don't want to learn of an exploit the hard way here.

EvalWithCodebase is full of silent false returns, e.g.

+    if (argc < 3 || !JS_ConvertArguments(cx, argc, argv, "SoW",
+                                         &source, &sandbox, &URL))
+        return JS_FALSE;

(lack of style rule #37 here, too).  The later ones follow XPCOM NS_FAILED(rv)
tests, but shouldn't something be reporting an error?

/be

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

OK, OK, I'll create a context every time.

What sort of errors should I be reporting here?  I'm really not keen on having
to add a bunch of i18n-ized properties for reporting messages, but I guess there
aren't too many other choices.  jsloader.msg, here I come.

(I ass-u-me that there's no standard nsresult-to-localized-message facility in
our codebase.)

Comment 25

[Brendan Eich [:brendan]]

BTW, FindProxyForURL is the function called on every URL fetch, but its
declaration is eval'd only once per PAC file load.

I thought we had some generic nsresult reporting foo in XPConnect.  Don't people
see "failure code 0x800xxxxx" all the time?  I've seen that in my console, and
in bug reports.

/be

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

How are they going to call FindProxyForURL, if not via evalWithCodebase?  Surely
they can't just do

   var sb = new Sandbox();
   evalWithCodebase("pac code", sb, "http://foo/my.pac");
   // ... time passes, seasons change ...
   sb.FindProxyForURL("http://www.mozilla.org");

or they're running potentially-hostile code with the Super Dangerous Context And
Global, right?  I think the rule should be that you always wear the
evalWithCodebase gloves when handling PAC source.

Do you really count spitting hex nsresults to the console as error reporting? 
If so, I guess JS_ReportError with some hard-coded English text is probably
perfectly satisfactory.  I'm amazed we don't get smacked down by the l10n crew,
but right now I'm not complaining.

(It'd be, like, cool, if JS_ConvertArguments threw an exception when it failed
to convert, or at least let the caller know which argument failed so the caller
could construct a helpful error message.  I'll file an RFE against you in
retaliation. =) )

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: braces and error reporting and scratch contexts, oh my!

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

What exactly do you mean by ``if you set up its global to be the sandbox first,
you don't need to JS_SetParent(..., NULL)''?  Do you want a scratch context for
sandbox creation as well?  Or do you just want me to switch the globals around
when defining functions and spinning up XPConnect?

(New patch: style rule #37, some error reporting, and a scratch context for
evaluation.)

Comment 29

[John Bandhauer]

Sounds to me like he meant: look again at John's "a (perhaps) better diff" patch 
and remind me what was substantially wrong with it.

Look at a recent patch to bug 53080. Search for 'LocalFindProxyForURL'. They 
eval the PAC file and save off a reference to ProxySandBox.FindProxyForURL as 
LocalFindProxyForURL. And then whenever the (xpconnect wrapped JS) interface's 
ProxyForURL method is called they call that LocalFindProxyForURL. I don't *know* 
how often they eval the PAC files, but why would they do it often?

From the *start* I wondered aloud here about the wisdom of doing this as a JS 
componets *because* the sandbox exploit possibilities are iffy. 

They are making later calls with whatever JSContext is to be gleaned from the 
JSContext stack - fine. And the global of the called function is statically 
scoped (remember) to the sandbox (not your super dangerous loader global). The 
*one* danger I see is that any JSObjects that they might have to pass into the 
sandbox are 'tainted' and there is some potential for walking parent/proto 
chains and touching powerful objects. but what can they do when they touch 
those powerful objects?

I'm *really* glad that someone is actually looking at the issues here. Please 
read back in this bug (and the other bug) and let's work this out. If this whole 
"use the JS component loader" scheme can't be safe then let's junk it. If it can 
be safe within limits, then let's work out the limits, figure out if they let 
these people do what they need to do, and make the limits clear.

Comment 30

[Brendan Eich [:brendan]]

shaver: I mean you must compile FindProxyForURL using evalWithCodebase, but you
need not *call* FindProxyForURL using evalWithCodebase.  JS_CallFunction* should
do the trick, no?

Your JS_ConvertArgument whaaaaaaaambulance is on the way!

jband: I think only string params are safe for such functions.

/be

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So here's the proto/parent chain walked for the global object and the Components
object with my latest (in-tree) patch.  I've intercepted
sandbox.Object.prototype.toString to prepend "SANDBOX: ", so I can see where any
leaks are.  Looks like the Components __proto__ chain that reaches outside, and
I'm not quite sure why that happens -- I'm making the sandbox the context global
object before calling InitClasses and everything.  I wonder if it's safe to just
re__proto__ the Components object be sandbox.Object.prototype.  I'll do that now.

Apparently, sandbox.Object.prototype.__proto__ pokes off somewhere too; I'll
have to analyze the Object bootstrapping code a little better.

SANDBOX: [object Sandbox].__proto__: SANDBOX: [object Object]
  SANDBOX: [object Object].__proto__: [object Object]
    [object Object].__proto__: [object Object]
      [object Object].__proto__: null

SANDBOX: [object Sandbox].__parent__: null

[xpconnect wrapped nsIXPCComponents @ 0x8121818].__proto__: [object Object]
  [object Object].__proto__: [object Object]
    [object Object].__proto__: null

[xpconnect wrapped nsIXPCComponents @ 0x8121818].__parent__: SANDBOX: [object
Sandbox]
  SANDBOX: [object Sandbox].__parent__: null

I thought there was a problem with running untrusted code with a Real Context,
because the security manager looks at it.  I really don't understand how it can
be unsafe to eval script with a real context, but safe to execute a function
resulting from that eval with a real context.  Can someone vend me a clue?

Comment 32

[Brendan Eich [:brendan]]

shaver: compiled functions retain a strong ref via their script to the
compile-time principals, which are used when invoking, IIRC.  If this isn't the
case, lots of things are insecure.

Create that sandbox on the new, safe context!  Otherwise you'll use some other
context's fp, or (if fp is null) its globalObject, to find a standard class
prototype when making the sandbox object.

/be

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm an idiot. =/

Brendan reminded me of the cx->fp search for standard classes, which of course
leads right out of the sandbox.  More new-context fairy dust and it's all better.

And, of course, it looks a lot like jband's original patch (though the
indentation and line-length are better =) ).

SANDBOX: [object Sandbox].__proto__: SANDBOX: [object Object]
  SANDBOX: [object Object].__proto__: null
SANDBOX: [object Sandbox].__parent__: null
[xpconnect wrapped nsIXPCComponents @ 0x81218f8].__proto__: SANDBOX: [object Object]
  SANDBOX: [object Object].__proto__: null
[xpconnect wrapped nsIXPCComponents @ 0x81218f8].__parent__: SANDBOX: [object
Sandbox]
  SANDBOX: [object Sandbox].__parent__: null

No way out!

I think this is safe now -- but I've thought that before =/ -- as long as people
don't pass in object parameters they create outside the sandbox.

I'll attach a new diff shortly, once I finish soaking my head.

Comment 34

[John Bandhauer]

> as long as people don't pass in object parameters they create outside the 
> sandbox.

If you sift through the other bug you'll see where I said that and somebody 
pointed out that they need to do that for something or another. That code has 
changed a lot since, but this might still be the case.

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Plus ça change...

Comment 36

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fans of this bug's early career will find the latest attachment eerily familiar
in approach.  Thanks to jband and brendan for patiently watching me figure it
out for myself from first principals.

Comment 37

[Brendan Eich [:brendan]]

+    if (argc < 3 || !JS_ConvertArguments(cx, argc, argv, "SoW",
+                                         &source, &sandbox, &URL)) {
+        JS_ReportError(cx,
+                       "Arguments of incorrect type(s) to evalWithCodebase");
+        return JS_FALSE;
+    }

Argh!  You are double-reporting an error in the case where argc >= 3 and
JS_ConvertArguments fails.  cx-as-first-arg API calls always report errors!

/be

Comment 38

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Duh.

Comment 39

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

If my error reporting is kosher, and others believe (as I do) that there are no
parent or proto leaks in the sandbox as constructed by my latest diff, then I'd
really appreciate getting r and sr.

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Better (OK, any) error reporting and renamed back to evalInSandbox

Comment 41

[Brendan Eich [:brendan]]

+NewSandbox(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
+{
+    nsCOMPtr<nsIXPConnect> xpc = do_GetService(kXPConnectServiceContractID);
+    if (!xpc)
+        return JS_FALSE;

No error report here.

+    
+    JSBool ok = JS_FALSE;
+    JSContext *sandcx = JS_NewContext(JS_GetRuntime(cx), 8192);

No error check here!  If OOM, BOOM!

Is 8192 a bit big for a temporary context's stack chunk size?  No big deal if
nothing allocates stack space on sandcx (as seems to be the case), just asking.

+    JSObject *sandbox = JS_NewObject(sandcx, &js_SandboxClass, nsnull, nsnull);
+    if (!sandbox)
+        goto out;

Add an 'if (!sandcx) goto out;' after the JS_NewContext call and r/sr=brendan.

/be

Comment 42

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

|return JS_FALSE;| added, with a JS_ReportOutOfMemory if we can't allocate
sandcx (jumping to |out| there will result in us trying to JS_DestroyContext a
NULL sandcx!).

stack-chunk size reduced to 1024, with a comment:

    /*
     * We're not likely to see much action on this context, so keep stack-arena
     * chunk size small to reduce bloat.
     */
    JSContext *sandcx = JS_NewContext(JS_GetRuntime(cx), 1024);
    if (!sandcx) {
        JS_ReportOutOfMemory(cx);
        goto out;
    }

Reporting error if we can't get the XPConnect service:

    nsresult rv;
    nsCOMPtr<nsIXPConnect> xpc = do_GetService(kXPConnectServiceContractID,
                                               &rv);
    if (!xpc) {
        JS_ReportError(cx, "Unable to get XPConnect service: %08lx", rv);
        return JS_FALSE;
    }

Comment 43

[Brendan Eich [:brendan]]

You're all over my half-assed comments -- now we just need another r/sr=.  Is
jband around?

/be

Comment 44

[John Bandhauer]

No. I'm not around. Hackers have taken over my bugzilla account. I'm not 
responsible.

How could I argue with this?

FWIW, In my later local patch I simply forward declared Reporter rather than 
moving it in order to minimize the diff.

> renamed back to evalInSandbox

ROFL!

sr=jband.

I *still* am worried about the users of this code and how easy it is for them to 
open up security holes.

Comment 45

[Brendan Eich [:brendan]]

jband: I'm teasing, and it worked!  Sorry.

Should we require that the sandbox object be of class Sandbox?  What other path
is there for unsafe objects to flow into the evaluation?

/be

Comment 46

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I have found a spare set of suspenders that matches my belt, and have therefore
added:

    if (!JS_InstanceOf(cx, sandbox, &js_SandboxClass, NULL)) {
        JSClass *cls = JS_GetClass(cx, sandbox);
        const char *className = cls ? cls->name : "<unknown!>";
        JS_ReportError(cx,
      "evalInSandbox passed object of class %s instead of Sandbox", className);
        return JS_FALSE;
    }

below the JS_ConvertArguments call in EvalInSandbox.  If someone snatches the
tree from the jaws of joe.chou tonight, I'll check this in.  If either of my
reviewers object to the above addition, I'll that part out ASAP.  But it
compiles, and works (including the class check), so who could mind?

Comment 47

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yay, all in the tree.

Comment 48

[Phil Schwartau]

Marking Verified -