Closed
Bug 424426
Opened 17 years ago
Closed 17 years ago
[FIX]Downgrading codebase principals in signed jars is not effective
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
People
(Reporter: mozilla, Assigned: bzbarsky)
References
()
Details
(Keywords: verified1.8.1.15, Whiteboard: [sg:high])
Attachments
(2 files)
7.79 KB,
patch
|
jst
:
review+
jst
:
superreview+
beltzner
:
approval1.9b5+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
12.45 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
dveditz
:
approval1.8.1.15+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•17 years ago
|
||
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•17 years ago
|
||
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•17 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.
![]() |
Assignee | |
Comment 4•17 years ago
|
||
Filed bug 424488 on having a decent way to test this in a good controlled manner.
![]() |
Assignee | |
Comment 5•17 years ago
|
||
> 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?
Comment 6•17 years ago
|
||
(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
![]() |
Assignee | |
Comment 7•17 years ago
|
||
> 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•17 years ago
|
||
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•17 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.
![]() |
Assignee | |
Comment 10•17 years ago
|
||
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.
![]() |
Assignee | |
Comment 11•17 years ago
|
||
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.
![]() |
Assignee | |
Comment 12•17 years ago
|
||
Assignee: dveditz → bzbarsky
Status: NEW → ASSIGNED
Attachment #311314 -
Flags: superreview?(jst)
Attachment #311314 -
Flags: review?(jst)
![]() |
Assignee | |
Updated•17 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•17 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•17 years ago
|
Attachment #311314 -
Flags: superreview?(jst)
Attachment #311314 -
Flags: superreview+
Attachment #311314 -
Flags: review?(jst)
Attachment #311314 -
Flags: review+
![]() |
Assignee | |
Comment 14•17 years ago
|
||
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 15•17 years ago
|
||
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+
![]() |
Assignee | |
Comment 16•17 years ago
|
||
We don't have any way to test this yet. See bug 424488.
![]() |
Assignee | |
Comment 17•17 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
Whiteboard: [sg:high]
Updated•17 years ago
|
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Updated•17 years ago
|
Whiteboard: [sg:high] → [sg:high][needs branch patch - eta July?]
Comment 18•17 years ago
|
||
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)
![]() |
Assignee | |
Comment 19•17 years ago
|
||
I'm not sure that the CSS thing is fixed on trunk either. I would think it would work there too, as things stand...
![]() |
Assignee | |
Comment 20•17 years ago
|
||
And it's covered by bug 424733.
![]() |
Assignee | |
Comment 21•17 years ago
|
||
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?
Updated•17 years ago
|
Whiteboard: [sg:high][needs branch patch - eta July?] → [sg:high][needs r/sr bz, possibly respin?]
Comment 22•17 years ago
|
||
(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 :(
![]() |
Assignee | |
Comment 23•17 years ago
|
||
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+
Comment 24•17 years ago
|
||
The bug I was referring to was bug 433328, fwiw.
Updated•17 years ago
|
Attachment #323787 -
Flags: approval1.8.1.15?
Comment 25•17 years ago
|
||
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+
Comment 26•17 years ago
|
||
Fixed on branch.
Keywords: fixed1.8.1.15
Whiteboard: [sg:high][needs r/sr bz, possibly respin?] → [sg:high]
Updated•17 years ago
|
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 → ---
Updated•17 years ago
|
Keywords: fixed1.8.1.15
Relanded, green = good.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Keywords: fixed1.8.1.15
Resolution: --- → FIXED
Comment 29•17 years ago
|
||
(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.
![]() |
Assignee | |
Comment 30•17 years ago
|
||
Al, see comemnt 20.
Comment 31•17 years ago
|
||
Doh. Ok. Well, this is verified for branch then.
Keywords: fixed1.8.1.15 → verified1.8.1.15
Updated•16 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•