Closed Bug 374071 Opened 15 years ago Closed 15 years ago

PAC privilege escalation by abusing the fix for bug 369213


(Core :: Security, defect)

Windows XP
Not set





(Reporter: moz_bug_r_a4, Assigned: mrbkap)



(Whiteboard: [sg:moderate] critical for PAC users; need SJsOW)


(1 file)

Attached file testcase
Assignee: dveditz → mrbkap
Blocks: 369213
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.9?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12+
Whiteboard: [sg:moderate] critical for PAC users
So, the minimal fix here would be to add a method on the sandbox to create bound functions (though I don't actually see a function on the JS API to do so). The problem is that the only safe thing we can do with anything that has been in contact with the sandbox is call it -- getting even .call off of it is an invitation for malicious code to abuse us.
Moving out per Blake
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+
Flags: blocking1.9? → blocking1.9+
Attached patch Proposed fixSplinter Review
Okay, this should fix the rest of these pesky PAC bugs. The idea is to use the safe JSObject wrapper, which is written in C++ to do what's really quite difficult to do in JS. Because the wrappers created here always create a scripted frame, we don't have to worry about the regular tricks, and because there's no chrome JS to call, we don't have to worry about the tricks used here.

There's one compatibility niggle: I now don't cache the value of FindProxyForURL, opting to look it up each time we call the function. This is the easiest way to ensure that 'this' for FindProxyForURL is the sandbox object. This means that scripts that modify FindProxyForURL in FindProxyForURL will behave differently, but I hope that that's not too big a problem.

There is some additional work done per host lookup, but it's simply the cost of calling two extra JS functions (pure JS, through the wrapper), so it shouldn't matter.
Attachment #268406 - Flags: superreview?(darin.moz)
Attachment #268406 - Flags: review?(crowder)
Attachment #268406 - Flags: review?(crowder) → review+
Attachment #268406 - Flags: superreview?(darin.moz) → superreview+
Fix checked into trunk.
Closed: 15 years ago
Resolution: --- → FIXED
I had to back this out because it turned the tinderboxes orange: XPCSafeJSObjectWrapper depends on being able to access an nsIScriptSecurityManager, but xpcshell doesn't give it one, so it crashes.
Resolution: FIXED → ---
Depends on: 239969
Fix checked in, again.
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
104 function proxyAlert(msg) {
105     // Ensure that we have a string.
106     if (typeof msg != "string")
107         msg = new XPCSafeJSObjectWraper(msg).toString();

There is a typo: "XPCSafeJSObjectWraper"
Indeed, sorry about that. I just checked in the obvious fix to trunk:

Checking in netwerk/base/src/nsProxyAutoConfig.js;
/cvsroot/mozilla/netwerk/base/src/nsProxyAutoConfig.js,v  <--  nsProxyAutoConfig.js
new revision: 1.50; previous revision: 1.49
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Flags: in-testsuite?
Is this going to make
Blocks: 374148
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Whiteboard: [sg:moderate] critical for PAC users → [sg:moderate] critical for PAC users; need SJsOW
Flags: blocking1.8.0.15+
Flags: blocking1.8.0.14-
Flags: blocking1.8.0.14+
Depends on: SJsOW
Flags: blocking1.8.1.12+
Group: core-security
You need to log in before you can comment on or make changes to this bug.