355214 - PAC privilege escalation (variant of Bug 337389)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

The fix for bug 337389 is to use importFunction() to add chrome functions to
the sandbox so that PAC scripts cannot get a privileged eval from a chrome
function's proto chain.  But, there is another way that PAC scripts get a
privileged eval.  Thus, the exploit described in bug 337389 is available.

1) When func.call(thisArg) or func.apply(thisArg) is called and thisArg is a
string primitive, JS engine creates a String object in the caller's scope and
uses it as |this| object for func.

2) When dnsResolve(obj) is called, obj.valueOf("string") is called in
dnsResolve.  Also, when alert(obj) is called, obj.valueOf("undefined") is
called in proxyAlert.

3) It's possible to turn func.valueOf() into func.call() or func.apply().

  function f() {
    x = this;
  };
  f.toString = function() { return this; };
  f.valueOf = f.call;
  dnsResolve(f);

x is a String object that has the system principal, and x.eval is a privileged
eval.

The trunk, 2.0 and 1.5.0.x are affected.  With 1.0.x backport patches (bug
336601 comment #16 and bug 337389 comment #22), 1.0.x and moz1.7.x are probably
also affected.

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 an html testcase.

An alert dialog that shows Components.stack will appear.

Comment 1

[moz_bug_r_a4]

Attachment: testcase - PAC script

Comment 2

[moz_bug_r_a4]

Attachment: testcase - html

Comment 3

[Daniel Veditz [:dveditz]]

Can this be applied against GreaseMonkey user scripts, too?

Comment 4

[moz_bug_r_a4]

(In reply to comment #3)
> Can this be applied against GreaseMonkey user scripts, too?

Yes.  In fact, GM user scripts can elevate privileges by using bug 344495 or
bug 346659 *without* this bug's trick.  I've not yet reported to GM developer
since "Arbitrary code execution with certain extensions" bugs seem to be going
to be fixed by a general core fix.  (But, maybe I should have mailed to GM
developer when bug 344495 and bug 346659 were minused for 1.8.0.7.)

Comment 5

[Daniel Veditz [:dveditz]]

Blake: any thoughts on these? Is there an alternative assignee that makes sense?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I don't have any ideas that will fix this for the general case. I can fix this specific exploit by checking for this case in the various functions, but I was hoping to find a more general fix.

Comment 7

[Jay Patel [:jay]]

Brian:  Any chance you can pick Blake's brain and run with this bug?

Comment 8

[Brian Crowder]

Does the patch in bug 314874 fix this (I think this depends on our
"extension"/broken interpretation of executing "valueOf" on things that are
already objects, in call and apply)?

Comment 9

[moz_bug_r_a4]

No. The patch in bug 314874 does not fix this.

Bug 314874 is involved when this._findProxyForURL.call(this._sandBox, testURI,
testHost) is called.  (When the sandboxed code tries to get a String object
from chrome code, thisArg is a string primitive ("string" or "undefined")).

var thisArg = {
  valueOf : function(t) {
    alert(t);
    return eval;
  }
};
Function.prototype.call.call(thisArg, "", "1");

without that patch:
  1. fun_call is called (obj = Function.prototype.call).
  2. thisArg.valueOf("object") is called, and returns eval.
  3. fun_call is called (obj = eval).
  4. eval is called.

with that patch:
  1. fun_call is called (obj = Function.prototype.call).
  2. fun_call is called (obj = thisArg).
  3. thisArg.valueOf("function") is called, and returns eval.
  4. eval is called.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&rev=3.173&mark=1565-1567#1556

Comment 10

[Daniel Veditz [:dveditz]]

Blake/Brian -- any closer to a solution? Seems too risky to rush into this release at this point.

Comment 11

[Brian Crowder]

Definitely won't make it for this release, alas.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I could attach a bad hack that should fix this bug, but would not be a systematic solution.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Hack

This patch "fixes" the bug by not allowing the crazy path through XPConnect to turn the function into a string. Instead, it creates a safe, scripted, toString function in the scope of the sandbox and calls that to turn the message and host into strings. This way, we still call valueOf and friends, but the string object created comes from the sandbox, not the component.

Comment 14

[Brian Crowder]

Comment on attachment 252570 [details] [diff] [review]
Hack

The idiom of creating "safe" functions for later use in this fashion might be worth documenting.

Comment 15

[moz_bug_r_a4]

Even with that patch applied, PAC script still can get a String object from the
component, since sandbox.valueOf() can be called when
this._findProxyForURL.call(this._sandBox, testURI, testHost) is called.
(see comment #9)

Comment 16

[moz_bug_r_a4]

Attachment: testcase 2 - PAC script

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Lather, rinse, ...

I looked at this when I made the other patch and didn't bother. Oops. I think this seals the last hole where we call into the sandbox with nary a scripted function to indicate that we've done so.

With this patch, running the testcases give:
[Exception... "'Permission denied to get property XPCComponents.classes' when calling method: [nsIProxyAutoConfig::getProxyForURI]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=241036 :: a :: line 13"  data: no]

Comment 18

[Brian Crowder]

Comment on attachment 252777 [details] [diff] [review]
Lather, rinse, ...

Let the whack-a-mole begin!  How often is sandboxing like this done (or rather, how often -should- it be)?  I'm guessing a lot....

Comment 19

[Darin Fisher]

Comment on attachment 252777 [details] [diff] [review]
Lather, rinse, ...

ok... r=me (rubber stamp really).  i think it is probably time to do away with XPConnect in the PAC implementation!

Comment 20

[moz_bug_r_a4]

There is a remaining issue we could deal with.  Please see bug 355215 comment
#6.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 252777 [details] [diff] [review]
Lather, rinse, ...

sr=dveditz

Comment 22

[Daniel Veditz [:dveditz]]

Should we take what we have here in 1.8.1.2 or wait until we fix bug 355215 at the same time? Since that's waiting for bug 355766 that's not going to make this set of releases.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #22)
> Should we take what we have here in 1.8.1.2 or wait until we fix bug 355215 at
> the same time? Since that's waiting for bug 355766 that's not going to make
> this set of releases.

bug 355215 comment 10. I'll check this fix in tomorrow and attach the final patch to that bug.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in on trunk.

Comment 25

[Robert Sayre]

Unit tests are failing after this check in:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest&maxdate=1170459060

Comment 26

[Brian Crowder]

I back this out this afternoon.  Probably needs more shampoo.

Checking in nsProxyAutoConfig.js;
/cvsroot/mozilla/netwerk/base/src/nsProxyAutoConfig.js,v  <--  nsProxyAutoConfig.js
new revision: 1.43; previous revision: 1.42
done

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #25)
> Unit tests are failing after this check in:

Robert, how can I run the tests for myself? |make check| in netwerk in my tree doesn't seem to do the trick.

Comment 28

[Jeff Walden [:Waldo]]

The command's really |make -C objdir/netwerk check|, if you hadn't found out already (and for others who might read this and not already have found out).  People tend to refer to it as just |make check| because objdir may be arbitrarily selected, making the command harder to describe.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch that should apply cleanly

This *should* be the same as the last patch, which was not the patch that I ended up checking in, apparently. This has been tested by sayrer, and I've made sure that I'm checking the right thing in this time. Sorry for the mix-up.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Not-totally-broken fix checked in on trunk.

Comment 31

[Jay Patel [:jay]]

Pushing out blocking flag to next release.  Let's try to get a handle on this one, bug 355214, and bug 369213 for 1.8.1.3.

Comment 32

[Jay Patel [:jay]]

Err, I meant to say bug 355215 above (since this bug is bug 355214). :-)

Comment 33

[Daniel Veditz [:dveditz]]

moving out per Blake

Comment 34

[Daniel Veditz [:dveditz]]

Will this patch work for the 1.8.1.8 release, or do we need to wait for a XOW-based solution in 1.8.1.9?