1322896 - (CVE-2017-7787) (cross domain) Iframe breakes scope on location.reload

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Oliver Wagner]

Attachment: ff-iframes-break-scope.zip

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.

Comment 1

[Oliver Wagner]

Attachment: ff-get-username.zip

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.

Comment 2

[Daniel Veditz [:dveditz]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Olli, do you have time to look?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: session_history_fragment_navigation_and_reload.diff

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).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

bz, would you have time to review the patch?
If not, I'll ask Samael.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(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)

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: iframe_reload.html

a test. I'll try some variants too.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Without the patch reload overrides the fragment navigation, and same with the patch.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8849692 [details] [diff] [review]
session_history_fragment_navigation_and_reload.diff

r=me

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

I'd rather not take this for beta 53, as we don't have much time for the real world testing smaug mentions.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5931a23fe03ef4a9769b4ad7d333ba3d2e3479a

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a5931a23fe03

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an Aurora approval request?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

No. This is super regression risky. If possible, could we not take this to branches.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Olli, can we get an ESR52 patch for this? It has been about two months. :-)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c254d3cc826c

Comment 25

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.