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
3.27 KB, patch
|Details | Diff | Splinter Review|
957 bytes, patch
|Details | Diff | Splinter Review|
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.
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
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 any sparks?
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?
This is so for me, but sorry no progress. I'll try to look at this soon.
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.
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.
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)
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.
I enabled cross domain history and history.back() etc. and tested the other bugs, and this doesn't regress those.
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+
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
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+
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.
I folded the test from bug 978408 into the second patch for the branch uplifts. https://hg.mozilla.org/releases/mozilla-aurora/rev/75ce83516916 https://hg.mozilla.org/releases/mozilla-aurora/rev/4db655b66945
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.
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.
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.
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.
I think the existing reasoning stands and we should probably not take this at the last minute.
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-esr24? → approval-mozilla-esr24+
https://hg.mozilla.org/releases/mozilla-esr24/rev/8ca0fab764c0 https://hg.mozilla.org/releases/mozilla-esr24/rev/a606379e17c9 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/df2cd7737e7a https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e6f6438a1732 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/df2cd7737e7a https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e6f6438a1732 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cf446bc80f25 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7dd7bac99876
Confirmed issue on Fx28 using both tests. Verified fixed on Fx24esr, Fx29 and Fx30.
You need to log in before you can comment on or make changes to this bug.