801305 - nsLocation::CheckURL still can use the wrong principal

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Please see bug 793121 and bug 797204.

With the testcases in bug 793121, the scripted caller is a chrome code whose global object is not a window and thus GetScriptDocument returns null.  But, if a chrome code's global object is a window, nsLocation::CheckURL still can use the wrong principal.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - XSS

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

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - arbitrary code execution with Error Console

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I'm not sure bug caused this.
Is this a regression?

Comment 4

[moz_bug_r_a4]

(In reply to Olli Pettay [:smaug] from comment #3)
> Hmm, I'm not sure bug caused this.
> Is this a regression?

Basically, this is the same bug as bug 793121 (which is a regression from bug 754202).  Bug 797204 fixed the particular testcases in bug 793121, but does not fix this.

Comment 5

[Andrew McCreight [:mccr8]]

Setting same keywords as bug 793121.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Use the bonafide subject principal for the loadinfo owner (aurora/beta fix). v1

Here's a fix for aurora/beta (i.e. pre bug 797204).

The basic issue here is that we do a bunch of munging to find the "calling
document" and its principal, which we do in order to make various calculations
regarding the referrer. This generally aligns with the bonafide subject
principal from the cx compartment, but not always. When it doesn't, I think
using the frame-munged principal for the loadinfo is probably wrong. So let's
just grab the real thing here.

This fixes the testcase in my local testing.

Comment 7

[Bobby Holley (:bholley)]

NB - I called nsContentUtils::GetCurrentJSContext() directly in order to avoid being subject to bug 747607.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Use the bonafide subject principal for the loadinfo owner (Nightly fix). v1

Same thing, but post bug 797204.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 671819 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (aurora/beta fix). v1

Why can't this just call the normal GetSubjectPrincipal()?

Or is that what bug 747607 is about?  If so, I would like to see some code comments here explaining what's going on.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 671821 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (Nightly fix). v1

r=me, I guess.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Comment on attachment 671819 [details] [diff] [review]
> Use the bonafide subject principal for the loadinfo owner (aurora/beta fix).
> v1
> 
> Why can't this just call the normal GetSubjectPrincipal()?

It can be. I was just trying to avoid the XPCOM goop associated with with outparams. I can change it if you like, though.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 671821 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (Nightly fix). v1

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Somewhat. It's clear that we're getting the principal wrong. Figuring out exactly how to construct a situation where that matters is somewhat non-obvious, but very doable for the likes of moz_bug_r_a4.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All, including esr10.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I have a beta/aurora fix in this bug, and I think it should apply to esr10 also.

How likely is this patch to cause regressions; how much testing does it need?

Medium-low risk. The new behavior matches the old behavior in the common case, and in the uncommon case I think this is definitely the right thing to do. It still could break something though. Testing on m-c would be nice.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I can change it if you like, though.

Please.  It's not obvious to casual reading that the principal this is getting is just the subject principal.

If we really care, we can add a non-XPCOM-y getSubjectPrincipal, by the way....

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> > I can change it if you like, though.
> 
> Please.  It's not obvious to casual reading that the principal this is
> getting is just the subject principal.
> 
> If we really care, we can add a non-XPCOM-y getSubjectPrincipal, by the
> way....

ok. r+ with that?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 671819 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (aurora/beta fix). v1

r=me modulo comments.

Comment 16

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bdbca5347f5

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0bdbca5347f5

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 671821 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (Nightly fix). v1

sec-approval+ per the landing plan discussed yesterday

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Please submit branch approval nominations for Beta and Aurora.

Comment 20

[Bobby Holley (:bholley)]

Akeybl gave me blanket approval to land this bug on all branches on monday.

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed broken on build 2012-10-13, Firefox 19.0a1.
Verified fixed on build 2012-10-22, Firefox 19.0a1. 
Mac 10.8.2, Windows 7 and Ubuntu 11.10

Comment 22

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8c52c692a79
https://hg.mozilla.org/releases/mozilla-beta/rev/a00a19219da2
https://hg.mozilla.org/releases/mozilla-esr10/rev/39677d1cfacb

Comment 23

[u279076]

Matt, can you please verify if this is fixed for...

latest-mozilla-aurora:
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora

17.0b3 (should be available by tomorrow morning):
ftp://ftp.mozilla.org/pub/firefox/nightly/17.0b3-candidates

Same test as you did in comment 38, thanks.

Comment 24

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-release/rev/3995ff235342

Comment 25

[Bobby Holley (:bholley)]

pushed to esr10 relbranch: https://hg.mozilla.org/releases/mozilla-esr10/rev/481667090414

Comment 26

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on build 2012-10-24, 17.0b3
Verified fixed on build 2012-10-24, Aurora 18.0a2
Mac 10.8.2, Windows 7 and Ubuntu 11.10

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on build 2012-10-24, 10.0.10esr
Verified fixed on build 2012-10-24, 16.0.2
Mac 10.8.2, Windows 7 and Ubuntu 11.10

Comment 28

[Daniel Veditz [:dveditz]]

Attachment: Testcase 1 - XSS (updated)

replaced www.mozilla.org with blog.mozilla.org in the testcase because we've added X-Frame-Options to the www domain to prevent framing. Verification with the original testcase 1 is invalid.