Closed Bug 895557 (CVE-2014-1530) Opened 7 years ago Closed 6 years ago

It's possible to set a document's URI to a different document's URI by confusing docshell

Categories

(Core :: DOM: Navigation, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- wontfix
firefox28 + wontfix
firefox29 + verified
firefox30 + verified
firefox-esr24 29+ verified
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: moz_bug_r_a4, Assigned: smaug)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [adv-main29+][adv-esr24.5+])

Attachments

(2 files, 2 obsolete files)

Bug 803870 was fixed by disallowing cross-origin history access, but the key problem is not fixed and it is still abusable with subframe.

the problem is:

history.forward(); // A
location.hash='a'; // B

1) A sets mLSHE to a SHEntry to be loaded.
2) B sets mLSHE to null and sets mOSHE to a new SHEntry.
3) the history load is processed, but nsDocShell::Embed does not update mOSHE since mLSHE is null.
This works on fx17,22-25.

Target page is https://www.rooms.hp.com/signin.aspx

This creates a fake login page whose URI is the target page's URI. And, if you already saved a password for the target page, the Password Manager will automatically fill in the password.
Attached file testcase 2 - XSS
This works on fx17,22-25.

Target page is http://www8.hp.com/us/en/sitemap.html

This injects a script that opens an alert dialog with cookies into the target page.
Please use the following URL to run testcase 2:

jar:https://bugzilla.mozilla.org/attachment.cgi?id=777994!/x.html
Component: Security → Document Navigation
I think we need justin or bz or someone else who understands docshell to fix this. My involvement in bug 803870 was just at level of cross-origin history access, which we knew wasn't the underlying problem and apparently isn't enough to fix the security bug.
I guess I should take this then.
Assignee: nobody → bugs
Olli, ping? Can you look at this reasonably soon, or do we need to find another owner here?
Olli, any progress on this?  Is there somebody else who can look at this?
Flags: needinfo?(bugs)
This is so for me, but sorry no progress. I'll try to look at this soon.
Flags: needinfo?(bugs)
One month reminder! ;) This is the oldest open DOM sec-high or -critical bug.
So as far as I see this is a regression from bug 791011, but this was in the tree also before
Bug 775009.
Blocks: 791011
Or actually, even the fix for bug 775009 was just missing some cases, so not a regression.
No longer blocks: 791011
Oh, there is also the fix for bug 757376 involved.
And bug 737307. Patches to all those had some issues.
Attached patch WIP (v2) (obsolete) — Splinter Review
Need to go through the tests for those other xss bugs, and session history mochitests.
This is super regression prone stuff, but this should, assuming the correct interpretation, actually bring us closer
to the spec. "cancel any preexisting but not yet mature attempt to navigate the browsing context"
Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

While I'm testing the stuff, does this ring any alarm bells?
Attachment #8375183 - Flags: feedback?(bzbarsky)
I need to still test unload handling. We probably should prevent hash navigation during unload
(follow the spec better there). That doesn't affect to the xss issue here though.
Ah, but we deal with unload already.
Attachment #8375183 - Flags: feedback?(bzbarsky) → review?(bzbarsky)
Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

I believe this patch would break this testcase:

  <a href="#" onclick="location = 'http://mozilla.org'">Click me</a>

which should load mozilla.org.  :(
Attachment #8375183 - Flags: review?(bzbarsky) → review-
Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

Bah, this is too strict.
Attachment #8375183 - Flags: review?(bzbarsky)
Attached patch WIP (v3) (obsolete) — Splinter Review
Need to test still other relevant bugs, but something like this should be
right.
Attachment #8375183 - Attachment is obsolete: true
Attachment #8382676 - Flags: feedback?(bzbarsky)
Comment on attachment 8382676 [details] [diff] [review]
WIP (v3)

This seems fairly plausible if it doesn't regress bug 775009.

That said, why do we need the JustStartedNetworkLoad() conditional?  Why can we not unconditionally set lsheForNewlyStartedLoad = mLSHE?
Attachment #8382676 - Flags: feedback?(bzbarsky) → feedback+
Yeah, we could probably remove that condition.

Bug 775009 as such doesn't regress, but I need to try a bit different testcase which doesn't
rely on history object being accessible cross domain.
Attached patch v4Splinter Review
I enabled cross domain history and history.back() etc. and tested the other
bugs, and this doesn't regress those.
Attachment #8382676 - Attachment is obsolete: true
Attachment #8383397 - Flags: review?(bzbarsky)
Comment on attachment 8383397 [details] [diff] [review]
v4

r=me
Attachment #8383397 - Flags: review?(bzbarsky) → review+
Comment on attachment 8383397 [details] [diff] [review]
v4

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I guess the commit message could say something like:
"Make fragment loading work per spec even in case of session history loads".
(because that is another aspect the patch fixes)
I should actually add a testcase for that. It wouldn't reveal anything about this bug.


Which older supported branches are affected by this flaw?
All

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply with some --fuzz

How likely is this patch to cause regressions; how much testing does it need?
This is regression prone. Needs as much testing time it can get.
Attachment #8383397 - Flags: sec-approval?
Or, I could add the testcase to bug 775009 and make it use todo(), 
and then change that todo() to is() in this bug.
Comment on attachment 8383397 [details] [diff] [review]
v4

sec-approval+ for trunk. Please make and nominate Aurora, Beta, and ESR24 patches ASAP after it lands.
Attachment #8383397 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/90fb37782bb5
https://hg.mozilla.org/mozilla-central/rev/cc271727af48
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8383397 [details] [diff] [review]
v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old stuff
User impact if declined: xss
Testing completed (on m-c, etc.): landed to m-c 
Risk to taking this patch (and alternatives if risky): risky. Needs real world testing. Artificial testcases are not enough.
String or IDL/UUID changes made by this patch: NA
Attachment #8383397 - Flags: approval-mozilla-esr24?
Attachment #8383397 - Flags: approval-mozilla-beta?
Attachment #8383397 - Flags: approval-mozilla-aurora?
Comment on attachment 8383397 [details] [diff] [review]
v4

So we only have a week of "real world testing" left for FF28 - what happens if we don't take this up to beta/esr24 on this release?  Can this wait for a full cycle on Beta with 29?
Attachment #8383397 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bugs)
This bug has been open for months, but that actually doesn't matter.
What matters is whether it is easy to construct the exploit from the patch.
I'd say it is not very easy.

Could we re-evaluate beta/esr24 after couple of days testing in m-c?
I'd like to see at least some Nightly testing.
Flags: needinfo?(bugs)
It's been a couple of days and our last 28 beta goes to build today - what have the few days on central showed?  What's the risk/reward here for landing at this late point?  If there's risk to stability of the product, it would be best to wait one more cycle.
Flags: needinfo?(bugs)
I haven't heard of any regressions.

Risk is that some page navigations don't work as expected, so it is not about stability, but
whether web pages work as expected.
Reward is to fix this xss.

I think waiting for one more cycle is ok. Perhaps dveditz has some opinion here.
Flags: needinfo?(bugs) → needinfo?(dveditz)
Since it is already a problem on 27, I think we can "won't fix" for 28 and let it ride the trains.
Flags: needinfo?(dveditz)
Comment on attachment 8383397 [details] [diff] [review]
v4

I would like to see this on ESR24 but I don't want to destabilize it just before release either so I think this should wait and land there after we make release builds.
Attachment #8383397 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
v1.1 will be EOL by the time this is approved for esr24, so wontfix for b2g18.
FWIW, this applies cleanly all the way down to esr24 and is green on Try on all branches. If you want to take this as a Pwn2Own ride-along fix, it shouldn't get in the way.
Flags: needinfo?(abillings)
I think the existing reasoning stands and we should probably not take this at the last minute.
Flags: needinfo?(abillings)
Attachment #8383397 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
This is a) only sec-high but also b) carries risk of web incompatibility and/or site breakage so we wouldn't take this as a last-minute landing to esr24 that is shipping with FF28.  It can wait a cycle.
Comment on attachment 8383397 [details] [diff] [review]
v4

Will leave the esr24 nomination for uplift until after we're clear of ESR24.4.0
Attachment #8383397 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8383397 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Confirmed issue on Fx28 using both tests.
Verified fixed on Fx24esr, Fx29 and Fx30.
Whiteboard: [adv-main29+][adv-esr24.5+]
Alias: CVE-2014-1530
Group: core-security
You need to log in before you can comment on or make changes to this bug.