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

RESOLVED FIXED

Status

()

Core
Security: CAPS
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Collin Jackson, Assigned: bz)

Tracking

({verified1.8.1.15})

unspecified
verified1.8.1.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high], URL)

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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.
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?
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

10 years ago
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.
Filed bug 424488 on having a decent way to test this in a good controlled manner.
(Assignee)

Updated

10 years ago
Depends on: 424488
> 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?
Flags: blocking1.9?
(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
> 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.
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

10 years ago
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.
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.
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.
Created attachment 311314 [details] [diff] [review]
Let's try it
Assignee: dveditz → bzbarsky
Status: NEW → ASSIGNED
Attachment #311314 - Flags: superreview?(jst)
Attachment #311314 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
Summary: Downgrading codebase principals in signed jars is not effective → [FIX]Downgrading codebase principals in signed jars is not effective
(Reporter)

Comment 13

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

Updated

10 years ago
Attachment #311314 - Flags: superreview?(jst)
Attachment #311314 - Flags: superreview+
Attachment #311314 - Flags: review?(jst)
Attachment #311314 - Flags: review+
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.
Attachment #311314 - Flags: approval1.9b5?
Attachment #311314 - Flags: approval1.9?
Comment on attachment 311314 [details] [diff] [review]
Let's try it

a1.9b5=beltzner for beta exposure

Tests?
Attachment #311314 - Flags: approval1.9b5?
Attachment #311314 - Flags: approval1.9b5+
Attachment #311314 - Flags: approval1.9?
Attachment #311314 - Flags: approval1.9+
We don't have any way to test this yet.  See bug 424488.
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
Whiteboard: [sg:high]
Depends on: 428873
No longer depends on: 428873
Depends on: 428873
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Whiteboard: [sg:high] → [sg:high][needs branch patch - eta July?]
Created attachment 323787 [details] [diff] [review]
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...
Attachment #323787 - Flags: superreview?(bzbarsky)
Attachment #323787 - Flags: review?(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...
And it's covered by bug 424733.
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?
Whiteboard: [sg:high][needs branch patch - eta July?] → [sg:high][needs r/sr bz, possibly respin?]
(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 on attachment 323787 [details] [diff] [review]
1.8 branch version

Without that chunk, r+sr=bzbarsky
Attachment #323787 - Flags: superreview?(bzbarsky)
Attachment #323787 - Flags: superreview+
Attachment #323787 - Flags: review?(bzbarsky)
Attachment #323787 - Flags: review+
The bug I was referring to was bug 433328, fwiw.

Updated

9 years ago
Attachment #323787 - Flags: approval1.8.1.15?
Comment on attachment 323787 [details] [diff] [review]
1.8 branch version

Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #323787 - Attachment description: Partial fix. → 1.8 branch version
Attachment #323787 - Flags: approval1.8.1.15? → approval1.8.1.15+
Fixed on branch.
Keywords: fixed1.8.1.15
Whiteboard: [sg:high][needs r/sr bz, possibly respin?] → [sg:high]
Flags: blocking1.9?
Backed out of branch, maybe causing orange. I'll be watching it over the weekend and try to reland before monday.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed1.8.1.15
Relanded, green = good.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago9 years ago
Keywords: fixed1.8.1.15
Resolution: --- → FIXED
(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.
Al, see comemnt 20.
Doh. Ok. Well, this is verified for branch then.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Group: core-security
You need to log in before you can comment on or make changes to this bug.