Last Comment Bug 747607 - (CVE-2012-4201) Problem with evalInSandbox and location
(CVE-2012-4201)
: Problem with evalInSandbox and location
Status: RESOLVED FIXED
[sg:high][adv-track-main17+][adv-trac...
: sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla19
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
: 801641 (view as bug list)
Depends on:
Blocks: 736537
  Show dependency treegraph
 
Reported: 2012-04-20 22:31 PDT by moz_bug_r_a4
Modified: 2013-03-18 12:51 PDT (History)
13 users (show)
bobbyholley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
affected
+
fixed
+
fixed
+
fixed
17+
fixed


Attachments
test user script for Greasemonkey (180 bytes, text/plain)
2012-04-20 22:33 PDT, moz_bug_r_a4
no flags Details
testcase 1 (817 bytes, text/html)
2012-04-20 22:35 PDT, moz_bug_r_a4
no flags Details
testcase 2 (1.06 KB, text/html)
2012-04-20 22:36 PDT, moz_bug_r_a4
no flags Details
Part 1 - Don't throw when we fail to find a source document. v1 (3.39 KB, patch)
2012-10-19 14:03 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2 - Remove GetContextFromStack. v1 (6.18 KB, patch)
2012-10-19 14:04 PDT, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2012-04-20 22:31:34 PDT
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.
Comment 1 moz_bug_r_a4 2012-04-20 22:33:10 PDT
Created attachment 617183 [details]
test user script for Greasemonkey
Comment 2 moz_bug_r_a4 2012-04-20 22:35:11 PDT
Created attachment 617184 [details]
testcase 1

This tries to get cookies for www.mozilla.org.
Comment 3 moz_bug_r_a4 2012-04-20 22:36:37 PDT
Created attachment 617185 [details]
testcase 2

This tries to steal a local file from file:///C:/WINDOWS/win.ini or file:///etc/passwd
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-21 10:11:21 PDT
CCing the usual suspects.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-04-21 11:08:31 PDT
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).
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-04-22 06:18:19 PDT
(In reply to Bobby Holley (:bholley) from comment #5)
> (once compartment-per-global goes away).

Err, once compartment-per-global _lands_. ;-)
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-07-26 07:47:06 PDT
Is this fixed now that bug 754202 landed?
Comment 8 moz_bug_r_a4 2012-07-26 23:52:10 PDT
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.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-07-27 01:56:58 PDT
(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.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 15:47:53 PDT
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.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 06:10:16 PDT
*** Bug 801641 has been marked as a duplicate of this bug. ***
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 07:15:36 PDT
Have a patch that passes local tests. Pushing to try:

https://tbpl.mozilla.org/?tree=Try&rev=1050baf9fb8d
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 09:56:59 PDT
IPC tests uncovered some problems. Added another patch, and pushed again:
https://tbpl.mozilla.org/?tree=Try&rev=5aea4a94942b
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-10-17 03:48:08 PDT
Still not quite right. Trying again:

https://tbpl.mozilla.org/?tree=Try&rev=db6900046e9b
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-10-19 14:03:26 PDT
Green on try. Uploading patches and flagging review.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-10-19 14:03:40 PDT
Created attachment 673420 [details] [diff] [review]
Part 1 - Don't throw when we fail to find a source document. v1
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-10-19 14:04:10 PDT
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 18 Boris Zbarsky [:bz] (still a bit busy) 2012-10-19 14:08:07 PDT
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 19 Boris Zbarsky [:bz] (still a bit busy) 2012-10-19 14:13:19 PDT
Comment on attachment 673422 [details] [diff] [review]
Part 2 - Remove GetContextFromStack. v1

r=me
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-10-19 14:27:01 PDT
(and thanks for the incredibly fast turnaround, bz)
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-10-23 08:03:40 PDT
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 24 Al Billings [:abillings] 2012-10-23 15:22:35 PDT
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.
Comment 25 Alex Keybl [:akeybl] 2012-10-23 16:03:50 PDT
(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.
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 08:15:59 PDT
(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 27 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 08:16:57 PDT
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.
Comment 28 Alex Keybl [:akeybl] 2012-10-25 16:12:44 PDT
Please also prepare an ESR10 patch. Thanks!
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2012-11-07 11:01:42 PST
With GetContextFromStack gone, this isn't likely to regress. Checking in a testcase here isn't a high priority for me right now.

Note You need to log in before you can comment on or make changes to this bug.