Bug 747607 (CVE-2012-4201)

Problem with evalInSandbox and location

RESOLVED FIXED in Firefox 17

Status

()

Core
Security
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla19
x86
Windows XP
sec-high
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox14 wontfix, firefox15 wontfix, firefox16 affected, firefox17+ fixed, firefox18+ fixed, firefox19+ fixed, firefox-esr1017+ fixed)

Details

(Whiteboard: [sg:high][adv-track-main17+][adv-track-esr17+])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 617183 [details]
test user script for Greasemonkey
(Reporter)

Comment 2

5 years ago
Created attachment 617184 [details]
testcase 1

This tries to get cookies for www.mozilla.org.
(Reporter)

Comment 3

5 years ago
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_. ;-)
Assignee: nobody → bobbyholley+bmo
status-firefox14: --- → affected
status-firefox15: --- → affected
Whiteboard: [sg:high]
Keywords: sec-high
Is this fixed now that bug 754202 landed?
(Reporter)

Comment 8

5 years ago
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.
Depends on: 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.
Depends on: 801641
No longer depends on: 767938
Duplicate of this bug: 801641
No longer depends on: 801641
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
Attachment #673420 - Flags: review?(bzbarsky)
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.
Attachment #673422 - Flags: review?(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
Attachment #673420 - Flags: review?(bzbarsky) → review+
Comment on attachment 673422 [details] [diff] [review]
Part 2 - Remove GetContextFromStack. v1

r=me
Attachment #673422 - Flags: review?(bzbarsky) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/355df2b4546a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36ef7e5a98cc
(and thanks for the incredibly fast turnaround, bz)
status-firefox14: affected → wontfix
status-firefox15: affected → wontfix
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
https://hg.mozilla.org/mozilla-central/rev/355df2b4546a
https://hg.mozilla.org/mozilla-central/rev/36ef7e5a98cc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox19: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +
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.
Attachment #673422 - Flags: sec-approval?
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.
Attachment #673422 - Flags: sec-approval? → sec-approval+
(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.
Attachment #673422 - Flags: approval-mozilla-beta?
Attachment #673422 - Flags: approval-mozilla-aurora?
Attachment #673422 - Flags: approval-mozilla-beta?
Attachment #673422 - Flags: approval-mozilla-beta+
Attachment #673422 - Flags: approval-mozilla-aurora?
Attachment #673422 - Flags: approval-mozilla-aurora+
Please also prepare an ESR10 patch. Thanks!
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 17+
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d56ee0d5f3e0
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5206efa1bce2
status-firefox18: affected → fixed
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fa830c91c0c4
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b3f5d96a0962
status-firefox17: affected → fixed
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/82ff28611ac7
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/6a0edb093b7a
status-firefox-esr10: affected → fixed
Whiteboard: [sg:high] → [sg:high][adv-track-main17+][adv-track-esr17+]
With GetContextFromStack gone, this isn't likely to regress. Checking in a testcase here isn't a high priority for me right now.
Flags: in-testsuite? → in-testsuite-
Blocks: 736537
Alias: CVE-2012-4201
Group: core-security
You need to log in before you can comment on or make changes to this bug.