1301876 - (CVE-2017-5421) Print preview hijacking leads to potential spoof

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Abdulrahman Alqabandi]

Attachment: PoC-print.html

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

Steps to reproduce:

This requires popups to be allowed. If we open a url within onbeforeprint event, we can trick a victim into thinking theyre in a trusted website.

Open attached PoC and follow instructions.


Actual results:

We are able to hijack the print preview window with any HTML and javascript we like. 


Expected results:

New content should have been blocked, as usually print preview has no javascript in it. But having these active contents might be a problem.

Comment 1

[Abdulrahman Alqabandi]

Attachment: Screenshot of the result

Comment 2

[Abdulrahman Alqabandi]

Attachment: PoCImg2.png

In this screenshot, I used an image of a real google websites addressbar, which sells it even more.

Comment 3

[:Gijs (he/him)]

Olli, seeing as you recently looked at this stuff in bug 1214805, I'm guessing you might be the best person to look at what's going on here? I *think* navigation should be blocked here, not sure why it isn't.

Comment 4

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

But the navigation doesn't happen on the window where beforeprint is dispatched.
And if any background tab opens a new window, it leads to this weird case.

FF UI shouldn't let other tabs to become the foreground tabs while showing pp.

Comment 5

[:Gijs (he/him)]

Attachment: Patch v0.1

Jared, are you up for reviewing this? I'd ask Mike but he's not taking more reviews right now.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

Review of attachment 8790652 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +3261,5 @@
>                                                      relatedBrowser: browser });
>        gBrowser.selectedTab = this._printPreviewTab;
> +      // NB: we do this here rather than in onEnter because when onEnter happens
> +      // we could already have switched tabs (via either user or website actions).
> +      gBrowser.tabSwitchingEnabled = false;

This is a bit scary how the comment describes this as being so racy. And what happens if entering print preview fails, does that mean tabbrowser is effectively broken as it won't allow any tab changes until Firefox is restarted?

::: browser/base/content/tabbrowser.xml
@@ +2978,5 @@
>            <![CDATA[
>            // Update the tab
> +          if (this.tabSwitchingEnabled) {
> +            this.mTabBox.selectedTab = val;
> +          }

Is this right? 

let a = b = 5;

Should `a` == 5 if we are unable to set `b` to 5? Or should `a` be set to the original value of `b`, thus `undefined` in this case.

Testing this in the JS Console, I did:
var d;
var c = d = (function() { throw 1; })();
and afterwards c == undefined as well as d == undefined.

Therefore, if we are unable to set `this.mTabBox.selectedTab` to val, then we should return `this.mTabBox.selectedTab` instead of val.

Comment 7

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8790652 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8790652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +3261,5 @@
> >                                                      relatedBrowser: browser });
> >        gBrowser.selectedTab = this._printPreviewTab;
> > +      // NB: we do this here rather than in onEnter because when onEnter happens
> > +      // we could already have switched tabs (via either user or website actions).
> > +      gBrowser.tabSwitchingEnabled = false;
> 
> This is a bit scary how the comment describes this as being so racy.

Well, the current thing isn't racy, adding it in onEnter (which was the first thing I tried) would be racy.

> And
> what happens if entering print preview fails, does that mean tabbrowser is
> effectively broken as it won't allow any tab changes until Firefox is
> restarted?

AIUI onExit will always be called if this code runs, so I don't think that can happen. Mike would probably know for sure?

And I agree that's scary, but equally I don't see how else we're going to do this. We do want to actually block the navigations while in PP.

> ::: browser/base/content/tabbrowser.xml
> @@ +2978,5 @@
> >            <![CDATA[
> >            // Update the tab
> > +          if (this.tabSwitchingEnabled) {
> > +            this.mTabBox.selectedTab = val;
> > +          }
> 
> Is this right? 
> 
> let a = b = 5;
> 
> Should `a` == 5 if we are unable to set `b` to 5? Or should `a` be set to
> the original value of `b`, thus `undefined` in this case.
> 
> Testing this in the JS Console, I did:
> var d;
> var c = d = (function() { throw 1; })();
> and afterwards c == undefined as well as d == undefined.
> 
> Therefore, if we are unable to set `this.mTabBox.selectedTab` to val, then
> we should return `this.mTabBox.selectedTab` instead of val.

TBH, I expect more code to fail if we do that than what the patch does. If code does:

let myTab = gBrowser.selectedTab = gBrowser.addTab(...);

do we really want myTab to now be the wrong reference?

(this pattern is pretty common in tests, see http://searchfox.org/mozilla-central/search?q=%3D%20gBrowser.selectedTab%20%3D , which makes me think add-ons might use it, too)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #7)
> TBH, I expect more code to fail if we do that than what the patch does. If
> code does:
> 
> let myTab = gBrowser.selectedTab = gBrowser.addTab(...);
> 
> do we really want myTab to now be the wrong reference?
> 
> (this pattern is pretty common in tests, see
> http://searchfox.org/mozilla-central/search?q=%3D%20gBrowser.
> selectedTab%20%3D , which makes me think add-ons might use it, too)

Right, which is why I thought it important to call out. I would prefer that we follow the standard way that assignment is implemented here. It's much easier to reason about than if we were to use some other behavior that isn't the norm.

I would like to think a little more about the onExit part first. I'll leave needinfo flag on the bug so I don't forget.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

I looked a little bit more in to this. I would like it if we could wrap the enter/exit with a try/finally and clear `tabSwitchingEnabled` in the finally, but we don't have a simple code path here. The code is mainly event based and looks to have entrances from multiple paths.

Mike, what do you think?

Comment 10

[:Gijs (he/him)]

One other thing we could do, which I didn't realize until long after I wrote this patch, is somehow try to force the print preview browser to stay on top. AIUI we create a separate browser for the print preview. It's not clear to me why that browser gets moved out of view for the newly-focused tab. If the new browser was "always on top" as it were, we wouldn't have to mess with tab switching in the browser (though of course the user could then still end up on a different tab than where they started from, when closing print preview).

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I dunno - fixing this in the front-end feels like an end-of-pipe solution. Why would we even support calling window.open from within onbeforeprint? Can we check to see if we're in one of these event handlers and NS_ERROR_FAILURE our way out of window.open in that case instead?

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #11)
> I dunno - fixing this in the front-end feels like an end-of-pipe solution.
> Why would we even support calling window.open from within onbeforeprint? Can
> we check to see if we're in one of these event handlers and NS_ERROR_FAILURE
> our way out of window.open in that case instead?

As Olli pointed out in comment #4, the same thing would happen if you had other background windows that did this. That's already confusing for users if it happens "by accident". If a malicious actor wanted to abuse this problem, they could easily do so via named windows and call window.open from somewhere else. The navigation doesn't happen in the window that's in print preview (and/or the one that fires beforeprint).

Comment 13

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

This is definitely a front-end issue. The issue isn't, in general, the page which gets before/afterprint but all the tabs user has open.

Comment 14

[:Gijs (he/him)]

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

(In reply to :Gijs Kruitbosch from comment #10)
> One other thing we could do, which I didn't realize until long after I wrote
> this patch, is somehow try to force the print preview browser to stay on
> top. AIUI we create a separate browser for the print preview. It's not clear
> to me why that browser gets moved out of view for the newly-focused tab. If
> the new browser was "always on top" as it were, we wouldn't have to mess
> with tab switching in the browser (though of course the user could then
> still end up on a different tab than where they started from, when closing
> print preview).

So it turns out the print preview browser is just another tabbed browser, so this isn't really any different from the existing patch.

Effectively, I think comment #9 devolves to r?mconley, so I'm going to set that.

One thing that may or may not make this more palatable from a "don't mess up the tabbrowser" perspective, but potentially more jarring for the user, is if we only disable the tabswitching in Enter(), but then also re-force the print preview tab to become the selected tab (and remove the tabswitch-disabling in Exit() as is done now).

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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

Review of attachment 8790652 [details] [diff] [review]:
-----------------------------------------------------------------

I have a safety suggestion - see below.

::: browser/base/content/browser.js
@@ +3261,5 @@
>                                                      relatedBrowser: browser });
>        gBrowser.selectedTab = this._printPreviewTab;
> +      // NB: we do this here rather than in onEnter because when onEnter happens
> +      // we could already have switched tabs (via either user or website actions).
> +      gBrowser.tabSwitchingEnabled = false;

I, like jaws, am worried about unforeseen conditions that might result in our user being unable to switch tabs in this window - for example, something going awfully wrong in the teardown of print preview.

Ideally, that'd be as impossible a condition as possible. Could we perhaps have some kind of safeguard, where if the user mouseenters the tabbrowser-tabs (which is normally hidden in print preview), we ensure that tabSwitchingEnabled is true? How do you feel about that - overkill? I'm just antsy about unknown unknowns here, since we're effectively adding a switch to a critical function of the browser.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

This bug just went public:

https://twitter.com/Qab/status/778022079350378496

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Capture of Tweet

:/

Comment 18

[Abdulrahman Alqabandi]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #17)
> Created attachment 8793530 [details]
> Capture of Tweet
> 
> :/

I didn't add any PoC, is the screenshot alone considered public disclosure?

Comment 19

[:Gijs (he/him)]

(In reply to Abdulrahman Alqabandi[test] from comment #18)
> (In reply to Mike Conley (:mconley) - (needinfo me!) from comment #17)
> > Created attachment 8793530 [details]
> > Capture of Tweet
> > 
> > :/
> 
> I didn't add any PoC, is the screenshot alone considered public disclosure?

Not necessarily, but in this particular case, I think the screenshot is pretty suggestive of how you'd exploit.

In any case, this bug is going to need a sec-rating and I need to update the patch, which I hope to do later today.

Comment 20

[Abdulrahman Alqabandi]

I guess people could guess how its done, tweet deleted.

Comment 21

[:Gijs (he/him)]

Attachment: Patch v0.2

I think this also works, and will be automatically unbroken when the navtoolbox is no longer collapsed, which seems like it would be enough to avoid permanently bricking stuff. There's a little flicker in the testcase for me, but that seems pretty OK given that this won't actually happen in "normal" usecases (only if the webpage tries to spoof you like the PoC).

I can look at doing a testcase for this but without a sec-rating I don't know if it'll even land when we fix this, so I'll leave that for now.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

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

Review of attachment 8795764 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is so much simpler. Thanks Gijs!

Comment 23

[Daniel Veditz [:dveditz]]

Calling this spoof sec-low (as are most spoofs).
 * only affects the tiny minority who disable the pop-up blocker
 * you have to convince a user to print preview your page (not a common action)
 * You're printing a page from foo.com and suddenly it turns into mybank.com?
   that's suspicious, why would the user believe that?
 * it's a print preview, not an interactive page. What can you get out of it?

Comment 24

[:Gijs (he/him)]

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

Comment 25

[:Gijs (he/him)]

Huh, when filing a bug for having an automated test, noticed bug 148892 and bug 244705 and bug 601248. Once this hits Nightly I'll see if those bugs can be resolved. AIUI we can open up sec-low spoofing bugs so I might just dupe the open ones here and make this public.

Comment 26

[Carsten Book [:Tomcat]]

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