Closed Bug 1367089 Opened 7 years ago Closed 7 years ago

Disable automation cue UI for mozscreenshots

Categories

(Testing :: mozscreenshots, enhancement)

Product:
Testing
Component:
mozscreenshots
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

Details

Attachments

(1 file)

Bug 1367089 - Remove automation clue window attribute in mozscreenshots.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Mozscreenshots (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots) is a tool that regularly takes various screenshots of mozilla-central and sends them to a mailing list for triage to ensure we didn't regress anything.

It's important that the screenshots reflect the actual Firefox UI and the new automation clue is hiding or changing important parts of the UI, i.e. the identity block and the URL bar.

It seems there's no pref to turn this off. Please advise us how to disable this for mozscreenshots.
Flags: needinfo?(ato)
Seems the simplest would be to set an additional attribute on the main window from mozscreenshots, and update the CSS rule that adds this style to not apply when the attribute is present.
There’s no pref to turn this off because we don’t want it to be trivial for a potential malicious attacker to disguise a Firefox where the remote protocol is active as a regular browsing session.

The intention of the visual UX cue is for example to alert users that it might not be a good idea to enter your banking password in a Firefox window that is being remotely controlled.

Because we as a browser vendor have special requirements for testing the browser, I would suggest instrumenting it to remove the "remotecontrol" attribute on ChromeWindow (<window id="main-window">) before taking the screenshot.  Removing this attribute will cause the cue to go away.

If you are using marionette-driver directly (untested code):

> with self.marionette.using_context("chrome"):
>     main_window = self.marionette.find_element(By.CSS_SELECTOR, "window#main-window")
>     self.marionette.execute_script("""
>         const [mainwin] = arguments;
>         mainwin.removeAttribute("remotecontrol");
>         """, (main_window,))
(In reply to :Gijs from comment #1)
> Seems the simplest would be to set an additional attribute on the main
> window from mozscreenshots, and update the CSS rule that adds this style to
> not apply when the attribute is present.

Yes, this is also a good suggestion.
Flags: needinfo?(ato)
So why do we not only show the UX cue when Marionette has actually an active session? If nothing is using Marionette, why do we need it?
(In reply to Henrik Skupin (:whimboo) from comment #4)
> So why do we not only show the UX cue when Marionette has actually an active
> session? If nothing is using Marionette, why do we need it?

Mozscreenshots uses marionette.
For which purposes? The docs don't list anything regarding Marionette.
(In reply to Henrik Skupin (:whimboo) from comment #6)
> For which purposes? The docs don't list anything regarding Marionette.

Mochitests run via marionette, don't they? Mozscreenshots is just a type of mochitest. The same thing (marionette-style UI cues) happen when you run other mochitest-browser tests.
(In reply to :Gijs from comment #7)
> (In reply to Henrik Skupin (:whimboo) from comment #6)
> > For which purposes? The docs don't list anything regarding Marionette.
> 
> Mochitests run via marionette, don't they? Mozscreenshots is just a type of
> mochitest. The same thing (marionette-style UI cues) happen when you run
> other mochitest-browser tests.

Yes, mochitest uses Marionette to bootstrap the unsigned automation addons
that would otherwise get blocked.
After installing the addons Marionette is no longer used, and as such the Marionette session can be disconnected. Which means Marionette would be active in terms that the socket is allowing connections, but no commands are getting executed.

Maybe that is an option...
(In reply to Henrik Skupin (:whimboo) from comment #9)
> After installing the addons Marionette is no longer used, and as such the
> Marionette session can be disconnected. Which means Marionette would be
> active in terms that the socket is allowing connections, but no commands are
> getting executed.

This is true, but disconnecting the client does not mean that the remote protocol isn’t running.  Following the security changes we have been recently, it is only when the Firefox binary is shut down that the remote control protocol gets deactivated.  Or in other words, the remote protocol is active for the duration of the Firefox process’ lifetime.
These are good suggestions, thanks to all of you! I'll try to go with the simplest route of just removing the remotecontrol attribute from the window for now.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
(In reply to Andreas Tolfsen ‹:ato› from comment #2)
> There’s no pref to turn this off because we don’t want it to be trivial for
> a potential malicious attacker to disguise a Firefox where the remote
> protocol is active as a regular browsing session.

I'm not sure I understand this. If an attacker can modify your hidden prefs, you have much more severe issues to worry about than them disabling this UI hint.
This patch is intended to be a short-term fix. I know this only works for the single active browser window but afaik we don't spawn new windows in mozscreenshots atm.

I would like to make a follow-up bug to disable this UI in Mochitests entirely or alternatively add a pref to disable this (since I can follow Dao's argument that if an attacker can set prefs you have lost the game).
(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > There’s no pref to turn this off because we don’t want it to be trivial for
> > a potential malicious attacker to disguise a Firefox where the remote
> > protocol is active as a regular browsing session.
> 
> I'm not sure I understand this. If an attacker can modify your hidden prefs,
> you have much more severe issues to worry about than them disabling this UI
> hint.

Such as?

It's become a lot harder to run untrusted code (ie unsigned add-ons) in release copies of Firefox, and we will continue to lock that down further. Changing a pref just requires user write access to the profile dir - changing running Firefox code and security UI (which this effectively is) should ideally require admin access to the machine.

We've removed a bunch of dangerous prefs already (e.g. the "let viruses take over your computer" pref now only works in very restricted circumstances) and moved other data out of prefs entirely (e.g. search engine preference) in order to help prevent tampering by outside actors.

TBH, I would prefer it if the hint lived in widget code and couldn't be disabled by anything/anyone except in the same situations as the aforementioned remote XUL/XBL stuff, but that goal is not achievable short-term. Still, I don't see why we should make things *easier* for malicious actors.
(In reply to :Gijs from comment #15)
> (In reply to Dão Gottwald [::dao] from comment #12)
> > (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > > There’s no pref to turn this off because we don’t want it to be trivial for
> > > a potential malicious attacker to disguise a Firefox where the remote
> > > protocol is active as a regular browsing session.
> > 
> > I'm not sure I understand this. If an attacker can modify your hidden prefs,
> > you have much more severe issues to worry about than them disabling this UI
> > hint.
> 
> Such as?

Most things under about:preferences#privacy and about:preferences#advanced for instance?
(In reply to Dão Gottwald [::dao] from comment #16)
> (In reply to :Gijs from comment #15)
> > (In reply to Dão Gottwald [::dao] from comment #12)
> > > (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > > > There’s no pref to turn this off because we don’t want it to be trivial for
> > > > a potential malicious attacker to disguise a Firefox where the remote
> > > > protocol is active as a regular browsing session.
> > > 
> > > I'm not sure I understand this. If an attacker can modify your hidden prefs,
> > > you have much more severe issues to worry about than them disabling this UI
> > > hint.
> > 
> > Such as?
> 
> Most things under about:preferences#privacy and about:preferences#advanced
> for instance?

Whether those are much more severe depends on the threat model, but I would not say no to trying harder to avoid having some of those settings changed by other apps / actors. Feel free to file bugs on the ones you think are the most serious (probably the proxy settings?).
(In reply to :Gijs from comment #15)
> Changing a pref just requires user write access to the profile dir -
> changing running Firefox code and security UI (which this effectively is)
> should ideally require admin access to the machine.

What about userChrome.css? Doesn't that only require file access, too?

Independent of that, I don't see the security threat of adding that pref.

Ato says this in bug 1355890 comment 24:

> It is worth noting that for the attacker to be able to connect to the Marionette session, she must have shell access since it is not possible to remotely connect since the server only accepts connections from loopback devices.

You both repeatedly say in that bug that this security measure is already trivial to circumvent for a skilled attacker:

> We are providing legit users reassurance which windows on their screen are under automation, to lessen the chance they use the wrong window for entering potentially private information.
> We are also adding one more hurdle for a malicious attacker with shell access to the user’s account to jump through for disguising an automation window as a real window.  This is thought to add to our layered security, but it is correctly possible, if not entirely trivial, to bypass.

Personally my favorite solution would be to turn this off by default in all Mochitests, pref or not. It's irritating when testing UI and I'm surprised this hasn't failed any identity UI tests.
(In reply to Johann Hofmann [:johannh] from comment #18)
> (In reply to :Gijs from comment #15)
> > Changing a pref just requires user write access to the profile dir -
> > changing running Firefox code and security UI (which this effectively is)
> > should ideally require admin access to the machine.
> 
> What about userChrome.css? Doesn't that only require file access, too?

Yep. Are we not getting rid of that with 57?

> Independent of that, I don't see the security threat of adding that pref.

If you don't think there's a security risk in the user not realizing their browser is under the control of some remote connection, I suggest taking it up with the relevant spec group for marionette. Safari has already implemented this as well, AIUI.

Once you accept that, then it's logical that once you have measures to make remote control obvious to the user, attackers being able to circumvent those measures is a security risk.

> You both repeatedly say in that bug that this security measure is already
> trivial to circumvent for a skilled attacker

And I would love for it to be less trivial. If you have time to submit patches to do that, that would be awesome. But "this is not great security" is not a good argument for "let's just not have it".

If we implement a pref, we'll just have to then also rescind/restrict the pref when we do eventually tighten this up further.
(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > There’s no pref to turn this off because we don’t want it to be trivial for
> > a potential malicious attacker to disguise a Firefox where the remote
> > protocol is active as a regular browsing session.
> 
> I'm not sure I understand this. If an attacker can modify your hidden prefs,
> you have much more severe issues to worry about than them disabling this UI
> hint.

Yes.  We’re not marketing this as a security feature at all.  It is a feature that is helpful to users to distinguish sessions under automation from regular browsing sessions.
Comment on attachment 8870799 [details]
Bug 1367089 - Remove automation clue window attribute in mozscreenshots.

https://reviewboard.mozilla.org/r/142362/#review148532

I guess this is fine for now… The PrivateBrowsing[1] configuration that I didn't port to m-c yet does open a new private window but I think you're right that the ones in-tree don't currently open windows.
[1] https://github.com/mnoorenberghe/mozscreenshots/blob/master/mozscreenshots/extension/configurations/PrivateBrowsing.jsm
Attachment #8870799 - Flags: review?(MattN+bmo) → review+
hg error in cmd: hg pull gecko -r 31be6a380395646db66e2a13b96f9065fe3f2729: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/898a81f1f62b
Remove automation clue window attribute in mozscreenshots. r=MattN
https://hg.mozilla.org/mozilla-central/rev/898a81f1f62b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Component: General → mozscreenshots
Product: Firefox → Testing
Target Milestone: Firefox 55 → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: