1356596 - (CVE-2017-7817) Spoof page in full screen mode Firefox Android with "scroll"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Raphael]

Attachment: poc1.html

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.112 Safari/537.36

Steps to reproduce:

1.Go to http://laraweron.mysit.ru/poc.html
2.Scroll down the page
3.Click on the link


Actual results:


In the browser version Firefox for Android v52.02, when entering full-screen mode, absent notification. Under certain circumstances, an attacker can replace a page or address page in the browser.When we scroll down the page in the browser, the address bar is automatically hidden, if the full-screen mode is enabled in this mode, the user will not notice it. When you navigate to another page in your browser, an error occurs.


Expected results:

The browser opens a new window in full-screen mode, deleting the address bar.

Verified by
Mozilla Firefox 52.02
Android 4.22
Android 5.0.1

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Sebastian, maybe you can take a first look at this?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

This is a dupe of one of Jordi's bugs. In release there is a black bar that appears where the address bar should be when the link is clicked which makes the 'phishing' fairly obvious. In beta and newer the address bar is shown then the 'phishing' image is shown below that. This works for me in beta and newer.

Comment 3

[Raphael]

1.In the browser there are no rules for switching to read mode, which makes it  insensibly switch it to full screen mode from the user.
2. There are no notifications about switching to full-screen mode
3. The browser does not have any instructions for switching links in full-screen mode Data URIs.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Flagging for triage and adding some of our front-end engineers to CC.

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Per discussion with Nevin, below two supposedly fix this issue.

Bug 1341276 - Clicking a link and open a new tab while fullscreen not working
Bug 1319366 - Clicking a link while presenting Google slides makes the navigation bar go black

Although, those were landed on 54 and after.
Raphael, could you try apply those patches and test again?

Comment 6

[Daniel Veditz [:dveditz]]

Randall: is this the same or different as some of the recent bugs you fixed in 53?

Comment 7

[Randall Barker [:rbarker]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> Randall: is this the same or different as some of the recent bugs you fixed
> in 53?

I don't believe it is related but also appears to be fixed in nightly at least.

Comment 8

[Randall Barker [:rbarker]]

So the real toolbar show up for me, but going into full screen without a prompt seems like a bad thing.

Comment 9

[Raphael]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #5)

> 
> Although, those were landed on 54 and after.
> Raphael, could you try apply those patches and test again?

Unfortunately I can not do this

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[triage]
as per comment7 we have those fixes in current nightly, and seems ok to have it just ride the train.
Ioana, can SV verify then we can have is FIXED at 55?

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Raphael from comment #9)
> > Although, those were landed on 54 and after.
> > Raphael, could you try apply those patches and test again?
> 
> Unfortunately I can not do this

Firefox Beta is available on the play store and is version 54 now so you can try this in those builds.

Comment 12

[Raphael]

I'm not sure that the patch will fix the error completely.
I rewrote the code. I deleted the redirection of the data:URL,using the pop-up method.
Test Poc#2
1.Open page https://laraweron.mysit.ru/poc_6.html

Comment 13

[Raphael]

Attachment: poc_6.html

Comment 14

[Raphael]

Has tested the bugs in the 54 version of Firefox Android poc1.html does not work.
In version 53,54(lastet)Andriod, with the help of poc6.html it is possible to spoof the attack, the address bar.

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to Randall Barker [:rbarker] from comment #8)
> going into full screen without a prompt seems like a bad thing.

Indeed -- that's the real problem here.

Comment 16

[Nevin Chen(Not active on Bugzilla)]

So I think the requirement / action item here is to add some notification when a user enter full screen?

Comment 17

[Daniel Veditz [:dveditz]]

Something to 1) let the user know it's full screen, and 2) how to get out

On Desktop we have a transition animation (fade-out/fade-in) and an overlay that slides over that announces the site is now full screen and Esc can be used to get out. Doesn't need to be the same thing, but needs to accomplish the same goals.

Comment 18

[Frederik Braun [:freddy]]

What's your timeline here, Nevin? We don't want security bugs to sit around stale.

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Hi

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Hi Anthonly
Could you please give me some UX guideline about commen 17?
Thanks!

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

hey Nevin, thanks for the NI!

This bug is tricky... I'm going to ask some questions because I want to understand some more - apologies! Just trying to figure out what the user problem is here and if a notification is the best way to solve it :)

When there is a page-load on scroll, what type of affordance are they getting from us (the browser app) here?

This issue is specially for when the user scrolls AND they're in full screen right?

If we have a notification, will they be able to see it if they're in "full screen"?

If we design a notification prompt before going into Full Screen, can't that notification UI also be spoofed?

Also, why do we expect that the user will remember this "state" of full-screen-ness if they saw the notification UI for it a few page-loads ago?

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Talked about this more with Jacqueline, our Privacy and Sec expert on the UX team, I'm gonna NI her for more help!

Comment 23

[Nevin Chen(Not active on Bugzilla)]

I'm imagine something like toast or snackbar when user go to fullscreen.
How do you think?

Comment 24

[Raphael]

Why this bug status to unconfirmed, when appointed the person ?

Comment 25

[Nevin Chen(Not active on Bugzilla)]

(In reply to Anthony Lam (:antlam) from comment #21)
> hey Nevin, thanks for the NI!
> 
> This bug is tricky... I'm going to ask some questions because I want to
> understand some more - apologies! Just trying to figure out what the user
> problem is here and if a notification is the best way to solve it :)
> 
> When there is a page-load on scroll, what type of affordance are they
> getting from us (the browser app) here?

I guess the bug now is not related to scrolling. We just need to let user know 
Something to 1) let the user know it's full screen, and 2) how to get out (per comment 17)

> This issue is specially for when the user scrolls AND they're in full screen
> right?
I think it's fullscreen in general

> If we have a notification, will they be able to see it if they're in "full
> screen"?

I think we can use snackbar or toast to notify the users. They are visible in full screen mode.

> 
> If we design a notification prompt before going into Full Screen, can't that
> notification UI also be spoofed?
> 

Snackbar is Native UI in Android. We don't have the ability to let web site send snackbar now.

> Also, why do we expect that the user will remember this "state" of
> full-screen-ness if they saw the notification UI for it a few page-loads ago?

The user should remember. And since we also hide Android status bar, it should be quit obvious.


Hi Daniel
Please correct me if I'm wrong.

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Attachment: Screenshot_2017-06-05-14-23-56.png

I've also attached the screenshot from Chrome as reference.
Is the what you want in comment 17?

Comment 27

[Jacqueline Savory [:jsavory] UX]

Adding in my two cents, 

I do think it would be great to make it more clear to the user when they enter fullscreen, however I'm not sure that the snackbar  will solve the security problem listed here. If the user is in a position where the browser and native UI has been replicated in a malicious manner, I'm worried that the user will not connect the fullscreen indicator with, 'this UI is malicious'. 

Unless the solution directly affects the UI to show that it is fake, I'm thinking it won't be clear enough. However, this will of course ruin the experience for people who are using fullscreen normally. This is a tough one and I'm thinking it might need a bit more exploration. :)

Comment 28

[Nevin Chen(Not active on Bugzilla)]

I'll unassign myself since there's no clear action item on this bug.
Please NI me again if there's a conclusion.

Comment 29

[Frederik Braun [:freddy]]

Jacqueline, what's the next step here?
A toast [1] when transitioning into fullscreen mode would work, no?
[1] https://developer.android.com/guide/topics/ui/notifiers/toasts.html

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Hi Jacqueline,

would parity with desktop and/or other browsers(see comment 17, using a toast or snackbar) not be an acceptable first step? 

I understand that the browser is less likely to occupy the entire screen on desktop, and so it's less likely to be susceptible to this kind of phish, meaning that the desktop solution may not solve the issue satisfactorily on mobile.

But right now we're not doing anything and that's worse.

If you require exploration or experimentation with a solution, can you provide a dev with a suggestion with what he actually implement for testing?

I'm clearing a bunch of needinfos here that I believe are no longer actual.

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to Nevin Chen [:nechen] from comment #25)
> (In reply to Anthony Lam (:antlam) from comment #21)
> > If we design a notification prompt before going into Full Screen, can't that
> > notification UI also be spoofed?

If you put the notification in the content-drawn area, sure, it could be spoofed.

1) why would you put it there? Why would you do it before going into fullscreen? On Desktop the notification overlay is shown after a transition animation. On Fennec you clear the status and notification bar--which a normal page can't do--and the notification would be to make sure the user has noticed these changes.

2) why would attackers spoof _entering_ fullscreen? Attackers don't want to spoof "fullscreen with 'fake' UI that's actually real because it's fake-fullscreen" -- they want to spoof that they are in non-fullscreen normal mode and that the fake "browser UI" the user sees is where the user really is. "I'm your bank! Please enter your password"

> > Also, why do we expect that the user will remember this "state" of
> > full-screen-ness if they saw the notification UI for it a few page-loads ago?

requestFullscreen() is tied to a DOM element (even if it's the whole document). When the user navigates to a new document that element is destroyed and fullscreen ends.

Comment 32

[Nevin Chen(Not active on Bugzilla)]

Hi Daniel.
I think you are replying to Anthony so I redirect the NI to him.

Comment 33

[Frederik Braun [:freddy]]

Assigning to Nevin Chen to implement a toast that comes up when entering fullscreen mode.

Comment 34

[Nevin Chen(Not active on Bugzilla)]

Hi Anthony
Please see comment 31
Thank you!

Comment 35

[Jacqueline Savory [:jsavory] UX]

Sorry for the delay on this one, I've talked with Anthony and we agree that testing the snackbar would be a great first step. I personally can't think of another UI solution for this case, so lets start with the snackbar. 

We were wondering if we could alter the frequency in which users see the snackbar and were hoping to test out a few options on our devices. The frequencies we would like to try are:
- Snackbar appears the first time the user enters fullscreen and never again
- Snackbar appears one time per website, but doesn't appear on that website after
- Snackbar appears every time user enters fullscreen

Let me know if this is possible, and thanks again for your patience!

Comment 36

[Nevin Chen(Not active on Bugzilla)]

All of them are possible. Just some concerns.
Just(In reply to Jacqueline Savory [:jsavory] UX from comment #35)
> - Snackbar appears the first time the user enters fullscreen and never again
This is possible by using Android SharedPreference

> - Snackbar appears one time per website, but doesn't appear on that website
Per website is kind of tricky. I'll need to compare what history somewhere.
And website-full-screen-spoofing may not related what website is entering full screen mode. So this restriction may not be pratical ?

> after
> - Snackbar appears every time user enters fullscreen
This is what Firefox Desktop and Chrome does. 
See http://labs.qnimate.com/full-screen-api/ for example.
I prefer this one :)

> Let me know if this is possible, and thanks again for your patience!

Hi Sebastian
Do you have any concerns if we go for "Snackbar appears every time user enters fullscreen"?
Thank you!

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nevin Chen [:nechen] from comment #36)
> Hi Sebastian
> Do you have any concerns if we go for "Snackbar appears every time user
> enters fullscreen"?
> Thank you!

No, I think showing a snackbar or toast is a good solution here.

Comment 38

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1356596.patch

Comment 39

[Nevin Chen(Not active on Bugzilla)]

Entering full screen in Nightly looks strange and not responsive today. But Beta/Release is fine. Not relate to this bug but I'll investigate further.

Comment 40

[Stephanie Ouillon [:arroway] (needinfo me)]

nechen, is there anything blocking this bug to move forward after your patch being r+?

Comment 41

[Nevin Chen(Not active on Bugzilla)]

Previously I saw full screen on nightly is jumpy. I'll do more testing before I land this patch.

Comment 42

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8887570 [details] [diff] [review]
1356596.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I'll consider it hard since we are only adding a toast message like Chrome.

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? release <= 56

If not all supported branches, which bug introduced the flaw? no

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? no

How likely is this patch to cause regressions; how much testing does it need? no

Comment 43

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

sec-approval+ for trunk. We'll want a beta patch nominated as well.

Comment 44

[Jacqueline Savory [:jsavory] UX]

Is it possible to get a video to see how the toast is working with fullscreen? Or is there another way I can test it? Thanks!

Comment 45

[Nevin Chen(Not active on Bugzilla)]

Attachment: device-2017-08-15-120057.mp4

Here's the demo video

Comment 46

[Nevin Chen(Not active on Bugzilla)]

Attachment: beta.patch

This patch is for beta

Comment 47

[Nevin Chen(Not active on Bugzilla)]

Attachment: central.patch

There are some changes/conflict in central. So I re-attach the patch again.
It should be fine to carry over the sec-approval+

Comment 48

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8897296 [details] [diff] [review]
beta.patch

Approval Request Comment
[Feature/Bug causing the regression]:n/a
[User impact if declined]:User won't see hint if he goes to full screen
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]: no
[Why is the change risky/not risky?]: just adding some hint
[String changes made/needed]: "fullscreen_warning":Drag from top and touch the back button to exit full screen.

Comment 49

[Nevin Chen(Not active on Bugzilla)]

btw, in comment 45. The status bar is gone after we hide full screen.
This issue is not related to the patch here.

Comment 50

[Jacqueline Savory [:jsavory] UX]

I'm wondering if we should change the text to be a bit clearer and reduce the length. The user will only need to figure out how to get out of full screen one time, the message that's more important here is that the user has gone into full screen mode. 

I suggest changing the text to something a bit simpler like: "Entered full screen"

Comment 51

[Nevin Chen(Not active on Bugzilla)]

I'll do as comment 50 if no other concerns.

Comment 52

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

n-i to flod since there are string changes suggested (for beta)

Comment 53

[Francesco Lodolo [:flod]]

Thanks for the heads-up. That string looks indeed quite long for Android.

Comment 54

[Francesco Lodolo [:flod]]

(adding Delphine to the bug, since she's the owner of localization for Firefox for Android).

Comment 55

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

Delphine, is this something we can localize for beta 56 or can you suggest a way to handle this issue?

Comment 56

[Delphine Lebédel [:delphine - Use needinfo]]

Yes, I think at this point we can still have this localized for beta56. Thanks for asking

Comment 57

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

Comment on attachment 8897296 [details] [diff] [review]
beta.patch

Fix for sec-high issue, let's land it on trunk and for 56 beta 6.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

The trunk patch needs rebasing. I'm going to hold off on the Beta landing until it's safely landed on trunk too. Given that it's Android-only, our next Beta isn't until next week anyway.

Comment 59

[Nevin Chen(Not active on Bugzilla)]

Attachment: mc-0824.patch

Let's try again for trunk

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I'll consider it hard since we are only adding a toast message like Chrome.

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? release <= 56

If not all supported branches, which bug introduced the flaw? no

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? no

How likely is this patch to cause regressions; how much testing does it need? no

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8900694 [details] [diff] [review]
mc-0824.patch

You don't need to re-request sec-approval once you've already gotten it. In the future, a better commit message would have been appreciated as well.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages

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

Comment 61

[Francesco Lodolo [:flod]]

It's already checked-in
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3fb981d0e10bcae8c73f6be6856fea266b30017

Comment 62

[Ryan VanderMeulen [:RyanVM]]

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

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a967f2b9796

Comment 64

[Francesco Lodolo [:flod]]

The string that landed in Beta is completely different from the one that landed in code. 

Looks like the Beta patch was never updated with "Entered full screen".
https://hg.mozilla.org/releases/mozilla-beta/rev/1a967f2b9796#l2.12
https://hg.mozilla.org/mozilla-central/rev/b3fb981d0e10#l2.13

Either the landing in beta is backed out (I have no idea if the code is the same), or a fix for the string alone needs to land ASAP.

Comment 65

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #64)
> Either the landing in beta is backed out (I have no idea if the code is the
> same), or a fix for the string alone needs to land ASAP.

I should have simply looked before commenting, it's 3 lines of code formatted differently. 

The only line that needs to change is the string
https://hg.mozilla.org/releases/mozilla-beta/rev/1a967f2b9796#l2.12

to

<!ENTITY fullscreen_warning "Entered full screen">

We don't have Fennec Beta exposed in Pontoon yet, so we're lucky, but until a fix lands we can't expose it either.

Comment 66

[Nevin Chen(Not active on Bugzilla)]

Sorry for the confusion.
I've added bug 1394347 for this.

Comment 67

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

Reporter, please do not mark the sec-bounty? flag on your own bugs. You need to contact us at [e-mail] (per Bounty Program instructions) normally to have your bug considered for the bounty. We have awarded a bounty for this though and I will be contacting you.

Comment 68

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jacqueline Savory [:jsavory] UX from comment #44)
> Is it possible to get a video to see how the toast is working with
> fullscreen? Or is there another way I can test it? Thanks!

(In reply to Nevin Chen [:nechen] from comment #45)
> Created attachment 8897270 [details]
> device-2017-08-15-120057.mp4
> 
> Here's the demo video

Thanks for posting this video Nevin! 

I'm sure everyone is always juggling multiple projects at Mozilla but its very hard for UX to review this without a NI' ping  since we don't watch the bugzilla comments go by as closely. So it looks like we missed your message this time :( .

In the future, could you please follow up before closing the issue? We obviously don't want to be a Blocker but this would just help us tremendously in keeping the product polished and super-high quality. If you could wait for a final review from UX, or just ping us on Slack! so that we know what's changed and what's not, that would really help us! 

Thanks!

Comment 69

[Nevin Chen(Not active on Bugzilla)]

Sorry Anthony
I'll be more careful in the future. 
I thought comment 50 is the latest comment. So I tag check-in needed after comment 60.
Next time I'll NI again before I land.

Is there anything I can do to make this bug better?

Thanks :)

Comment 70

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

No worries! I realized there may be some communication confusion - sorry about that!

We should leave this bug closed to avoid confusion. Regarding UX follow up, I'm not sure if Jacqueline has the capacity to go and review the UX again at this point. But we'll file follow up bugs if we see something come up.

Thanks!

Comment 71

[u549602]

Verified as fixed on the latest Nightly (57.0a1 2017-09-19) and latest Beta build 56.0b13.
This issue was tested on a Samsung Galaxy S6 EDGE (Android 7.0), a Samsung TabS3 (Android 6.0) and a Redmi Note 3 (Android 6.0.1)