2.76 KB, application/java-archive
5.07 KB, application/java-archive
1.42 KB, patch
|Details | Diff | Splinter Review|
229 bytes, text/html
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36 Steps to reproduce: Please open ff-iframe-break-scope.zip in the attachment to reproduce the bug. I have a website (index.html). On that website i include Iframe 1 (iframe-1.html) with <iframe ...>. Also there is a link to reload the page with location.reload(). In Iframe 1 (iframe-1.html) there I create a new Iframe with document.createElement. In Iframe 2 (iframe-2.html) I create another new Iframe. I have a Iframe Element without src-attribute in the page. And after a timeout I Iframe-Element gets a Source. Iframe 3 only contains some HTML. --- Important: This bug also works with 3rd party (cross domain) Iframes. In online advertising this is not a unusal construction. In ff-get-username.zip I use this bug to get the autofill username from a page. Just start with step-1.html. Actual results: If I press the "Click me" link twice, the page doesn't reload. Instead the Content from the last Iframe ist shown. Expected results: The page should reload.
This bug can be used to get autofil-usernames on a website from "inside" a cross domain iframe / 3rd party advertising. Step 1: Fill a username and password on step-1.html Step 2: Save the username and password Step 3: The Browser autofills the first form. If I now press the link twice, the Cross domain Iframe with the login form will be break out and the username is filled in. So a 3rd party advert can stole the username.
Attachment #8817863 - Attachment mime type: application/zip → application/java-archive
Attachment #8817864 - Attachment mime type: application/zip → application/java-archive
I confirmed the first testcase on ESR-45.5 and Firefox 50.0.2 (with and without e10s). You have to wait for the words "Last IFrame" to reappear after the first click before clicking the second time. I cannot reproduce a similar effect from clicking the reload button or Cmd-R, not even via manual "location.reload()" in the web console even though that's all the link's onclick handler does. In fact I used the console to dump location at each step and it kept saying the outer URL even immediately before clicking on the link where location.reload() loaded iframe-3 instead. This is an intriguing mystery
Group: firefox-core-security → dom-core-security
status-firefox50: --- → affected
status-firefox-esr45: --- → affected
Component: Untriaged → Document Navigation
Product: Firefox → Core
Status: UNCONFIRMED → NEW
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Ever confirmed: true
Olli, do you have time to look?
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
Somehow we have mLSHE without link to parent in a child docshell and then that gets assigned as the mOSHE of the root docshell when reload happens first time. And then the next reload just uses url from mOSHE...
Ok, so the testcase does reload and fragment navigation simultaneously and also adds iframe dynamically etc. The (or an) issue is that SHistory ends up pointing to SHEntry for fragment navigation but DocShell's mLSHE (and later mOSHE) points to the entry for the reload (which was initiated before the fragment navigation).
bz, would you have time to review the patch? If not, I'll ask Samael.
Comment on attachment 8849692 [details] [diff] [review] session_history_fragment_navigation_and_reload.diff So just to make sure I understand correctly... Without the patch, we start the reload, that sets mLSHE. At this point the session history is still pointing to mOSHE? Then we do a fragment navigation. This creates an updated entry in the session history, which no longer points to either mOSHE or mLSHE. Is that correct? And the patch is changing the behavior of which? The fragment navigation, or the OnNewURI we get when the reload actually gets to the point where we are about to create the document for it? Why do we only change the mSessionHistory case? What if this docshell is a subframe (so doesn't have its own mSessionHistory)?
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7) > Comment on attachment 8849692 [details] [diff] [review] > session_history_fragment_navigation_and_reload.diff > > So just to make sure I understand correctly... > > Without the patch, we start the reload, that sets mLSHE. At this point the > session history is still pointing to mOSHE? yes > > Then we do a fragment navigation. This creates an updated entry in the > session history, which no longer points to either mOSHE or mLSHE. mOSHE points to the fragment navigation and also the shistory. Then reload ends, but it is doing replace, not shistory addition, so mOSHE points to reloaded entry and shistory to fragment entry. And because we remove dynamically added entries when unloading, the fragment entry contains only partial session history tree vs. what mOSHE has > > Is that correct? And the patch is changing the behavior of which? The patch is changing the behavior so that fragment navigation effectively disappears. Other option would be to change reload behavior so that it becomes non-reload but a new entry. That means there would be 3 entries in the shistory. The original, fragment, copy-of-the-original > Why do we only change the mSessionHistory case? What if this docshell is a > subframe (so doesn't have its own mSessionHistory)? Because we care about the SHEntry of the current index in shistory to start looking for how to modify shistory when doing iframe loads. See GetEntryAtIndex calls. (and yes, this is complicated)
So just so I'm clear. With this patch, if I do the "start reload, then do fragment navigation" thing in a subframe, what does the shistory end up looking like? What about without this patch?
ah, that kind of case. Let me test, but I don't think it should matter whether fragment navigation happens in subframe. Reload should still overwrite. And without the patch we should have still similar issue as with top level fragment navigation. But need to test.
a test. I'll try some variants too.
Without the patch reload overrides the fragment navigation, and same with the patch.
Comment on attachment 8849692 [details] [diff] [review] session_history_fragment_navigation_and_reload.diff r=me
Attachment #8849692 - Flags: review+
Comment on attachment 8849692 [details] [diff] [review] session_history_fragment_navigation_and_reload.diff [Security approval request comment] How easily could an exploit be constructed based on the patch? Not very easily, I think Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Checking comment could be: "Bug 1322896, synchronize SHistory entry with mLSHE, r=bz" That doesn't tell much how the issue can be used in a bad way. Which older supported branches are affected by this flaw? All If not all supported branches, which bug introduced the flaw? This is ancient stuff Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Same patch applies to branches How likely is this patch to cause regressions; how much testing does it need? This is extremely regression risky, as session history changes tend to be. Needs as much testing as possible, and testing here means real world web usage. (There is no reasonable spec for session history. What is currently in the spec doesn't map at all to what browsers do, and then all the browsers do things a bit differently and have bugs.)
Attachment #8849692 - Flags: sec-approval?
sec-approval+ for trunk. I think we may want to take this on Aurora once it is on trunk but I'm concerned about taking it further because of the identified regression risks.
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
status-firefox52: affected → wontfix
status-firefox54: --- → affected
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → +
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
Attachment #8849692 - Flags: sec-approval? → sec-approval+
I'd rather not take this for beta 53, as we don't have much time for the real world testing smaug mentions.
status-firefox53: affected → wontfix
tracking-firefox53: ? → +
tracking-firefox54: ? → +
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this ready for an Aurora approval request?
No. This is super regression risky. If possible, could we not take this to branches.
Per IRC discussion with smaug, this is something we might still want to take on ESR52 at some point after it has shipped on release (i.e. 52.3 at a minimum, maybe later) and looks good from a regression standpoint, so leaving that set to affected while marking the others as wontfix.
status-firefox54: affected → wontfix
status-firefox-esr45: affected → wontfix
tracking-firefox-esr45: ? → ---
Olli, can we get an ESR52 patch for this? It has been about two months. :-)
Comment on attachment 8849692 [details] [diff] [review] session_history_fragment_navigation_and_reload.diff [Approval Request Comment] See comment 14 Fix Landed on Version: 55
Attachment #8849692 - Flags: approval-mozilla-esr52?
Attachment #8849692 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
status-firefox-esr52: affected → fixed
Whiteboard: [adv-main55+] → [adv-main55+][adv-esr52.3+]
Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]
I was able to reproduce the initial issue on 53.0a1 (2016-12-11). I can confirm the issue is fixed on 55.0b13 build1 (20170727114534) and 52.2.1esr build2 (20170627155318) using Windows 10 x64, Ubuntu 16.04 x86 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox-esr52: fixed → verified
You need to log in before you can comment on or make changes to this bug.