nsLocation::CheckURL still can use the wrong principal

VERIFIED FIXED in Firefox 16

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bobbyholley)

Tracking

(4 keywords)

unspecified
mozilla19
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16+ verified, firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox-esr10 verified)

Details

(Whiteboard: variant of bug 793121 to evade fix in bug 797204 [Must Land in 17.0])

Attachments

(2 attachments)

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.
Posted file testcase 1 - XSS (obsolete) —
This tries to get cookies for www.mozilla.org.
Blocks: 797204
Hmm, I'm not sure bug caused this.
Is this a regression?
No longer blocks: 797204
(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.
Blocks: 754202
Whiteboard: variant of bug 793121 to evade fix in bug 797204
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.
Attachment #671819 - Flags: review?(bzbarsky)
NB - I called nsContentUtils::GetCurrentJSContext() directly in order to avoid being subject to bug 747607.
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 on attachment 671821 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (Nightly fix). v1

r=me, I guess.
Attachment #671821 - Flags: review?(bzbarsky) → review+
(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.
Whiteboard: variant of bug 793121 to evade fix in bug 797204 → variant of bug 793121 to evade fix in bug 797204 [Must Land in 17.0]
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.
Attachment #671821 - Flags: sec-approval?
> 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....
(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 on attachment 671819 [details] [diff] [review]
Use the bonafide subject principal for the loadinfo owner (aurora/beta fix). v1

r=me modulo comments.
Attachment #671819 - Flags: review?(bzbarsky) → review+
Assignee: nobody → bobbyholley+bmo
https://hg.mozilla.org/mozilla-central/rev/0bdbca5347f5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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
Attachment #671821 - Flags: sec-approval? → sec-approval+
Please submit branch approval nominations for Beta and Aurora.
Akeybl gave me blanket approval to land this bug on all branches on monday.
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
Attachment #671821 - Flags: approval-mozilla-release?
Attachment #671821 - Flags: approval-mozilla-beta+
Attachment #671821 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
QA Contact: mwobensmith
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
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
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.
Group: core-security
Attachment #671821 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.