321101 - Malicious PAC script can escalate privilege

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

On Firefox 1.0.7 and Mozilla 1.7.12:
Within the function FindProxyForURL, |this| refers to the nsProxyAutoConfig
component's compilation scope that is a BackstagePass object.  Thus, a PAC
script can run arbitrary code with chrome privileges by using privileged
functions in the BackstagePass.

On Firefox 1.5 and trunk:
Within the function FindProxyForURL, |this| refers to the nsProxyAutoConfig
object.  Thus, a PAC script can run arbitrary code with chrome privileges by
using privileged functions of the nsProxyAutoConfig object.

Note: Even if |this| issue is fixed, a PAC script can still use valueOf.call
trick (Bug 312871) to access the BackstagePass and run arbitrary code.

Steps to Reproduce:
1. Setup a PAC file testcase on web server, or save it to local hard disk.
2. Set Automatic proxy configuration URL to its URL.
3. Open any web site.

An alert dialog that shows Components.stack will appear.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - Malicious PAC script for Firefox 1.0.7 and Mozilla 1.7.12

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - Malicious PAC script for Firefox 1.5 and trunk

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - Malicious PAC script using valueOf.call()

Comment 4

[Daniel Veditz [:dveditz]]

What are the security boundaries of a PAC script? In the old Netscape code they used to be able to upgrade the product or install plugins so further security measures were pointless. Even now they have access to all sorts of prefs which could lower the security of the product.

Still, we did try some sort of sandboxing here, I'm just not sure how effective it is even if it were bug free.

Comment 5

[Darin Fisher]

I think the important bug here is not so much the impact on PAC but rather the impact on the XPConnect Sandbox API.  Programmers, who use that API from other parts of the application or from extensions, should be able to use that API from a JS XPCOM component without having to understand comment #0 ;-)

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Does my patch over in bug 319263 help here? Testcase 2 seems to be simple highway robbery, not really connected to evalInSandbox (or am I missing something?). Giving untrusted scripts direct access to trusted functions (functions that were created with chrome priviledges anyway) seems like a bad idea in general.

Thinking about this at 5am local time (though really only 2am to me), I'm starting to waver on making valueOf.call(null) do some sort of security check, though it still doesn't really make sense to me.

Comment 7

[Darin Fisher]

> Giving untrusted scripts direct access to trusted functions
> (functions that were created with chrome priviledges anyway) seems like a bad
> idea in general.

Can the sandbox object not protect us in this case?  I mean, we need chrome privs in order to use nsIDNSService, so I don't see how to get around that.  What we need is an easy way to build a bridge between trusted JS and untrusted JS.  Can the sandbox object not do that for us somehow?  (Assertion: the programmer shouldn't have to know so much in order to use the sandbox correctly.)

What is the alternative?  Should I write some C++ code that invokes the JS API directly instead of trying to muck around with making trusted and untrusted JS interoperate?

Comment 8

[Darin Fisher]

What is the solution to the _findProxyForURL problem?  Is it to just always refer to that function as this._sandBox.FindProxyForURL?  Or does that incur some hidden-to-me attack vector as well?

i.e., are you saying that code like this is simply bad:

  this._findProxyForURL = this._sandBox.FindProxyForURL;
  this._findProxyForURL(...);

Comment 9

[moz_bug_r_a4]

What |this| refers to depends on how the function is called.

var gF; //global variable
function test() {
  var sandbox = new Components.utils.Sandbox("");
  Components.utils.evalInSandbox("function f() { return this; }", sandbox);
  var obj = {};
  var f = gF = obj.f = sandbox.f;

  a1 = sandbox.f();
  a2 =(sandbox.f)();
  b1 = obj.f();
  b2 = (obj.f)();
  c1 = gF();
  c2 = (gF)();
  d1 = f();
  d2 = (f)();
}

a1) sandbox.f()   : [object Sandbox]
a2) (sandbox.f)() : [object Sandbox]
b1) obj.f()       : [object Object]  <== this is what 1.5 is doing
b2) (obj.f)()     : [object Sandbox]
c1) gF()          : [object Window]  <== this is what 1.0.7 is doing
c2) (gF)()        : [object Sandbox]
d1) f()           : [object Sandbox]
d2) (f)()         : [object Sandbox]

I think that gF is not a reference, and gF() should return [object Sandbox].
I wonder why there is the difference between gF() and f().


The following changes can stop testcase 1 and testcase 2.
1.5:
  - return this._findProxyForURL(testURI, testHost);
  + var f = this._findProxyForURL;
  + f(testURI, testHost);

1.0.7:
  - return LocalFindProxyForURL(uri.spec, uri.host);
  + var f = LocalFindProxyForURL;
  + f(uri.spec, uri.host);

Comment 10

[moz_bug_r_a4]

Attachment: testcase4 - shows obj.f(), gF(), f()

Comment 11

[moz_bug_r_a4]

By the way, a) and b) have the same effect as obj.valueOf.call(null).

a) (obj.valueOf)();
b) var f = obj.valueOf;
   f();

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> By the way, a) and b) have the same effect as obj.valueOf.call(null).
> 
> a) (obj.valueOf)();

This case demonstrates bug 320032.

> b) var f = obj.valueOf;
>    f();

This (assuming var f is global) shows how this-binding in JS is dynamic and depends on the call expression.  For a call via a global variable, |this| binds to the global object.

/be
> 

Comment 13

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

(In reply to comment #7)
> Can the sandbox object not protect us in this case?  I mean, we need chrome
> privs in order to use nsIDNSService, so I don't see how to get around that. 
> What we need is an easy way to build a bridge between trusted JS and untrusted
> JS.  Can the sandbox object not do that for us somehow?  (Assertion: the
> programmer shouldn't have to know so much in order to use the sandbox
> correctly.)

The sandbox should protect you (and Greasemonkey) from this.

> What is the alternative?  Should I write some C++ code that invokes the JS API
> directly instead of trying to muck around with making trusted and untrusted JS
> interoperate?

mrbkap and I had a lovely conversation yesterday in which we designed a sandbox.importFunction API, which would let you present a privileged function to the unprivileged sandbox without leaving the __parent__ or __proto__ links to leak out.

In addition, we propose to make adding a property on the sandbox object error out for non-primitives.  PAC's model of eval()ing source script is the right path there, and evalInSandbox(uneval(myWhatever)) will let you avoid creating strings yourself if that's preferred.

It will still be _possible_ for a sandbox-API user to introduce property leaks leading out to the trusted environment, but I think it will be basically impossible for them to do so unintentionally.

(I also proposed that evalInSandbox perform a complete search of the objects in the sandbox before running the eval, looking for parent/proto links that led outside its sandbox.  That would have unpleasant perf consequences for sandboxes with a lot of "default" content, so it would want pref control, but it seems like something that the PAC or GM developers might want to be able to test with.)

Wish I'd chased down bug 297877 in more detail at the time, but I didn't know about the valueOf.call escalation path then.  Alas.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

This bug is running right along in parallel with bug 319263. Since I can more easily test stuff on PAC, I'll attach patches in here. I have a partial patch implementing comment 13 (or bug 319263, comment 10). More details when I attach it.

If I've left anybody off of the CC list who should be CC'd, please add them.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Work in progress

This patch successfully blocks attachment 206497 [details] since the priviledged functions no longer lead outside of the sandbox. Note that I had to extend the importFunction API to accept a name parameter to override the given funtion's name (since PAC wants to do this for proxyAlert -> alert).

Still to-do:
*) The XXX in importFunction to do security checks needs to be addressed. As things stand, it could be bad if a script got its hands on a function it shouldn't be able to and called importFunction (I don't see a way around letting user scripts call importFunction).
*) I still need to disallow setting non-primitive properties directly on the sandbox.
*) The worker function (SandboxForwarder) needs to at least assert that it got a function.

Comment 16

[moz_bug_r_a4]

(In reply to comment #15)
> This patch successfully blocks attachment 206497 [details] since the priviledged
> functions no longer lead outside of the sandbox. 

sandbox.__proto__ is Object.prototype that belongs to BackstagePass.  Thus, 
sandbox.valueOf.call(null) returns BackstagePass.  That patch doesn't block 
this case.

  function FindProxyForURL(url, host) {
    alert(valueOf.call(null)); // this shows [object BackstagePass]
  }

Note: In 1.0.7, sandbox.__proto__ is sandbox.Object.prototype.  Thus, 
sandbox.valueOf.call(null) returns sandbox itself.

Comment 17

[moz_bug_r_a4]

Attachment: testcase 5 - Malicious PAC script using sandbox.valueOf.call()

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #16)
> sandbox.__proto__ is Object.prototype that belongs to BackstagePass.

Yikes, that's a bad regression from bug 302834 that's only tangentally related to this bug. I'll open a new bug on fixing that regression.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 321522.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential implementation

The failure cases (such as a sandbox script trying to import a function it shouldn't be able to call) need to be tested hard and the idea of getting an nsIScriptSecurityManager out of the default security manager off of my nsXPConnect seems slightly unkosher, but this seems to work.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Note: One problem with the potential implementation is the trick I use on the return value. Consider a function imported through importFunction that e.g., grabs a DOM node from some other domain. As far as I can tell, DOM nodes' toSource return {}, iow it doesn't round trip (and neither do objects that have native code as members, such as Components).

So the problem is that we still need some sort of wrapper (though not XPCNativeWrapper!) or way to prevent the returned object from being parented to a chrome context. Perhaps a cloneObject function would be useful here, which would walk up the parent chain (and proto chain?) and clone all of the objects (to eventually be parented by the sandbox), but that sounds like it might be slow.

Currently, PAC is safe, since it only returns strings from the various functions, but GreaseMonkey might want to return objects (though maybe not native objects).

Comment 22

[Aaron Boodman]

Does anyone have an example of how to use importFunction()? Would I be meant to use this with, eg, GM_xmlhttpRequest?

So: 

var sb = new Components.utils.Sandbox(contentWin);
sb.importFunction("GM_xmlhttpRequest", GM_xmlhttpRequestImplementation)'

What does this buy me? Now, if content somehow snags a ref to GM_xmlhttpRequest, they can't call it?

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

You've gotten the arguments switched around, see the nsProxyAutoConfig.js changes in my patch. Content will be able to call GM_xmlhttpRequest (mostly because the userscript itself is content). The real difference here is that content will be limited to doing only what you provide, and not be able to grab a reference to the backstage pass and create some function that steals all of your passwords, etc.

so given: var sb = new ...(contentWin);
sb.importFunction(GM_xmlhttpRequestImplementation, "GM_xmlhttpRequest");

Now if I'm content, GM_xmlhttpRequest.__parent__ === sb and __proto__.__parent__ === sb, so unless you provide GM_eval (or similar), content is limited to only being able to do what you want it to do.

Comment 24

[Aaron Boodman]

I see.

I had briefly considered moving GM to chrome privs. Since I'm in a
sandbox, I figured, the script may have chrome, but it can only access
what I provide, so perhaps that is better.

Better in one way atleast: content cannot walk up the stack to get
GM_xhr, which is really quite bad.

But of course, the downside to this is that the sandboxed script can
access Components.classes and I can't seem to stop it.

So ideally, I'd really like to be able to put a barrier between the
sandbox script and content the same way there is one between chrome
and content. So that content script cannot walk up the call stack to
get at user scripts.

However, I don't want to give user script chrome either.

Any ideas?

Comment 25

[moz_bug_r_a4]

Testcase 1 doesn't work on 1.0.8, since scopePrincipals is null when obj.eval
is called. (See bug 331943)

Also, testcase 3 doesn't work on 1.0.8 when FindProxyForURL is called from
getProxyForURI.  Note, it works when FindProxyForURL is called in the sandbox
(i.e. at the time of loading PAC script), since js_CheckPrincipalsAccess
doesn't work as intended in the sandbox (bug 331942).

Comment 26

[moz_bug_r_a4]

Attachment: testcase 6 - Malicious PAC script for Firefox 1.0.8 and Mozilla 1.7.13

This is an updated version of testcase 1.

- p.sis = function() { return { read : this.eval }; };
+ p.read = this.eval;
+ p.sis = function() { return p; };

Comment 27

[moz_bug_r_a4]

Attachment: testcase 7 - Malicious PAC script using valueOf.call()

This is an updated version of testcase 3.

- p.sis = function() { return { read : b.eval }; };
+ p.read = b.eval;
+ p.sis = function() { return p; };

Comment 28

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

(In reply to comment #24)
> But of course, the downside to this is that the sandboxed script can
> access Components.classes and I can't seem to stop it.
> 
> So ideally, I'd really like to be able to put a barrier between the
> sandbox script and content the same way there is one between chrome
> and content. So that content script cannot walk up the call stack to
> get at user scripts.
> 
> However, I don't want to give user script chrome either.

What you want is reasonable, and something that I want in a more general sense: a way to restrict some types of addons to a subset of chrome's capabilities.  (Themes are one example of these restricted addons, and search plugins are another.  But they're pretty degenerate cases, and I want to find something better in the middle.)

We should have a deeper conversation about this (and a more public one), but I wanted to say that what you ask for is an eminently reasonable thing, but also a hard thing and not one that we can readily provide right now.

Comment 29

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

Comment on attachment 208229 [details] [diff] [review]
Potential implementation

>+    // XXX There are known bugs where the toSource of an object doesn't return a
>+    // compilable string.

Can you reference a bug number here, perhaps a toSource-roundtrip metabug?

>+    return JS_EvaluateUCScript(cx, obj, JS_GetStringChars(str),
>+                               JS_GetStringLength(str), "", 0, rval);
>+}

In light of the above XXX (prefer FIXMEs!), should we be mapping a failure here to
our own exception, so that callers who hit those bugs don't get a
very-confusing SyntaxError?

Maybe that's a follow-on FIXME with bug number, I'd be fine with that, though 

XPCThrower::Throw(NS_ERROR_NOT_IMPLEMENTED, cx)

seems easy enough!

>+    // Functions come with two extra reserved slots on them. Use the 0-th slot
>+    // to communicate the wrapped function to our forwarder.
>+    return JS_SetReservedSlot(cx, newfunobj, 0, argv[0]);
>+}

I haven't really thought through the GC consequences of this -- do we need to worry about things getting badly entrained through this link?

>+JS_STATIC_DLL_CALLBACK(JSBool)
>+sandbox_setProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
>+{
>+    // Disallow directly adding to the sandbox from chrome code. It's too easy
>+    // to shoot yourself in the foot. Instead, prefer to evalInSandbox the
>+    // propeer definitions and use importFunction if 

sp: "proper"

Also, that's a great comment that most people who hit the error
will never read; is there a way to put an informative string in an exception yet?

r=shaver with those nits picked appropriately.

Comment 30

[Daniel Veditz [:dveditz]]

In mail Blake said:
> I'm going to come up with another patch to address additional concerns that I
> have, since as written the patch in the bug will break GreaseMonkey, and any
> non-PAC consumer of Components.utils.evalInSandbox. The patch in the bug
> attempted to address both GreaseMoneky and PAC problems. I think that fixing
> both PAC and GreaseMonkey for 1.8.0.4 is going to be impossible, so I'll
> attach a patch that allows GreaseMonkey to keep working as-is, and shores
> up PAC against these attacks.

Could you explain "this patch breaks GreaseMonkey"?

I'm not sure I want to rush something for PAC into 1.8.0.4 if it doesn't really solve the problem and we're going to have to patch it again in 1.8.0.5. Can you define "shores up"?

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #30)
> Could you explain "this patch breaks GreaseMonkey"?

As written, this patch breaks the currently recommended (!) pattern of doing:
var box = new Components.utils.Sandbox(...);
box.myFun = myFun;

This is dangerous for the reasons outlined in this bug (calling fun1.valueOf.call(null) will give access to the global object that created fun1. In order to force consumers to move to the new API, the patch outlaws doing the above in favor of:
box.importFunction(myFun);

But this is an incompatible change that will make GM unhappy.

> Can you define "shores up"?

I simply meant that the this fixes one of the problems pointed out in this bug (really bug 297877).

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 208229 [details] [diff] [review]
Potential implementation

Fixed differently (in a way that doesn't break GreaseMonkey) in bug 319263

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

This should now be fixed on the 1.8 branch and trunk by the checkin for bug 319263.

It might be worth investigating the approach in attachment 208229 [details] [diff] [review], so I'll file a new bug on that, but as that approach is not necessary to squash this bug, I'm marking this fixed.

Comment 34

[Daniel Veditz [:dveditz]]

Fixed by checkins for 319263 and 336313 (but see variant bug 336601)

Comment 35

[Bob Clary [:bc] (inactive)]

verified fixed firefox 1.5.0.4/2006050817 on winxp and mac os x.

Comment 36

[Bob Clary [:bc] (inactive)]

Attachment: http://test.mozilla.com/tests/mozilla.org/security/321101-02.html

Attempting to paste the different PAC scripts into the automatic proxy config did not show the exploit however this test of the sandbox showed the following on windows/linux 2.0 and trunk (as well as 1.5.0.4)

mfTest: bug 321101: INFORM: sandbox.f() [object Sandbox]
mfTest: bug 321101: ASSERT: Passed : sandbox.f() == sandbox
mfTest: bug 321101: INFORM: (sandbox.f)(): [object Sandbox]
mfTest: bug 321101: ASSERT: Passed : (sandbox.f)() == sandbox
mfTest: bug 321101: INFORM: obj.f(): [object Object]
mfTest: bug 321101: ASSERT: Failed : obj.f() == sandbox
mfTest: bug 321101: INFORM: (obj.f)(): [object Object]
mfTest: bug 321101: ASSERT: Failed : (obj.f)() == sandbox
mfTest: bug 321101: INFORM: gF(): [object Window @ 0x3fea508 (native @ 0x4125b04)]
mfTest: bug 321101: ASSERT: Failed : gF() == sandbox
mfTest: bug 321101: INFORM: (gF)(): [object Window @ 0x3fea508 (native @ 0x4125b04)]
mfTest: bug 321101: ASSERT: Failed : (gF)() == sandbox
mfTest: bug 321101: INFORM: f(): [object Sandbox]
mfTest: bug 321101: ASSERT: Passed : f() == sandbox
mfTest: bug 321101: INFORM: (f)(): [object Sandbox]
mfTest: bug 321101: ASSERT: Passed : (f)() == sandbox

Comment 37

[Bob Clary [:bc] (inactive)]

marking verified 1.8 but moz_bug_r_a4 I would appreciate your confirmation.

Comment 38

[moz_bug_r_a4]

I've tested with fx2.0-2006082203 and fx3.0-2006082204 on Windows XP, and
confirmed that PAC privilege escalation bugs (bug 321101, bug 336313, bug
336601 and bug 337389) are fixed.