747607 - (CVE-2012-4201) Problem with evalInSandbox and location

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

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]

Attachment: test user script for Greasemonkey

Comment 2

[moz_bug_r_a4]

Attachment: testcase 1

This tries to get cookies for www.mozilla.org.

Comment 3

[moz_bug_r_a4]

Attachment: testcase 2

This tries to steal a local file from file:///C:/WINDOWS/win.ini or file:///etc/passwd

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

CCing the usual suspects.

Comment 5

[Bobby Holley (:bholley)]

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)]

(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)]

Is this fixed now that bug 754202 landed?

Comment 8

[moz_bug_r_a4]

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)]

(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)]

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 12

[Bobby Holley (:bholley)]

Have a patch that passes local tests. Pushing to try:

https://tbpl.mozilla.org/?tree=Try&rev=1050baf9fb8d

Comment 13

[Bobby Holley (:bholley)]

IPC tests uncovered some problems. Added another patch, and pushed again:
https://tbpl.mozilla.org/?tree=Try&rev=5aea4a94942b

Comment 14

[Bobby Holley (:bholley)]

Still not quite right. Trying again:

https://tbpl.mozilla.org/?tree=Try&rev=db6900046e9b

Comment 15

[Bobby Holley (:bholley)]

Green on try. Uploading patches and flagging review.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't throw when we fail to find a source document. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove GetContextFromStack. v1

This function is just wrong. Let's kill it and fix up the callers.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 673422 [details] [diff] [review]
Part 2 - Remove GetContextFromStack. v1

r=me

Comment 20

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/355df2b4546a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36ef7e5a98cc

Comment 21

[Bobby Holley (:bholley)]

(and thanks for the incredibly fast turnaround, bz)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/355df2b4546a
https://hg.mozilla.org/mozilla-central/rev/36ef7e5a98cc

Comment 23

[Bobby Holley (:bholley)]

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 - ex-MoCo]]

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]]

(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)]

(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)]

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]]

Please also prepare an ESR10 patch. Thanks!

Comment 29

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d56ee0d5f3e0
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5206efa1bce2

Comment 30

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fa830c91c0c4
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b3f5d96a0962

Comment 31

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/82ff28611ac7
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/6a0edb093b7a

Comment 32

[Bobby Holley (:bholley)]

With GetContextFromStack gone, this isn't likely to regress. Checking in a testcase here isn't a high priority for me right now.