1359472 - Let geckodriver forward a "--allow-system-access" argument to Firefox to prevent parent process access by default

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P2)

Description

[James Graham [:jgraham]]

In general WebDriver implementations involve giving clients some privileged access to the browser. Therefore when enabling it it is assumed that users are happy with the security implications. However giving chrome access does provide additional capabilities compared to other drivers (more system access other than reading files, access to change the firefox UI, etc.).

It therefore might make sense to make this a separate opt-in feature. I think that an environment variable like `MOZ_MARIONETTE_CHROME` makes sense since geckodriver users can't set environment variables through capabilities.

Comment 1

[David Burns :automatedtester]

Why not have it as a capability? If the capability is missing or is `false` then block all Chrome type calls? 

We can extend this so that in GeckoDriver we block all /moz/* unless the that capability is there.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that we would also have to restrict execute(_async)_script() for the system sandbox.

Comment 3

[James Graham [:jgraham]]

If it's a capability then you are in exactly the same situation as today; enabling marionette gives full chrome access to anyone who can start a session. The point of the environment variable suggestion is exactly that that isn't possible to set from within the protocol itself (whereas command line flags to gecko are possible to set).

Comment 4

[James Graham [:jgraham]]

Since it wasn't really clear, some of the motivating scenarios for this change:

* We add UI to tell a user that a webdriver session is running. In the case that this session is somehow malicious, the first thing it does is executes Chrome script to disable that UI.

* A vulnerability exists in a VM environment that allows an attacker with the ability to execute code to access the host operating system. Geckodriver on Saucelabs or Browserstack provides a trivial means of exploiting this bug without also having a browser exploit.

Now I don't claim these are critical issues; in the first situation you probably lost already, and in the second you probably will lose eventually. But it does provide a little defence in depth and allows people to reason about the security implications of WebDriver in general without having to consider that Geckodriver/Marionette gives significantly more access by default.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to James Graham [:jgraham] from comment #0)

It therefore might make sense to make this a separate opt-in feature. I
think that an environment variable like MOZ_MARIONETTE_CHROME makes sense
since geckodriver users can't set environment variables through capabilities.

Maybe something has changed since then but environment variables can be set via capabilities:
https://developer.mozilla.org/en-US/docs/Web/WebDriver/Capabilities/firefoxOptions#Env

But these should only affect Firefox but not geckodriver itself, and as such geckodriver can disallow switching into chrome scope. So maybe this might be enough for now?

Comment 6

[James Graham [:jgraham]]

Yeah, I think that an enironment variable that applies to geckodriver is enough.

Comment 7

[James Graham [:jgraham]]

Attachment: Bug 1359472 - Disable switching contexts without command line flag,

Usually WebDriver users should not need to run code in Chrome scope. For the few cases
where they do, allow passing a command line flag to GeckoDriver to enable the context
related endpoints.

The approach here just puts a conditional in when sending the message to WebDriver;
if we did more things like this it would be better to have a mechanism to avoid
registering the routes at all.

Comment 8

[James Graham [:jgraham]]

My patch is a partial solution here in that it works at the geckodriver level to forbid access to these routes. We could still consider making marionette itself require some setup to allow this kind of access, but this at least helps ensure that it's harder for WebDriver users to escape the sandbox.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

James, are you going to continue work on this patch? If not can you please unassign?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

James, with bug 1708689 in place for a remote attack I'm worried about a simple chrome context switch by just using Marionette. There is still a vector for attack that we should remove at the basis, and only allow switching to the chrome context when a special command line argument (like --marionette-enable-chrome) or environment variable (MOZ_MARIONETTE_CHROME) is set for Marionette when starting Firefox.

Using a command line argument for Firefox would make it easier for geckodriver to forward its own command line argument.

Comment 11

[James Graham [:jgraham]]

I think the patch here is in a state where we could just land it. I agree that a followup to require specifying a command line, pref, or environment variable, on the marionette side would be a good idea.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This patch will actually only make changes to geckodriver but not Marionette. As such I will file a follow-up for the remaining work.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As discussed in Monday's triage meeting I'll have to check how this change will affect tests for WebExtensions, and how these get run in some CI. I'll try to find some time over the next days.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We should first settle on a solution for Marionette before we can actually patch geckodriver to restrict chrome context support.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #14)

We should first settle on a solution for Marionette before we can actually patch geckodriver to restrict chrome context support.

There is actually a problem with this proposal. If Marionette restricts access first all consumers of geckodriver, that require these extra permissions, will not be able to run their tests anymore with older versions of Firefox. I would strongly vote for not doing that. Instead we should follow the proposal from James on that patch and ship the geckodriver changes first. At a later point when Marionette fully supports handling the same flag for Firefox (including some previous releases) geckodriver can be updated again to just pass-through the commands and to not handle them on its own.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We agreed on to use a --enable-system-access command line argument for geckodriver which will just forward it to Firefox as --remote-enable-system-access if set. Even through the argument is not yet present for Firefox it will not cause failures when starting Firefox but just a log entry mentioning it as unrecognized argument. Marionette will then be updated on the follow-up bug in February or March as well.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1359472 - [geckodriver] Add "--allow-system-access" argument to allow testing in "chrome" context.

Adds a command line argument that gets forwarded to Firefox
and will control in future versions of Firefox when access
to the browser's parent process for eg. browser ui testing
is allowed.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Freddy, while we originally filed this as a security bug I wonder how much it still stands. We allow this feature for quite some time and now with the reviewed patch (to get landed) we will do the first step in restricting the access to the parent process by default. In geckodriver we are just forwarding the added argument to Firefox now, which will soon get this argument as well - and will only allow access to the parent process if the argument is specified when starting Firefox.

Can I just go ahead and land the patch or do we need a specific security review? Maybe we should rather do that for bug 1710425 and bug 1944565?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, I just stumbled over the following document when I tried to open lando for this patch:
https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html#on-requesting-sec-approval

Given that we have sec-want I can just push the changes.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12d7b2544b10
[geckodriver] Add "--allow-system-access" argument to allow testing in "chrome" context. r=webdriver-reviewers,Sasha,jgraham

Comment 21

[amarc]

Backed out for causing btime failures

• Backout link
• Push with failures
• Failure Log
• Failure line:

raptor-main Critical: TEST-UNEXPECTED-FAIL: no raptor test results were found for constant-regression

Comment 22

[Frederik Braun [:freddy]]

Yes, fine to land. Is this something that impacts production / end-user builds of Firefox or is this solely for users of geckodriver? The latter is fine. The former should really get a security review :)

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The problem was that I missed to update a line when getting the command line flag and it was referencing still the old name of the flag. I was able to reproduce it with starting geckodriver without any arguments:

✗ geckodriver                                                                                              git:(geckodriver-system-access↑1|…3
thread 'main' panicked at testing/geckodriver/src/main.rs:259:29:
Mismatch between definition and access of `enable_system_access`. Unknown argument or group id.  Make sure you are using the argument id and not the short or long flags

I'll push a fix.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Frederik Braun [:freddy] from comment #22)

Yes, fine to land. Is this something that impacts production / end-user builds of Firefox or is this solely for users of geckodriver? The latter is fine. The former should really get a security review :)

This change is for geckodriver only. I'll get back to you over on the other bug (where I've already needinfo'ed you).

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afa1b448c3dc
[geckodriver] Add "--allow-system-access" argument to allow testing in "chrome" context. r=webdriver-reviewers,Sasha,jgraham

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 27

[Daniel Veditz [:dveditz]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #18)

Freddy, while we originally filed this as a security bug ...

You didn't, actually! It's filed as "employee confidential" which is given the description Confidential Mozilla Employee Bug (non-security); It is explicitly labeled "not a security bug".

The sec-want tag means my team wants to track it as a security enhancement but it's not a vulnerability. Whether those bugs are hidden or not depends on what the enhancement might imply about other existing vulnerabilities.

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #19)

Oh, I just stumbled over the following document when I tried to open lando for this patch:
https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html#on-requesting-sec-approval

Did lando point you there for this bug? We might need to fix that. That instruction applies to "Security" bugs, not "Confidential" bugs. At the top of this bug you can see a violet banner with the word "CONFIDENTIAL" in it. On a security bug you'll see an orange banner, with the words "SECURITY ISSUE - APPROVAL PROCESS" where "approval process" is a link to that document.

Comment 28

[Daniel Veditz [:dveditz]]

If there are other geckodriver/remote protocol security bugs that are currently "confidential" instead of in a security group we'd appreciate it if you could change those. If you run into permission problems doing that I will be happy to help.

("needinfo'd" so this request doesn't get lost in bugspam, but not actually a question that needs a response in this bug)

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Daniel Veditz [:dveditz] from comment #27)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #19)

Oh, I just stumbled over the following document when I tried to open lando for this patch:
https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html#on-requesting-sec-approval

Did lando point you there for this bug? We might need to fix that. That instruction applies to "Security" bugs, not "Confidential" bugs. At the top of this bug you can see a violet banner with the word "CONFIDENTIAL" in it. On a security bug you'll see an orange banner, with the words "SECURITY ISSUE - APPROVAL PROCESS" where "approval process" is a link to that document.

Good question which I cannot answer anymore now. But I'll keep it in mind when attaching a revision to Phabricator for another confidential bug. If that's the case I can certainly file a bug. Thanks Dan!