Closed Bug 1356596 (CVE-2017-7817) Opened 3 years ago Closed 2 years ago

Spoof page in full screen mode Firefox Android with "scroll"

Categories

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

52 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 + verified
firefox57 + verified

People

(Reporter: Laraweron, Assigned: cnevinchen)

Details

(Keywords: csectype-spoof, sec-high, Whiteboard: [adv-main56+][post-critsmash-triage])

User Story

Skip to comment 12. Unrelated fixes broke the original POC but didn't fix the problem which is going into full-screen mode with no notice (or instruction on how to get out).

Attachments

(6 files, 2 obsolete files)

Attached file 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
Sebastian, maybe you can take a first look at this?
Flags: needinfo?(s.kaspari)
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.
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.
Flagging for triage and adding some of our front-end engineers to CC.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
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?
Flags: needinfo?(Laraweron)
Randall: is this the same or different as some of the recent bugs you fixed in 53?
Flags: needinfo?(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.
Flags: needinfo?(rbarker)
So the real toolbar show up for me, but going into full screen without a prompt seems like a bad thing.
(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
Flags: needinfo?(Laraweron)
Assignee: nobody → cnevinchen
tracking-fennec: ? → +
Priority: -- → P1
[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?
Flags: needinfo?(ioana.chiorean)
(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.
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
Attached file poc_6.html
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.
Flags: needinfo?(dveditz)
Attachment #8858338 - Attachment filename: poc1.html → poc1.html (OBSOLETE)
(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.
User Story: (updated)
Flags: needinfo?(dveditz)
So I think the requirement / action item here is to add some notification when a user enter full screen?
Flags: needinfo?(whuang)
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
What's your timeline here, Nevin? We don't want security bugs to sit around stale.
Flags: needinfo?(cnevinchen)
Hi
Flags: needinfo?(cnevinchen)
Hi Anthonly
Could you please give me some UX guideline about commen 17?
Thanks!
Flags: needinfo?(alam)
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?
Flags: needinfo?(alam) → needinfo?(cnevinchen)
Talked about this more with Jacqueline, our Privacy and Sec expert on the UX team, I'm gonna NI her for more help!
Flags: needinfo?(jsavory)
I'm imagine something like toast or snackbar when user go to fullscreen.
How do you think?
Flags: needinfo?(cnevinchen) → needinfo?(alam)
Why this bug status to unconfirmed, when appointed the person ?
(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.
Flags: needinfo?(dveditz)
I've also attached the screenshot from Chrome as reference.
Is the what you want in comment 17?
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. :)
Flags: needinfo?(jsavory)
I'll unassign myself since there's no clear action item on this bug.
Please NI me again if there's a conclusion.
Assignee: cnevinchen → nobody
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
Flags: needinfo?(jsavory)
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.
Flags: needinfo?(whuang)
Flags: needinfo?(ioana.chiorean)
(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.
Flags: needinfo?(dveditz) → needinfo?(cnevinchen)
Hi Daniel.
I think you are replying to Anthony so I redirect the NI to him.
Flags: needinfo?(cnevinchen)
Assigning to Nevin Chen to implement a toast that comes up when entering fullscreen mode.
Assignee: nobody → cnevinchen
Hi Anthony
Please see comment 31
Thank you!
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!
Flags: needinfo?(jsavory)
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!
Flags: needinfo?(s.kaspari)
Flags: needinfo?(jsavory)
(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.
Flags: needinfo?(s.kaspari)
Attached patch 1356596.patch (obsolete) — Splinter Review
Attachment #8887570 - Flags: review?(s.kaspari)
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8887570 - Flags: review?(s.kaspari) → review+
nechen, is there anything blocking this bug to move forward after your patch being r+?
Flags: needinfo?(cnevinchen)
Previously I saw full screen on nightly is jumpy. I'll do more testing before I land this patch.
Flags: needinfo?(cnevinchen)
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
Attachment #8887570 - Flags: sec-approval?
sec-approval+ for trunk. We'll want a beta patch nominated as well.
Attachment #8887570 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(alam)
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!
Flags: needinfo?(jsavory)
Here's the demo video
Flags: needinfo?(jsavory)
Attached patch beta.patchSplinter Review
This patch is for beta
Attached patch central.patch (obsolete) — Splinter Review
There are some changes/conflict in central. So I re-attach the patch again.
It should be fine to carry over the sec-approval+
Attachment #8897299 - Flags: sec-approval?
Attachment #8897299 - Flags: review+
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.
Attachment #8897296 - Flags: approval-mozilla-beta?
btw, in comment 45. The status bar is gone after we hide full screen.
This issue is not related to the patch here.
Attachment #8897299 - Flags: sec-approval? → sec-approval+
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"
Flags: needinfo?(jsavory)
I'll do as comment 50 if no other concerns.
n-i to flod since there are string changes suggested (for beta)
Flags: needinfo?(francesco.lodolo)
Thanks for the heads-up. That string looks indeed quite long for Android.
Flags: needinfo?(francesco.lodolo)
(adding Delphine to the bug, since she's the owner of localization for Firefox for Android).
Delphine, is this something we can localize for beta 56 or can you suggest a way to handle this issue?
Flags: needinfo?(lebedel.delphine)
Yes, I think at this point we can still have this localized for beta56. Thanks for asking
Flags: needinfo?(lebedel.delphine)
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.
Attachment #8897296 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(cnevinchen)
Attached patch mc-0824.patchSplinter Review
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
Attachment #8887570 - Attachment is obsolete: true
Attachment #8897299 - Attachment is obsolete: true
Flags: needinfo?(cnevinchen)
Attachment #8900694 - Flags: sec-approval?
Attachment #8900694 - Flags: review+
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
Attachment #8900694 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/b3fb981d0e10
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Group: firefox-core-security → core-security-release
Flags: sec-bounty?
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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(cnevinchen)
(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.
Sorry for the confusion.
I've added bug 1394347 for this.
Flags: needinfo?(cnevinchen)
Flags: needinfo?(ryanvm)
Reporter, please do not mark the sec-bounty? flag on your own bugs. You need to contact us at security@mozilla.org (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.
Flags: sec-bounty? → sec-bounty+
(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!
Flags: needinfo?(cnevinchen)
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 :)
Flags: needinfo?(cnevinchen) → needinfo?(alam)
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!
Flags: needinfo?(alam)
Alias: CVE-2017-7817
Whiteboard: [adv-main56+]
Flags: qe-verify+
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
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)
Group: core-security-release
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.