386695 - PAC privilege escalation using exception objects came from outside of sandbox

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

When FindProxyForURL() is being called, exception objects are created in a
context that is trying to load a url.

function FindProxyForURL(url, host) {
  try { new XPCNativeWrapper(); } // or Components.lookupMethod(), etc.
  catch (e) { x = e; }
}

x is an exception object came from a web page, chrome (browser.xul), or the
safe context (hidden window).

An object that has the system principal or the hidden window's principal can be
used to defeat the XPCSafeJSObjectWrapper in proxyAlert() and dnsResolve(). 
(Actual exploit code relies on other security bugs.)

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[moz_bug_r_a4]

Would bug 386635 fix this?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Without trying it, I suspect that bug 386635 will fix this -- but that's based on seeing that this exploit depends on the toString hack (covered by bug 372075). That said, this is a hole in the sandbox that we should plug, and the hole is that we associate JSContexts with principals. The bug (I believe) is here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpcthrower.cpp&rev=1.28&root=/cvsroot&mark=273-274#265

Since the context is PAC context here, we'll gladly get the global chrome window and use its constructor for the exception object. IMO, we should be calling JS_GetScopeChain (or a non-mutating version thereof).

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential fix

I'm not sure if this actually fixes the bug -- this is in a tree with a patch for bug 386635. It seems like we're (correctly, I think) throwing the error much earlier, but I'm going to hold off on asking for reviews until I have a chance to test more.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

One potential problem with the patch is that we'll now throw an exception if we use XPCThrower with no JS on the stack. I'm not sure if that's a problem or not, though.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So trying to use the JSAPI from C, say, on XPCWrappedNative options, could lead to that?

I guess it'll be an exception while trying to throw an exception, so not too terrible, perhaps...

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> So trying to use the JSAPI from C, say, on XPCWrappedNative options, could lead
> to that?

Yeah. If it turns out to be a problem, it's pretty easy to use JS_FrameIterator to avoid the problem and fall back on the current behavior.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 271164 [details] [diff] [review]
Potential fix

Yeah, this works the way I was hoping.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I backed this out -- JS_GetScopeChain is asserting.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 387591 on the assertion.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I checked this patch back in.

Comment 13

[Daniel Veditz [:dveditz]]

I think mrbkap said this one was good to go on the branch (no XOW dependency). Please correct if not true.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 271164 [details] [diff] [review]
Potential fix

This patch applies to the 1.8 branch as-is.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 271164 [details] [diff] [review]
Potential fix

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.

Comment 17

[moz_bug_r_a4]

I forgot to mention that the testcase in comment #1 uses bug 344495's trick.

Comment 18

[Alexander Sack]

Comment on attachment 271164 [details] [diff] [review]
Potential fix

a=asac for 1.8.0.15

(unmodified distro patch)

Comment 19

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in js/src/xpconnect/src/xpcthrower.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcthrower.cpp,v  <--  xpcthrower.cpp
new revision: 1.25.36.1; previous revision: 1.25
done