180 bytes, text/plain
817 bytes, text/html
1.06 KB, text/html
3.39 KB, patch
|Details | Diff | Splinter Review|
6.18 KB, patch
|Details | Diff | Splinter Review|
This is related to bug 736537. If a code executed by evalInSandbox() sets location.href, nsLocation::CheckURL() gets the wrong subject principal since GetContextFromStack() ignores a sandbox's JS context and gets a JS context on which a chrome code that calls evalInSandbox() is running. Greasemonkey user scripts that access unsafeWindow could be affected. - An attacker can perform an XSS attack. - An attacker can steal a local file.
Created attachment 617185 [details] testcase 2 This tries to steal a local file from file:///C:/WINDOWS/win.ini or file:///etc/passwd
CCing the usual suspects.
Ugh. IIUC, the issue here is that we're breaking the contract with getCxSubjectPrincipal, which is that we pass the cx at the top of the stack (this variant of getSubjectPrincipal exists for performance reasons). I think this will all just go away after I re-jigger CAPS to pull the subject principal off the compartment (once compartment-per-global goes away).
(In reply to Bobby Holley (:bholley) from comment #5) > (once compartment-per-global goes away). Err, once compartment-per-global _lands_. ;-)
Is this fixed now that bug 754202 landed?
No. The problem here is that GetContextFromStack gets a JSContext that is not related to the currently running code, thus it seems that bug 754202 does not affect this bug. testcase 2 is fixed on trunk by bug 770429 (testcase 2 can no longer load file: url), but testcase 1 is not fixed. Since the Web Console code has been changed, if someone wants to run the testcases on fx15-17 without Greasemonkey, please use the Scratchpad instead of the Web Console.
(In reply to moz_bug_r_a4 from comment #8) > No. The problem here is that GetContextFromStack gets a JSContext that is > not related to the currently running code, thus it seems that bug 754202 > does not affect this bug. Oh, right! Sorry, I was confused. So the "big architectural fix" here is bug 767938.
Oh, heh. I'd totally forgotten about this bug, but I just filed bug 801641 by inspection. Same issue. I'll get to it soon.
Have a patch that passes local tests. Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=1050baf9fb8d
IPC tests uncovered some problems. Added another patch, and pushed again: https://tbpl.mozilla.org/?tree=Try&rev=5aea4a94942b
Still not quite right. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=db6900046e9b
Green on try. Uploading patches and flagging review.
Created attachment 673420 [details] [diff] [review] Part 1 - Don't throw when we fail to find a source document. v1
Created attachment 673422 [details] [diff] [review] Part 2 - Remove GetContextFromStack. v1 This function is just wrong. Let's kill it and fix up the callers.
Comment on attachment 673420 [details] [diff] [review] Part 1 - Don't throw when we fail to find a source document. v1 s/and absolute/an absolute/ and r=me
Comment on attachment 673422 [details] [diff] [review] Part 2 - Remove GetContextFromStack. v1 r=me
(and thanks for the incredibly fast turnaround, bz)
Comment on attachment 673422 [details] [diff] [review] Part 2 - Remove GetContextFromStack. v1 I already landed this on m-c because I thought we might ship it in a 16.0.2. But given that we don't seem to be, I'm flagging this for sec approval to land on branches. [Security approval request comment] How easily can the security issue be deduced from the patch? Somewhat, in the sense that we're removing code that waltzes around the security mechanism of the JSContext stack. Getting from that to a bonafide security issue is a pretty big leap though. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. There's one comment that says, in reference to "when might we have a null DOM on the cx?", "This doesn't generally happen with the DOM, but can sometimes happen with extension code in certain IPC configurations." This isn't very informative, and doesn't point to the sandbox issue. Which older supported branches are affected by this flaw? Since the beginning of time. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Don't have backports, but the patches should be mostly identical. How likely is this patch to cause regressions; how much testing does it need? Medium risk. We should land it earlier if we can.
Comment on attachment 673422 [details] [diff] [review] Part 2 - Remove GetContextFromStack. v1 sec-approval+. It is exposed and we're late enough in the cycle that we should land this anyway, I think.
(In reply to Al Billings [:abillings] from comment #24) > Comment on attachment 673422 [details] [diff] [review] > Part 2 - Remove GetContextFromStack. v1 > > sec-approval+. It is exposed and we're late enough in the cycle that we > should land this anyway, I think. Given the medium risk evaluation and sg:high rating, are we committing to taking this fix in FF17? If it does need to be fixed in FF17, we need to land on Aurora/Beta asap as well. Any more info about what may regress will help in QA verification.
(In reply to Alex Keybl [:akeybl] from comment #25) > Given the medium risk evaluation and sg:high rating, are we committing to > taking this fix in FF17? Given that I was suggesting taking it for 16.0.2, I think we should probably take this on 17. Medium risk might be overly-conservative. Medium-low? > If it does need to be fixed in FF17, we need to land on Aurora/Beta asap as > well. Any more info about what may regress will help in QA verification. Anything related to referrer URLs, but TBH I can't really imagine that happening.
Comment on attachment 673422 [details] [diff] [review] Part 2 - Remove GetContextFromStack. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): Longstanding. User impact if declined: Potential security vulnerabilities. Testing completed (on m-c, etc.): Baked on m-c for almost a week. Risk to taking this patch (and alternatives if risky): Not hugely risky (low or medium-low). No alternatives. String or UUID changes made by this patch: None.
Please also prepare an ESR10 patch. Thanks!
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d56ee0d5f3e0 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5206efa1bce2
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/fa830c91c0c4 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/b3f5d96a0962
remote: https://hg.mozilla.org/releases/mozilla-esr10/rev/82ff28611ac7 remote: https://hg.mozilla.org/releases/mozilla-esr10/rev/6a0edb093b7a
With GetContextFromStack gone, this isn't likely to regress. Checking in a testcase here isn't a high priority for me right now.