Closed Bug 1355888 Opened 3 years ago Closed 3 years ago

Prevent Marionette TCP server from starting/stopping on flipping marionette.enabled

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

It was decided that we have no real use case for making the Marionette TCP server start and stop when flipping the marionette.enabled preference in today’s Marionette meeting.
Group: mozilla-employee-confidential
Group: core-security-release
Blocks: 1355883
Group: mozilla-employee-confidential
Group: mozilla-employee-confidential
Group: core-security-release
Group: mozilla-employee-confidential
After careful reconsideration, I think we should remove marionette.enabled.  This might cause problems for Android since it might not be possible to pass the -marionette flag to start Marionette and it might depend on setting this preference to do that.
Assignee: nobody → ato
Status: NEW → ASSIGNED
It appears that Services.startup.quit(eAttemptQuit | eRestart) does not preserve command-line flags to Firefox.  This means that removing the marionette.enabled pref will prevent the Marionette server from starting up again after Firefox restarts.
A peak at ps(1) whilst running the test_in_app_restart and the preceding test:

Before Services.startup.quit(eRestart) is called:

> % ps aux | grep firefox
> ato      25084 89.5  0.7 1931064 256024 pts/0  Rl   18:22   0:01 /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -marionette -profile /tmp/tmptKMT37.mozrunner
> ato      25199  0.0  0.0  12784   968 pts/1    S+   18:22   0:00 grep firefox

Firefox process after restart has been called:

> % ps aux | grep firefox
> ato      25084  119  0.3 1556352 106856 pts/0  Rl   18:22   0:02 /home/ato/src/gecko/obj-x86_64-pc-linux-gnu/dist/bin/firefox
It appears that nsAppRunner works around this by setting environmental variables when Firefox needs to restart with a different profile.

http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4761:
>     if (rv != NS_SUCCESS_RESTART_APP_NOT_SAME_PROFILE) {
>       // Ensure that these environment variables are set:
>       SaveFileToEnvIfUnset("XRE_PROFILE_PATH", mProfD);
>       SaveFileToEnvIfUnset("XRE_PROFILE_LOCAL_PATH", mProfLD);
>       SaveWordToEnvIfUnset("XRE_PROFILE_NAME", mProfileName);
>     }

There is also a global gSavedVars[] that lets nsIAppStartup.quit restore various environment variables so that they are restored before re-launching the application.

http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2796:
> // To support application initiated restart via nsIAppStartup.quit, we
> // need to save various environment variables, and then restore them
> // before re-launching the application.
> 
> static struct SavedVar {
>   const char *name;
>   char *value;
> } gSavedVars[] = {
>   {"XUL_APP_FILE", nullptr}
> };

Apparently MOZ_APP_RESTART is still set and accessible to Marionette after the restart, so we could potentially set an output variable that, potentially in the combination with MOZ_APP_RESTART, would enable Marionette.
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review133786

::: testing/marionette/prefs/marionette.js
(Diff revision 2)
>  // It is included in Firefox, but not enabled by default unless the
>  // --marionette flag is passed or the marionette.enabled preference is
>  // set to true.
>  
> -// Whether or not Marionette is enabled.
> -pref("marionette.enabled", false);

Please mark the preference in `geckoinstance.py` as obsolete, but don't remove it yet.
Attachment #8857956 - Flags: review?(hskupin) → review+
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review133790

::: testing/marionette/client/marionette_driver/geckoinstance.py
(Diff revision 2)
>          "hangmonitor.timeout": 0,
>  
>          "javascript.options.showInConsole": True,
>  
> -        # Enable Marionette component
> -        "marionette.enabled": True,

I would have expected this in the first commit. Can you please combine both? Also please do not remove this preference yet due to backward compatibility with older Firefox releases. Just mark it as deprecated.
Attachment #8857957 - Flags: review?(hskupin) → review-
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review133792

Please note that with having this commit as the very last one, all the commits before in this commit series will cause bustage once landed, and cannot individually be regression tested.

::: testing/marionette/components/marionette.js:53
(Diff revision 1)
>  };
>  
> +// Complements -marionette flag for starting the Marionette server.
> +// We also set this if Marionette is running in order to start the server
> +// again after a Firefox restart.
> +const ENV_ENABLED = "MARIONETTE";

I would call this a workaround due to deficies of Firefox in not forwarding the -marionette argument to the new instance when Firefox gets restarted. 

I'm not sure how complicated a real fix is, so I suggest filing a new bug and referencing it here with a TODO comment.

::: testing/marionette/components/marionette.js:125
(Diff revision 1)
>      }
>    },
>  };
>  
>  function MarionetteComponent() {
> -  this.enabled = false;
> +  this.enabled = env.get(ENV_ENABLED) || false;

It's better to use `exists()` here because we are only interested that the env variable is set but not the value. That's what we do in various other places too.

::: testing/marionette/server.js:345
(Diff revision 1)
>      this.listener = new ServerSocket(this.port, flags, 1);
>      this.listener.asyncListen(this);
>  
>      this.alive = true;
>      this._acceptConnections = true;
> +    env.set(ENV_ENABLED, "1");

I think that we should set the environment variable inside the code block when we handle the command line argument.
Attachment #8858459 - Flags: review?(hskupin) → review-
Attachment #8857958 - Flags: review?(hskupin) → review?(james)
Comment on attachment 8857959 [details]
Bug 1355888 - Remove unused DEFAULT_PORT constant;

https://reviewboard.mozilla.org/r/129998/#review133796
Attachment #8857959 - Flags: review?(hskupin) → review+
Comment on attachment 8857960 [details]
Bug 1355888 - Clarify that -marionette flag works cross platform;

https://reviewboard.mozilla.org/r/130000/#review133798
Comment on attachment 8857960 [details]
Bug 1355888 - Clarify that -marionette flag works cross platform;

https://reviewboard.mozilla.org/r/130000/#review133800
Attachment #8857960 - Flags: review?(hskupin) → review+
(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> It appears that Services.startup.quit(eAttemptQuit | eRestart) does not
> preserve command-line flags to Firefox.  This means that removing the
> marionette.enabled pref will prevent the Marionette server from starting up
> again after Firefox restarts.

Yes, there were several bugs for that, whereby most of them got wontfixed. For example see bug 399317. Something similar biting us is bug 1299601, which is caused by not forwarding the -console argument with a restart.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review133790

> I would have expected this in the first commit. Can you please combine both? Also please do not remove this preference yet due to backward compatibility with older Firefox releases. Just mark it as deprecated.

We have been passing -marionette since forever, so this is safe to remove.

I want to make this a separate commit because it changes a different component.
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review133786

> Please mark the preference in `geckoinstance.py` as obsolete, but don't remove it yet.

marionette.enabled can safely be removed because mozrunner has been passing it since forever.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review133790

> We have been passing -marionette since forever, so this is safe to remove.
> 
> I want to make this a separate commit because it changes a different component.

Well, due to the requirement of a ENV variable it would cause breakage for this commit. I always love to see stand-alone and working commits, but maybe this time we can leave this alone.

At least please merge with the former commit.
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review133792

I can move it earlier in the changeset, to before we remove marionette.enabled.

> I would call this a workaround due to deficies of Firefox in not forwarding the -marionette argument to the new instance when Firefox gets restarted. 
> 
> I'm not sure how complicated a real fix is, so I suggest filing a new bug and referencing it here with a TODO comment.

As I point out in my earlier comments, this is how most state is preserved when Firefox restarts, see e.g. http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4761 and MOZ_APP_RESTART.  I don’t consider it a workaround, but in line with how Firefox deals with this problem elsewhere.

> It's better to use `exists()` here because we are only interested that the env variable is set but not the value. That's what we do in various other places too.

I can make that change.  It’s functionally equivalent, but perhaps it reads better:

>  function MarionetteComponent() {
> -  // guards against this component
> -  // being initialised multiple times
> +  this.enabled = env.exists(ENV_ENABLED);
>    this.running = false;

> I think that we should set the environment variable inside the code block when we handle the command line argument.

That defeats the purpose.  We only handle the flag in testing/marionette/components/marionette.js when we receive a flag.  When Services.startup.quit(eRestart) is called a flag isn’t passed, which is why we set MARIONETTE.

We also don’t want to set it unless we have actually started the server in case there are any issues with starting Marionette, which might leave Firefox in an unusable state.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review133790

> Well, due to the requirement of a ENV variable it would cause breakage for this commit. I always love to see stand-alone and working commits, but maybe this time we can leave this alone.
> 
> At least please merge with the former commit.

I have moved the commit adding an environment variable to the start of the changeset, which means tests relying on Services.startup.quit(eRestart) will keep working when the next commit removes the preference.

I don’t want to merge it with the previous commit removing marionette.enabled from the server component because this is a change to the client.  This is also why I don’t want the next commit, which removes it from wptrunner, to be part of it either.  In wptrunner’s case it’s also due to practical reasons that the commit needs to be uplifted to https://github.com/w3c/wptrunner.git.
(In reply to Henrik Skupin (:whimboo) from comment #22)
> (In reply to Andreas Tolfsen ‹:ato› from comment #7)
> > It appears that Services.startup.quit(eAttemptQuit | eRestart) does not
> > preserve command-line flags to Firefox.  This means that removing the
> > marionette.enabled pref will prevent the Marionette server from starting up
> > again after Firefox restarts.
> 
> Yes, there were several bugs for that, whereby most of them got wontfixed.
> For example see bug 399317. Something similar biting us is bug 1299601,
> which is caused by not forwarding the -console argument with a restart.

Thanks for the context.  The solution I picked is to set the MARIONETTE
environment variable, which is in line with MOZ_APP_RESTART, XORE_PROFILE_PATH,
XRE_PROFILE_LOCAL_PATH, XRE_PROFILE_NAME, et al.

I don’t think this should be considered a workaround solution for not
preserving arguments, which may be left out for other reasons.  MARIONETTE and
-marionette are analogous, so in any case I don’t see any harm in using it to
preserve state.  The remaining Marionette state we need is preserved as
preferences written to the profile.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review133790

> I have moved the commit adding an environment variable to the start of the changeset, which means tests relying on Services.startup.quit(eRestart) will keep working when the next commit removes the preference.
> 
> I don’t want to merge it with the previous commit removing marionette.enabled from the server component because this is a change to the client.  This is also why I don’t want the next commit, which removes it from wptrunner, to be part of it either.  In wptrunner’s case it’s also due to practical reasons that the commit needs to be uplifted to https://github.com/w3c/wptrunner.git.

A follow-up here: Because MARIONETTE is being introduced in this changeset, I was likely mistaken to claim that -marionette alone will solve all our problems: we would still have issues with any tests using Services.startup.quit(eRestart) since clients connecting to earlier releases without the environment variable patch would not know to start the Marionette server again after the restart.

I have therefore changed the patch to keep marionette.enabled in geckoinstance.py, but to mark it as deprecated like you suggested initially.
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review133786

> marionette.enabled can safely be removed because mozrunner has been passing it since forever.

I believe you are correct here and my previous statement is wrong.  See the comment I made in https://reviewboard.mozilla.org/r/129994/#review133790.

I have marked the use of marionette.enabled in geckoinstance.py as deprecated instead of removing it from the client.
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review133792

Thanks a lot for doing that!

> As I point out in my earlier comments, this is how most state is preserved when Firefox restarts, see e.g. http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4761 and MOZ_APP_RESTART.  I don’t consider it a workaround, but in line with how Firefox deals with this problem elsewhere.

Hm, what I only wonder is that there are only three profile related values which are saved, and nothing else. Looks like we get rid of everything else. What about the "-forground" argument we also use? Looks like this is also gone? 

Given that I'm not that confident with the AppRunner code, maybe you better ask Ted for review here?

> That defeats the purpose.  We only handle the flag in testing/marionette/components/marionette.js when we receive a flag.  When Services.startup.quit(eRestart) is called a flag isn’t passed, which is why we set MARIONETTE.
> 
> We also don’t want to set it unless we have actually started the server in case there are any issues with starting Marionette, which might leave Firefox in an unusable state.

If the environment variable is set outside of Firefox it will also be present all the time. Then it doesn't matter if Marionette starts correctly or not. Having a differnt handling for `-marionette` feels wrong to me.

But thinking more about all that with the security hat attached, we maybe should not even rely on a globally set env variable at all, given that it could also cause Marionette to be enabled even if the user doesn't want it - he/she may even not know that some other malicious code has set the variable. 

I would vote for removing the env variable in case the command line argument has not been given. That will save us from accidentally enabling Marionette after a restart.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review134240

::: testing/marionette/client/marionette_driver/geckoinstance.py:76
(Diff revision 4)
>          "hangmonitor.timeout": 0,
>  
>          "javascript.options.showInConsole": True,
>  
>          # Enable Marionette component
> +        # Deprecated and can be removed when Firefox 55 ships

Err, I don't think this version is correct unless you want to uplift this patch to esr52! I believe you will hit a couple of merge conflicts.

If no uplift to esr52 is planned, we can remove this preference after the next esr release which will be 60.
Attachment #8857957 - Flags: review?(hskupin) → review+
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review133792

> Hm, what I only wonder is that there are only three profile related values which are saved, and nothing else. Looks like we get rid of everything else. What about the "-forground" argument we also use? Looks like this is also gone? 
> 
> Given that I'm not that confident with the AppRunner code, maybe you better ask Ted for review here?

I can’t speak for -foreground or any of the other state that is being set as flags.

> Given that I'm not that confident with the AppRunner code, maybe you better ask Ted for review here?

I’m not requesting review of AppRunner code though.  This is about setting an environment variable to maintain one piece of Marionette state across a restart.

> If the environment variable is set outside of Firefox it will also be present all the time. Then it doesn't matter if Marionette starts correctly or not. Having a differnt handling for `-marionette` feels wrong to me.
> 
> But thinking more about all that with the security hat attached, we maybe should not even rely on a globally set env variable at all, given that it could also cause Marionette to be enabled even if the user doesn't want it - he/she may even not know that some other malicious code has set the variable. 
> 
> I would vote for removing the env variable in case the command line argument has not been given. That will save us from accidentally enabling Marionette after a restart.

If MARIONETTE is set in the shell, before Firefox starts, I want it to behave the same way that -marionette does.  They should be analogous.  Let’s not complicate this behaviour unnecessarily.

To the question of an environment variable being a security risk, what do you base that argument on?  The -marionette is also a security risk and functionally equivalent.  If an attacker has access to your system and your shell, you probably have bigger concerns than whether Firefox can be started with a flag.
Comment on attachment 8857957 [details]
Bug 1355888 - Mark marionette.enabled pref in client as a deprecated;

https://reviewboard.mozilla.org/r/129994/#review134240

> Err, I don't think this version is correct unless you want to uplift this patch to esr52! I believe you will hit a couple of merge conflicts.
> 
> If no uplift to esr52 is planned, we can remove this preference after the next esr release which will be 60.

Changed to 60.
ted and maja_zf: I want to request an informal security review of this code before we land it.  I believe that this changeset generally reduces the attack surface for Marionette by removing the marionette.enabled preference, but I would appreciate the feedback you might have.

Currently, flipping marionette.enabled in about:config at runtime lets you switch the Marionette TCP server on and off.  This is a nice feature, but we have no production use cases for it.

We also believe that by reversing the client/server relationship as ted suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=1240830 we can in the future solve a potential race conditions, but going down that path would implicitly render marionette.enabled useless.

It is also true in general that fewer options gives less room for programming mistake.  By removing the marionette.enabled preference, we leave the -marionette flag and the MARIONETTE environment variable as the two only ways of enabling Marionette.  I would also like to make them analogous for the same reasons, e.g. passing either or both would invoke the exact same behaviour.

The contentious part of the attached patches is that it introduces an environment variable that lets a potentially malicious attacker start the Marionette server.  Environment variables might be perceptually harder to spot than a flag, but my argument is that if the attacker has shell access to your system, you probably have bigger problems.

We recently had a meeting evaluating Marionette’s security story, and we came up with some concrete steps for action listed as dependents of https://bugzilla.mozilla.org/show_bug.cgi?id=1355883.  Removing marionette.enabled is one step on that plan.
Comment on attachment 8857958 [details]
Bug 1355888 - Remove marionette.enabled pref from wptrunner;

https://reviewboard.mozilla.org/r/129996/#review135248

Sure, assuming it works.
Attachment #8857958 - Flags: review?(james) → review+
Latest update is merely a rebase.
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review135870

::: testing/marionette/components/marionette.js:63
(Diff revision 6)
>  // This allows marionette prefs to persist when we do a restart into
>  // a different profile in order to test things like Firefox refresh.
>  // The environment variable itself, if present, is interpreted as a
>  // JSON structure, with the keys mapping to preference names in the
>  // "marionette." branch, and the values to the values of those prefs. So
> -// something like {"enabled": true} would result in the marionette.enabled
> +// something like {"port": 4444} would result in the marionette.enabled

The end of this sentence still refers to marionette.enabled.

::: testing/marionette/prefs/marionette.js:9
(Diff revision 6)
>  
>  // Marionette is the remote protocol that lets OOP programs communicate
>  // with, instrument, and control Gecko.
>  //
>  // It is included in Firefox, but not enabled by default unless the
>  // --marionette flag is passed or the marionette.enabled preference is

Remove comment about marionette.enabled.
Attachment #8857956 - Flags: review?(mjzffr)
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review135852

*puts on ill-fitting Security Hat* Can addons use nsIEnvironment? If so, using an environment variable is like using a pref, but less visible to the user. If we add some sort of visual feedback to indicate that Marionette is on, I think this is okay. 

Is there any way for a user to see what environment variables the browser is paying attention to? Would that be useful to add to about:support, say?

::: commit-message-ec9ed:11
(Diff revision 5)
> +
> +When the server is started, we set the environment variable to
> +preserve Marionette's enabled state across internal restarts.
> +When Services.startup.quit(eRestart) is called, Firefox is restarted
> +without the original command line flags it was started with, which means
> +we loose track of whether Marionette was enabled.  By setting MARIONETTE

sp: lose

::: testing/marionette/components/marionette.js:54
(Diff revision 5)
>  };
>  
> +// Complements -marionette flag for starting the Marionette server.
> +// We also set this if Marionette is running in order to start the server
> +// again after a Firefox restart.
> +const ENV_ENABLED = "MARIONETTE";

I suggest "MOZ_MARIONETTE_ENABLED" to be consistent with the other env var below and to make it easier to infer what it does (for someone who happens to be looking through their environment variables). No reason not to use a longer name like this, since it's really for internal use, whereas the -marionette flag is geared to Marionette users.
Attachment #8858459 - Flags: review?(mjzffr)
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review135852

> I suggest "MOZ_MARIONETTE_ENABLED" to be consistent with the other env var below and to make it easier to infer what it does (for someone who happens to be looking through their environment variables). No reason not to use a longer name like this, since it's really for internal use, whereas the -marionette flag is geared to Marionette users.

Good call. Any of the environment variables Firefox is using start with `MOZ_`. So also using this as the name of the env variable on the system would make sense, so not only for the variable in this code.
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review135852

> Good call. Any of the environment variables Firefox is using start with `MOZ_`. So also using this as the name of the env variable on the system would make sense, so not only for the variable in this code.

When we expose an environment variable that enables the server, it is no longer only for internal use: it will be analogous to passing -marionette to the binary.

There seems to be a precedence for naming things MOZ_*, so I have renamed it to MOZ_MARIONETTE.  This appears to be in line with how MOZ_LOG and MOZ_HEADLESS is named.
> *puts on ill-fitting Security Hat* Can addons use nsIEnvironment? If so, using an environment variable is like using a pref, but less visible to the user. 

I don't think there's any security benefit to this change, is there? There are many other bad things that are controlled with prefs e.g. [1] so we are presmably comfortable with the idea that anyone with enough access to change prefs already has enough access to do whatever malicious thing they like.

It is possible that the change is still reasonable for other reasons, however.

[1] http://searchfox.org/mozilla-central/source/js/xpconnect/src/xpcpublic.h#616
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review135870

> The end of this sentence still refers to marionette.enabled.

Sorry, I thought I had fixed this, but I think I did for an earlier revision I did of this patch.
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review135852

We should be careful to brand this changeset as an improvement to improve Marionette’s security model.  Removing marionette.enabled, which currently lets a user switch the TCP server on and off at runtime through about:config, was in hindsight an unnecessary change.

I expect addons can use nsIEnvironment because they run in chrome context and that they can induce the same behaviour by setting the output variable and restarting Firefox.  Today they can simply set the preference without the restart.  dburns has submitted https://github.com/mozilla/amo-validator/pull/527 to the AMO validator to check for future changes to any marionette.* pref.  I’m working on https://bugzilla.mozilla.org/show_bug.cgi?id=1355889 to add the visual UX cue you’re requesting.

> Is there any way for a user to see what environment variables the browser is paying attention to? Would that be useful to add to about:support, say?

I think that could be a good follow-up, but I’m not sure how we would keep an authoritative list of environment variables maintained.
Comment on attachment 8857956 [details]
Bug 1355888 - Remove marionette.enabled pref;

https://reviewboard.mozilla.org/r/129992/#review136270
Attachment #8857956 - Flags: review?(mjzffr) → review+
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review136272

The reasoning I'm hearing, roughly, is: we can achieve want we need with an environment variable and command-line flag, so we don't need a corresponding preference; we are adding security measures to mitigate the use of either an environment variable or a pref; the pref and environment variable carry similar risks, although in somewhat different forms.
Attachment #8858459 - Flags: review?(mjzffr) → review+
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review136272

> The reasoning I'm hearing, roughly, is: we can achieve want we need
> with an environment variable and command-line flag, so we don't
> need a corresponding preference; we are adding security measures to
> mitigate the use of either an environment variable or a pref; the pref
> and environment variable carry similar risks, although in somewhat
> different forms.

I think this is the best and most succinct summary so far.
Comment on attachment 8858459 [details]
Bug 1355888 - Add env var MOZ_MARIONETTE to start server;

https://reviewboard.mozilla.org/r/130424/#review136460
Attachment #8858459 - Flags: review?(hskupin) → review+
pauljt: This changeset makes changes to the way the Marionette remote control server is started, and it would be good to have an extra pair of eyes look at it from a security angle.  The changes are succinctly described in https://bugzilla.mozilla.org/show_bug.cgi?id=1355888#c95.

Could you please suggest who would be in a position to look at it?
Flags: needinfo?(ptheriault)
My team is pretty stretched at the moment, but Im not sure this needs too much review. I had a skim through the comments, and I dont think we are altering the risk profile too much (but then, take this with a grain of salt, I haven't looked at marionette since b2g days).

So as I understand it:
 - previously marionette (specifically the TCP server that listen for marionette commands) was stop/started with a preference
 - this patch changes the service to be started when Firefox begins, through the use of an environment variable or command line switch

My thoughts here are: 
 - As far as I can tell, there isn't really any difference here  from a threat perspective (not better or worse) : the ability to change a preference is already a powerful privilege (as pointed about, but also there are prefs which basically result in arbitrary shell commands)
 - So from an add-on perspective there is no difference: legacy add-ons have full privileges so can change prefs, set env variables, run shell commands, do things which require chrome privileges. (Note that web extension can do none of these things...)
- the only small risk that I can think of is that since an env variable is managed outside of Firefox there is a risk that the env variable gets set, and then forgotten about. So doing the followup  mentioned in 87 (or even something more visible) might be a good idea. And I guess try to use the command line switch where possible since that isn't persistent.
- The key risk we want to mitigate is a regular user accidentally enabling marionette (or being coerced into enabling this) I dont think that risk is materially different.
- I also assume there are no longer any use cases that require enabling and disable marionette (this was required in b2g days when you wanted to do development on your everyday phone, but I don't imagine that its needed for desktop/android)

Thats about all i can think of. Is that the sort of input you were looking for? If you need more I (or someone if I can find someone, we are pretty stretched) would need a refresher in the finer points of the marionette security model I think.
Flags: needinfo?(ptheriault)
Thanks for the quick turnaround on this, pauljt!  This was exactly the sort of input we were looking for.

I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1361712 as a follow-up to list environment variables in about:support, as maja_zf suggested.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abefb6ff0f01
Add env var MOZ_MARIONETTE to start server; r=maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/c06f9475eab4
Remove marionette.enabled pref; r=maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/469e5e17e31c
Mark marionette.enabled pref in client as a deprecated; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/3bfb71d74072
Remove marionette.enabled pref from wptrunner; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/e84633de5fa0
Remove unused DEFAULT_PORT constant; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/cd8a44efa422
Clarify that -marionette flag works cross platform; r=whimboo
You need to log in before you can comment on or make changes to this bug.