369213 - PAC script can access internal BackstagePass object

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[shutdown]

Attachment: PoC PAC script

By using Script object, PAC script can access an internal BackstagePass
object which can be used to execute arbitrary code with chrome privileges.

Comment 1

[shutdown]

The testcase works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1.2pre)
 Gecko/20070203 BonEcho/2.0.0.2pre

IMO, trunk is not vulnerable since it has no Script object (bug 355820).

Comment 2

[Daniel Veditz [:dveditz]]

Are the fixes for bug 355214/355215 likely to help here?

Comment 3

[Brian Crowder]

It seems like they should, yes.  shutdown, can you experiment with those patches (especially, I think, the fix in 355214, even though it breaks the netwerk tests currently)?

Comment 4

[Brian Crowder]

Actually this might want both....  not sure.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

As a guess (I haven't tested), this exploit will still work (albeit s/call/apply/). I think we should simply filter for FindProxyForURL instanceof sandbox.Function.

Comment 6

[shutdown]

(In reply to comment #3)
> It seems like they should, yes.  shutdown, can you experiment with those
> patches (especially, I think, the fix in 355214, even though it breaks the
> netwerk tests currently)?

I cannot access those bugs.

(In reply to comment #5)
> As a guess (I haven't tested), this exploit will still work (albeit
> s/call/apply/). I think we should simply filter for FindProxyForURL instanceof
> sandbox.Function.

Such check could be defeated by doing |Function = Script;| in a PAC script.

Comment 7

[Daniel Veditz [:dveditz]]

Unless this fix falls out of the ones we're already working on (and at risk for this release as it is) this will have to wait for next release.

Comment 8

[Daniel Veditz [:dveditz]]

shutdown: please contact chris hofmann. he's been unable to reach you by mail

Comment 9

[Brian Crowder]

Giving this one to mrbkap, since he's got the other 2 or 3 PAC bugs I'm aware of.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This fix clamps the 'this' value to be the sandbox when we call into FindProxyForURL.

Comment 11

[Darin Fisher]

Comment on attachment 255644 [details] [diff] [review]
Fix

ok.. there's so much subtlety here.  Is there anything you can do to better document how this safety is achieved in the code?

Comment 12

[Brian Crowder]

Comment on attachment 255644 [details] [diff] [review]
Fix

While I agree that some comments might prove useful for future reference, I'm not sure you want them to be TOO useful either...  I'm torn.

Comment 13

[Darin Fisher]

I've said this before, but using XPConnect for PAC seems extremely fragile, as evidenced by how many of these bugs keep appearing.  A simple C++ solution that just programs to JSAPI directly seems ideal to me.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> I've said this before, but using XPConnect for PAC seems extremely fragile, as
> evidenced by how many of these bugs keep appearing.  A simple C++ solution that
> just programs to JSAPI directly seems ideal to me.

That's how PAC was implemented originally (all C, actually). But so were all other bindings, which meant a lot of hand-written glue code (native methods, getters, setters, etc.).

XPConnect looks like the right hammer for such nails in Mozilla, but it has unfixed security bugs. They bite PAC, but not only PAC. We need to fix them; we are going to fix them systematically, so there will be a general fix that cures this bug's symptom and ensures that it doesn't reopen. More in a bit.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #14)
> XPConnect looks like the right hammer for such nails in Mozilla, but it has
> unfixed security bugs. They bite PAC, but not only PAC. We need to fix them; we
> are going to fix them systematically

The realization that I came to in bug 355215, comment 8 was that the safe object wrapper that jst just wrote would have solved most of the PAC problems. The problem of calling back into the PAC code in a safe way (with all of these horrible .calls floating around) is unique to PAC and, without some of the more major changes that we've discussed to subject principal computation would look the same, even with the safe object wrapper.

FWIW, this is exactly what bothered me when I started looking at bug 355214 -- I was hoping for an evalInSandbox fix that would help other consumers, but instead got a PAC specific one. I think the general fix in this case is to use correctly-compiled functions to talk with the sandbox until the safe object wrapper can do it for you.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> I've said this before, but using XPConnect for PAC seems extremely fragile, as
> evidenced by how many of these bugs keep appearing.  A simple C++ solution that
> just programs to JSAPI directly seems ideal to me.

At the risk of inviting a deluge of PAC security bugs, I'm going to say that we're at the end of privilege-escalation bugs of this type in PAC. As far as I can tell, all points where privileged code touches unprivileged code now go through intermediaries that save us from ourselves.

The change to use the JSAPI directly as well as C code obviously has other benefits, I just don't think security should be the main reason to make the switch.

Comment 17

[Darin Fisher]

So, you guys are proposing a solution that is much simpler?  (The current PAC code is extremely non-trivial, and I think most people would be rightly fearful to touch it.)

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 255644 [details] [diff] [review]
Fix

sr=dveditz

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

I checked the fix into the trunk.

Darin, I believe that Brendan was talking about fixing the underlying security bugs in XPConnect/CAPS (in most cases this refers to native functions being skipped when computing subject principals). I was talking about using XPCSafeJSObjectWrapper to hide most of the function-creation.

I've also been thinking about simply returning a safe object wrapper from the sandbox constructor, which I *think* should make everything "just work".

Comment 20

[Daniel Veditz [:dveditz]]

Moving out per Blake

Comment 21

[Daniel Veditz [:dveditz]]

Can we land this safely in 1.8.1.8?