Last Comment Bug 482206 - [FIX]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...
Status: VERIFIED FIXED
[sg:high] doesn't affect trunk due to...
: regression, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: 1.9.1 Branch
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 465806
Blocks: 445004 495270
  Show dependency treegraph
 
Reported: 2009-03-09 03:38 PDT by moz_bug_r_a4
Modified: 2013-08-25 18:07 PDT (History)
13 users (show)
mbeltzner: blocking1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.45 KB, text/html)
2009-03-09 03:40 PDT, moz_bug_r_a4
no flags Details
Proposed fix (6.15 KB, patch)
2009-03-09 18:35 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
screenshot (106.38 KB, image/jpeg)
2009-04-02 21:49 PDT, moz_bug_r_a4
no flags Details
testcase 2 (1.59 KB, text/html)
2009-04-27 18:30 PDT, moz_bug_r_a4
no flags Details

Description moz_bug_r_a4 2009-03-09 03:38:37 PDT
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.
Comment 1 moz_bug_r_a4 2009-03-09 03:40:38 PDT
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.
Comment 2 Boris Zbarsky [:bz] 2009-03-09 06:58:01 PDT
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.
Comment 3 moz_bug_r_a4 2009-03-09 09:10:21 PDT
(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.
Comment 4 Justin Dolske [:Dolske] 2009-03-09 15:00:05 PDT
(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).
Comment 5 Boris Zbarsky [:bz] 2009-03-09 17:41:24 PDT
> 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.
Comment 6 Boris Zbarsky [:bz] 2009-03-09 18:35:13 PDT
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.
Comment 7 moz_bug_r_a4 2009-03-10 00:24:21 PDT
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.
Comment 8 Boris Zbarsky [:bz] 2009-03-10 07:04:01 PDT
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.  :(
Comment 9 Boris Zbarsky [:bz] 2009-03-10 07:35:41 PDT
The "Proposed fix" patch applies directly to 1.9.0 and fixes the bug there too.
Comment 10 moz_bug_r_a4 2009-03-10 18:03:39 PDT
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?
Comment 11 Boris Zbarsky [:bz] 2009-03-10 18:10:27 PDT
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?
Comment 12 moz_bug_r_a4 2009-03-10 20:01:12 PDT
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.
Comment 13 Boris Zbarsky [:bz] 2009-03-10 20:15:55 PDT
OK.  I'll do the usual "wait till we ship" thing, I guess.  I hate that.  :(
Comment 14 Boris Zbarsky [:bz] 2009-03-16 08:02:34 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a1584533724 without the test.
Comment 15 Boris Zbarsky [:bz] 2009-03-16 08:03:01 PDT
Comment on attachment 366467 [details] [diff] [review]
Proposed fix

Need this on 1.9.0 as well.
Comment 16 Daniel Veditz [:dveditz] 2009-03-16 14:58:18 PDT
Comment on attachment 366467 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 17 Boris Zbarsky [:bz] 2009-03-16 18:16:38 PDT
Checked into CVS, still without test.
Comment 18 Al Billings [:abillings] 2009-03-19 16:38:08 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.