[FIX]It's possible to create a document whose URI does not match the document's principal

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bz)

Tracking

({regression, verified1.9.0.9, verified1.9.1})

1.9.1 Branch
x86
Windows XP
regression, verified1.9.0.9, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.9 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] doesn't affect trunk due to 465806)

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
This seems to be a regression from bug 445004.  fx3.0.7 and fx3.1 are affected.
On trunk, this seems to have been fixed by bug 465806.

In nsHTMLDocument::OpenCommon(), we get the URI from the document returned by
GetDocumentFromContext(), but we get the principal by using
GetSubjectPrincipal().

An attacker can create a fake login page whose URI is a real login page's URI. 
And, if a password for the real login page was already saved in the Password
Manager, an attacker can steal the password without user interaction.
(Reporter)

Comment 1

9 years ago
Created attachment 366276 [details]
testcase

Steps to reproduce:
1. Load a testcase.
2. Wait a few second.

A fake login page whose URI is
https://addons.mozilla.org/en-US/firefox/users/login appears.

And, if you already saved a password for
https://addons.mozilla.org/en-US/firefox/users/login then the Password Manager
will automatically fill in the password.
Flags: blocking1.9.1?
Flags: blocking1.9.0.8?
Whiteboard: [sg:critical] doesn't affect trunk due to 465806
Whiteboard: [sg:critical] doesn't affect trunk due to 465806 → [sg:high] doesn't affect trunk due to 465806
(Assignee)

Comment 2

9 years ago
Hmm.  So I'd be fine with backporting bug 465806 (and maybe adding a hack of some sort for system principals so as not to break extension compat).  jst, sicking, what do you think?

I guess the key to the testcase is that during execution of the function a() we actually change the principal and URI of the document whose context the function is running on.  So the function has the old principals (which are still same-origin with the parent page) while the URI of the subframe is now a different URI, as is its principal.  Is that correct?

I wish password manager didn't just rely on the URI; there are all sorts of ways the URI can be weird with document.write, especially once we implement the HTML5-specced behavior for the URI of document.write result pages.
(Reporter)

Comment 3

9 years ago
(In reply to comment #2)
> I guess the key to the testcase is that during execution of the function a() we
> actually change the principal and URI of the document whose context the
> function is running on.  So the function has the old principals (which are
> still same-origin with the parent page) while the URI of the subframe is now a
> different URI, as is its principal.  Is that correct?

Yes.
Assignee: nobody → bzbarsky
Blocks: 445004
Depends on: 465806
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.8+
Keywords: regression
(In reply to comment #2)

> I wish password manager didn't just rely on the URI; there are all sorts of
> ways the URI can be weird with document.write, especially once we implement the
> HTML5-specced behavior for the URI of document.write result pages.

What would it use instead?

If a page is able to bypass same-origin restrictions to reach into other content, it seems like that's the root problem that would lead to all kinds of other issues (say, stealing login cookies).
(Assignee)

Comment 5

9 years ago
> What would it use instead?

The principal.  That's what identifies what the page actually "is".  For example, for a data: URI it would have the actual originating URI.  That sort of thing.  It's probably a good idea to cross-check the two in the password manager case, really...

> If a page is able to bypass same-origin restrictions to reach into other
> content

All the script running in this case is script with the origin of the original outer document.  You did read the testcase, right?  There are no same-origin restrictions being bypassed.
(Assignee)

Comment 6

9 years ago
Created attachment 366467 [details] [diff] [review]
Proposed fix

This is basically the backport I described in comment 2, with a test that's based on the testcase in this bug but sufficiently different to hopefully make it unclear what the danger is...  I'd like to check in that same testcase to mozilla-central as well.  moz_bug_r, is that reasonable?

I've tested this on 1.9.1; waiting on my 1.9.0 builds.
Attachment #366467 - Flags: superreview?(jst)
Attachment #366467 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
Summary: It's possible to create a document whose URI does not match the document's principal → [FIX]It's possible to create a document whose URI does not match the document's principal
(Reporter)

Comment 7

9 years ago
I don't think your testcase is sufficiently different.  If someone runs your
testcase in browser, he/she can realize that the URI in the location bar is the
cross-origin page's URI while content is "Something".  That is a real exploit
code, I guess.
(Assignee)

Comment 8

9 years ago
Hmm.  I could obfuscate it a bit more by running the test in a subframe so that the URI is not obvious.  Would that help?

I can avoid pushing the test, but it seems a shame to have to do that.  :(
(Assignee)

Comment 9

9 years ago
The "Proposed fix" patch applies directly to 1.9.0 and fixes the bug there too.

Updated

9 years ago
Attachment #366467 - Flags: superreview?(jst)
Attachment #366467 - Flags: superreview+
Attachment #366467 - Flags: review?(jst)
Attachment #366467 - Flags: review+
Flags: blocking1.9.1? → blocking1.9.1+
(Reporter)

Comment 10

9 years ago
Once this bug is made public, attackers understand what they can do with this
bug.  So, even if the testcase in comment #1 is still private, they can easily
create a password stealing page based on your testcase.  Hmm, am I worrying
more than necessary?
(Assignee)

Comment 11

9 years ago
Once the bug is made public, I'm checking in the testcase no matter what, assuming I still remember to.  At that point we will have shipped fixes on all branches for the bug, right?
(Reporter)

Comment 12

9 years ago
Ah, checking in after shipping fixes is OK, indeed.

I'm not sure whether running the test in a subframe is helpful, if you check in
the testcase before shipping fixes.
(Assignee)

Comment 13

9 years ago
OK.  I'll do the usual "wait till we ship" thing, I guess.  I hate that.  :(
(Assignee)

Comment 14

9 years ago
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a1584533724 without the test.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Keywords: fixed1.9.1
Resolution: --- → FIXED
Version: unspecified → 1.9.1 Branch
(Assignee)

Comment 15

9 years ago
Comment on attachment 366467 [details] [diff] [review]
Proposed fix

Need this on 1.9.0 as well.
Attachment #366467 - Flags: approval1.9.0.8?
Comment on attachment 366467 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #366467 - Flags: approval1.9.0.8? → approval1.9.0.8+
(Assignee)

Comment 17

9 years ago
Checked into CVS, still without test.
Keywords: fixed1.9.0.8
Verified for 1.9.0.8 on XP with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8pre) Gecko/2009031905 GranParadiso/3.0.8pre (.NET CLR 3.5.30729). 

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090319 Shiretoko/3.5b4pre.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.8, fixed1.9.1 → verified1.9.0.8, verified1.9.1
Flags: wanted1.8.1.x-
Whiteboard: [sg:high] doesn't affect trunk due to 465806 → [sg:high] doesn't affect trunk due to 465806; post 1.8-branch
Flags: wanted1.8.1.x- → wanted1.8.1.x?
Whiteboard: [sg:high] doesn't affect trunk due to 465806; post 1.8-branch → [sg:high] doesn't affect trunk due to 465806
Flags: blocking1.8.1.next?
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next+
(Assignee)

Updated

8 years ago
Blocks: 465806
No longer depends on: 465806
(Assignee)

Updated

8 years ago
No longer blocks: 465806
Depends on: 465806
Whiteboard: [sg:high] doesn't affect trunk due to 465806 → [sg:high] doesn't affect trunk due to 465806; see comment 19 for 1.8-branch
Blocks: 495270
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next+
Whiteboard: [sg:high] doesn't affect trunk due to 465806; see comment 19 for 1.8-branch → [sg:high] doesn't affect trunk due to 465806
Group: core-security
You need to log in before you can comment on or make changes to this bug.