424426 - [FIX]Downgrading codebase principals in signed jars is not effective

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Collin Jackson]

Following up on bug 424188, relative paths in signed JARs are still dangerous. Here is an example of code that, if signed, would lead to universal XSS with on users who trust the signer:

<script>
function getURL(url) {
  netscape.security.PrivilegeManager.enablePrivilege('UniversalBrowserRead');
  var xhr = new XMLHttpRequest();
  xhr.open('GET', url, false);
  xhr.send(null);
  return xhr.responseText;
}
</script>
<script src="some_relative_path.js"></script>

You can try this test case ("Globals") at <http://crypto.stanford.edu/~collinj/research/signed-scripts/more-relative-paths.html>.

The problem is that attacker can swap in a malicious unsigned some_relative_path.js file. Although this script downgrades the privileges of the document, the getURL function has already been defined before the downgrade, so it can still request privileges on behalf of the signer.

Even if you downgrade the principal associated will all the window's closures, this attack may still work if the getURL function was defined in another frame. See the "Remote-window Globals" test case.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, yeah.  So the options as I see them are to either somehow hunt down and change all the occurrences of the old principal or to mutate the principal instead of replacing it.  The latter is what we do for document.domain changes, e.g.

Mutating would mean that anything that shares that principal pointer (e.g. data:/javascript: subframes, about:blank subframes, etc, etc) would get its principal changed too.  Perhaps that's the right thing to do...

I'm not quite sure what the remote-window testcase is doing (when I get that file directly, it's blank), but I suspect doing the above would also fix it.

Brendan, jst, thoughts?

Comment 2

[Brendan Eich [:brendan]]

Mutate. Is there hazardous sharing of about:blank already due to document.domain change-initiated principal mutation? If not, then the same isolation should help here. It would be good to have some defense in depth against wrongful sharing, at least in DEBUG builds.

/be

Comment 3

[Adam Barth]

The remote-window test case works as follows:

1) Frame A from the signed JAR defines a function GetURL(url) that enables UniversalBrowser and uses it to XHR for the specified URL.
2) Frame A embeds an iframe, Frame B, to another document in the signed JAR.
3) Frame B grabs a pointer to GetURL by executing "var GetURL = parent.GetURL;".
4) Frame B embeds <script src="some_relative_path.js">.
5) The evil server responds with an unsigned JAR containing a malicious "some_relative_path.js" script.
6) Frame B's principal gets downgraded, but Frame A's principal does not.
7) The malicious script then calls GetURL in Frame B.
8) The GetURL function is now run with the principal of the frame in which it was defined, Frame A, and happy retrieves the attacker-specified URL.

The point of this example is that it in not enough to mutate the frame's principal when it embeds an unsigned script because the frame could have pointers to closures defined in other frames (whose principals are not mutated).

Apologies if this example seems contrived.  We'd prefer to give examples with real web sites, but our understanding is that these exist only inside company intranets.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Filed bug 424488 on having a decent way to test this in a good controlled manner.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Is there hazardous sharing of about:blank already due to
> document.domain change-initiated principal mutation?

Hazardous in what way?  If I have an about:blank subframe, and I set my document.domain, that effectively also sets the subframe's document.domain.  That keeps me same-origin with the subframe, but also makes it same-origin with the other document.domain setter.  I think we do want to stay same-origin with it.

Adam, thanks for the explanation!  Blake, Brendan, do we have any sort of security check at that callsite?  If not, would that not be an issue for document.domain too?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #3)
> The remote-window test case works as follows:
> 
> 1) Frame A from the signed JAR defines a function GetURL(url) that enables
> UniversalBrowser and uses it to XHR for the specified URL.
> 2) Frame A embeds an iframe, Frame B, to another document in the signed JAR.
> 3) Frame B grabs a pointer to GetURL by executing "var GetURL =
> parent.GetURL;".
> 4) Frame B embeds <script src="some_relative_path.js">.
> 5) The evil server responds with an unsigned JAR containing a malicious
> "some_relative_path.js" script.
> 6) Frame B's principal gets downgraded, but Frame A's principal does not.

At this point, ideally we want to revoke B's capability to call A's function. This would require a fair amount of JS engine work, to be efficient.

> 7) The malicious script then calls GetURL in Frame B.
> 8) The GetURL function is now run with the principal of the frame in which it
> was defined, Frame A, and happy retrieves the attacker-specified URL.

The Java security model would compute the greatest lower bound of trust labels active on the stack, IIRC. We could stop this particular scenario by doing the same, but would that be an incomplete solution? Is there a way (setTimeout comes to mind) to run GetURL without anything from B active on the stack?

Checking access on every function call could be optimized but it will hurt. The designs over the years have attempted to avoid needing to do it by monitoring references at window boundaries (if you can't name it, you can't get/set/call it).

I wish we could ditch this signed script support. Is it really in use? If so it must not be widespread, because it's not supported in other browsers. (You can tell that I've been paying too little attention to this area by the questions I'm asking.)

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is it really in use?

Apparently for intranet apps...  They use ActiveX in IE if they're not bothering to just mandate that Firefox is the one supported browser for the app.

And that wouldn't solve the document.domain problem, though that needs a little more cooperation from the various sites to be a problem, perhaps.

Another option for signed jars is that instead of downgrading we could just refuse to run the script if it doesn't come from the same jar (or at least a jar signed by the same cert and served by the same site)...  Might break some people, I guess, but not as many as disabling it altogether.

Comment 8

[Brendan Eich [:brendan]]

I was going to ask that: why not just fail hard instead of downgrading? At least as a b5 experiment (not that these intranet apps will test -- so an Fx 3.0 test).

/be

Comment 9

[Adam Barth]

Keep in mind that <script src="..."> is just the tip of the iceberg.  We haven't tested stylesheets, SWFs, Java applets, etc.

One possible solution was outlined in Comment #2 of Bug 418996:
https://bugzilla.mozilla.org/show_bug.cgi?id=418996#c2

This doesn't downgrade the principal when an unsigned script is included, but guarantees that a relative path (be it script, CSS, SWF, etc) always resolves to the same JAR.  This prevent the attacker from switching in a malicious script when the JAR meant to load its own script.

If the signed JAR embeds an unsigned script via an absolute path, e.g. <script src="http://www.example.com/lib.js">, then it seems fine not to downgrade the principal and let www.example.com run script as the signed JAR because that's how the rest of the web works.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The code that does the load and subsequent processing just has a URI object.  It has no idea where the URI object came from.  It doesn't have a concept of "relative" URI: it always has an absolute URI.  Changing this would require significant a significant overhaul, changes to a number of frozen interfaces, etc.

I agree that there are various issues here, and perhaps the jar protocol handler could be modified somehow to not allow the same underlying URI to resolve to different jar files.  Or something.  But we can't use the relative/absolute thing to make that decision.

I don't think we should be allowing signed jars to include random script without downgrading them.  Unlike web sites (where the site just screws itself or the user's interaction with itself), a signed jar that does this could seriously screw the user in general (e.g. enable installing malware on the user's hard drive).  Given the huge difference in user impact, I don't think that we should worry about how much the rest of the web works.

I think we should fix the script thing to close the known attack vector, while thinking about a way to prevent the jar-switching in general.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, the "Globals" testcase doesn't work because of this bit of code in nsScriptSecurityManager::GetFunctionObjectPrincipal:

    if (frameScript && frameScript != script)
      // Get principals from the function, etc.
    }

    // Since principals follow scope, we must get the object
    // principal from the function's scope chain. There are no
    // reliable principals compiled into the function itself.

In this case, the parent chain leads to the window, so we get the downgraded principal.  The "Remote-window Globals" testcase does work, though.  Blocking the script from running fixes it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's try it

Comment 13

[Collin Jackson]

I filed bug 424733 regarding the stylesheet issue described in comment #9. It appears that -moz-binding bypasses the downgrading/blocking for codebase principals.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 311314 [details] [diff] [review]
Let's try it

Requesting approval.  This will only affect signed jars: they will no longer be able to link in scripts that are not signed by the same cert and served from the same server.  Some breakage is possible, so would prefer beta baking.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311314 [details] [diff] [review]
Let's try it

a1.9b5=beltzner for beta exposure

Tests?

Comment 16

[Boris Zbarsky [:bzbarsky]]

We don't have any way to test this yet.  See bug 424488.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 18

[Johnny Stenback (:jst)]

Attachment: 1.8 branch version

bz, this fixes bug 424188 and all but the CSS impersonation testcase in this bug. Any thoughts what might cause that still w/ this patch? I haven't dug into that yet, figured I'd ask before diving too deep...

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm not sure that the CSS thing is fixed on trunk either.  I would think it would work there too, as things stand...

Comment 20

[Boris Zbarsky [:bzbarsky]]

And it's covered by bug 424733.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 323787 [details] [diff] [review]
1.8 branch version

>Index: content/base/src/nsScriptLoader.cpp
>-  NS_ENSURE_TRUE(globalObject, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsPIDOMWindow> pwin(do_QueryInterface(globalObject));
>+  NS_ENSURE_TRUE(pwin && pwin->IsInnerWindow(), NS_ERROR_FAILURE);

Why this change?

Comment 22

[Johnny Stenback (:jst)]

(In reply to comment #21)
> (From update of attachment 323787 [details] [diff] [review])
> >Index: content/base/src/nsScriptLoader.cpp
> >-  NS_ENSURE_TRUE(globalObject, NS_ERROR_FAILURE);
> >+
> >+  nsCOMPtr<nsPIDOMWindow> pwin(do_QueryInterface(globalObject));
> >+  NS_ENSURE_TRUE(pwin && pwin->IsInnerWindow(), NS_ERROR_FAILURE);
> 
> Why this change?
> 

Um, ignore that change, it's for a different bug. Too many changes in the same CVS tree :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 323787 [details] [diff] [review]
1.8 branch version

Without that chunk, r+sr=bzbarsky

Comment 24

[Johnny Stenback (:jst)]

The bug I was referring to was bug 433328, fwiw.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 323787 [details] [diff] [review]
1.8 branch version

Approved for 1.8.1.15, a=dveditz for release-drivers

Comment 26

[Johnny Stenback (:jst)]

Fixed on branch.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Backed out of branch, maybe causing orange. I'll be watching it over the weekend and try to reland before monday.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Relanded, green = good.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #18)
> bz, this fixes bug 424188 and all but the CSS impersonation testcase in this
> bug. Any thoughts what might cause that still w/ this patch? I haven't dug into
> that yet, figured I'd ask before diving too deep...
> 

I've verified that all but the CSS impersonation is fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre. Do we need another bug for just that issue? I don't want to mark this verified and lose track of the CSS issue.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Al, see comemnt 20.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

Doh. Ok. Well, this is verified for branch then.