743904 - Security review for Element.mozRequestFullscreenWithKeys

Status: VERIFIED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

* Who is/are the point of contact(s) for this review?
Chris Pearce

* Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):
Allows key input to be used by sites that enter fullscreen mode using the DOM API. Useful for fullscreen games. See attachment 613472 [details] for a video of my proposed UI. See bug 716107.

* Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:
See attachment 613472 [details] for a video of my proposed UI. See bug 716107.

* Does this request block another bug? If so, please indicate the bug number
bug 716107 ?

* This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
As soon as possible.

* Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
Adds API to HTMLElement. So code changes to Firefox and anything that uses Gecko.

* Are there any portions of the project that interact with 3rd party services?
No.

* Will your application/service collect user data?
No.

* If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
N/A.

* Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite. 
As soon as possible. I'm in New Zealand, so the review needs to be scheduled at or after 10am New Zealand Standard Time, on a Tuesday to Friday NZST (e.g. Monday to Thursday California time). I need at least 24 hours notice. Invite me and whoever you want to bring. Vidyo is preferred so I can see when you roll your eyes.

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:dveditz will lead this one, get required reading, etc

Comment 2

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Review sched for 23-april

https://mail.mozilla.com/home/[e-mail]/Security%20Review.html?view=month&action=view&invId=164907-164906&pstat=AC&instStartTime=1335218400000&instDuration=3600000

:dveditz
Add assigned reading to the etherpad here: 
https://etherpad.mozilla.org/requiredreading

Comment 3

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Item to be reiviewed: Element.mozRequestFullscreenWithKeys
Link to calendar entry: https://mail.mozilla.com/home/[e-mail]/Security%20Review.html?view=month&action=view&invId=164907-164906&pstat=AC&instStartTime=1335218400000&instDuration=3600000
SecReview Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=743904
Security Lead: dveditz
Required Reading List:
* Screencast of current proposal, which enables keys in fullscreen mode, but requires explicit approval of all fullscreen requests the first time a domain enters fullscreen:
https://bugzilla.mozilla.org/attachment.cgi?id=616431
* Screencast of previous proposed UI, which has separate fullscreen with and without keys modes:
https://bugzilla.mozilla.org/attachment.cgi?id=613472
* Impact of fullscreen approval UI on pointer-lock API:
https://bugzilla.mozilla.org/show_bug.cgi?id=746885
* Previous security review:
https://wiki.mozilla.org/Security/Reviews/Firefox10/CodeEditor/FullScreenAPI
(If possible prefill this area for copying to the notes section of the review)
Introduce Feature (5-10 minutes) [can be answered ahead of time to save meeting time]
- Goal of Feature, what is trying to be achieved (problem solved, use cases, etc)

    Enable keys to be used in fullscreen mode, for games and other immersive experiences. 

    Video also needs fullscreen, but only cares about the video controls' keys.

    Any solution has to take into consideration how fullscreen approval UI interacts with pointer lock.

- What solutions/approaches were considered other than the proposed solution?

    Having separate Element.requestFullscreenWithKeys() method, but I think this:

    Would lead to confusion by users as to why they have to grant permission for fullscreen in some cases (with keyboard input) but not others.

    I doubt developers will care about the difference and just use requestFullscreenWithKeys() most of the time anyway. The bad guys definitely won't care.

    This doesn't match the current draft spec, which only has requestFullscreen(), and doesn't mention key input at all.

    This won't match Chrome's behaviour (grant key access regardless whether it's requested or not).

    What should we do on mobile when this is called?

    We could have the fullscreen API ask permission (rather than forgiveness) before entering fullscreen the first time, and automatically grant subsequent requests. Similar to how geolocation works. This has the disadvantage that there may not be an explicit "deny" action from the user to notify script that the fullscreen request failed; script could be left waiting for a "fullscreendenied" event that never comes.

    Also considering blocking mouse events to page while showing the approval UI (and we should grey out the page if we do this I think), as we're thinking of doing this so if script requests pointer-lock when we enter fullscreen (but before it's been approved) we can pretend to be pointer-locked but still show the mouse in order to use the approval UI.

    May instead just delay granting pointer lock until fullscreen is approved, still haven't reached consensus with Olli Petay on this.

- Why was this solution chosen?

    Balances convenience with awareness and security.

    I would also like to remove the "dim <browser> for first 2 seconds when entering fullscreen". Seems to be unneccessary when we don't auto-hide the approval UI.

- Any security threats already considered in the design and why?

    Spoofing attacks; we show the domain name in the fullscreen approval UI, so user is aware they're in fullscreen mode, and what website is fullscreen. Appoval UI doesn't auto-hide, so user can't fail to notice they're in fullscreen.

    Web page can put up lots of boxes styled similarly to the "you've entered fullscreen" approval UI, in order to overwhelm the user so they don't read the UI warning and don't realise what's going on. Since we don't dismiss the UI unless the user clicks on it, the user shouldn't 

* Threat Brainstorming (30-40 minutes)

    Cross domain subdocs? Throw up the approval UI again? Are they actually a threat?

* Conclusions / Action Items (10-20 minutes)