Closed Bug 386695 Opened 15 years ago Closed 15 years ago

PAC privilege escalation using exception objects came from outside of sandbox

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8.0.15, fixed1.8.1.12, Whiteboard: [sg:moderate] critical when PAC is used, testcase reveals 344495)

Attachments

(1 file)

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.)
Attached file testcase
Would bug 386635 fix this?
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).
Attached patch Potential fixSplinter Review
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.
Assignee: dveditz → mrbkap
Status: NEW → ASSIGNED
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.
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...
(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 on attachment 271164 [details] [diff] [review]
Potential fix

Yeah, this works the way I was hoping.
Attachment #271164 - Flags: superreview?(jst)
Attachment #271164 - Flags: review?(jst)
Attachment #271164 - Flags: superreview?(jst)
Attachment #271164 - Flags: superreview+
Attachment #271164 - Flags: review?(jst)
Attachment #271164 - Flags: review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
I backed this out -- JS_GetScopeChain is asserting.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 387591
I filed bug 387591 on the assertion.
Status: REOPENED → ASSIGNED
I checked this patch back in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:moderate] critical when PAC is used
Flags: in-testsuite?
Flags: blocking1.8.1.12?
I think mrbkap said this one was good to go on the branch (no XOW dependency). Please correct if not true.
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Comment on attachment 271164 [details] [diff] [review]
Potential fix

This patch applies to the 1.8 branch as-is.
Attachment #271164 - Flags: approval1.8.1.12?
Comment on attachment 271164 [details] [diff] [review]
Potential fix

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #271164 - Flags: approval1.8.1.12? → approval1.8.1.12+
Fixed on the 1.8 branch.
Keywords: fixed1.8.1.12
I forgot to mention that the testcase in comment #1 uses bug 344495's trick.
Depends on: 344495
Whiteboard: [sg:moderate] critical when PAC is used → [sg:moderate] critical when PAC is used, testcase reveals 344495
Group: security
Comment on attachment 271164 [details] [diff] [review]
Potential fix

a=asac for 1.8.0.15

(unmodified distro patch)
Attachment #271164 - Flags: approval1.8.0.15+
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
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.