1732622 - Add opt-in support for hosts and origins other then local loopback interfaces

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

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

As reported this is a regression in geckodriver 0.30.0 as caused by bug 1652612. Since it has been landed we only allow connections from a local loopback device. But there are scenarios where folks setup different docker images, and as such need a way to whitelist specific network addresses.

More details can be found at https://github.com/mozilla/geckodriver/issues/1935.

James, can you please take this up?

Comment 1

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

There has also https://github.com/mozilla/geckodriver/issues/1936 been filed, which covers the Ipv6 part. ::1 should actually be allowed.

Comment 2

[Sebastian Tschan]

Thanks for creating this issue ticket Henrik.

Could we consider providing a configuration that allows connections from all network hosts (with all Host headers) while still rejecting requests with Origin headers other than localhost or those explicitly allowed?

From my understanding, this would protect from CSRF attacks (since these include an Origin header other than localhost), while making the typical remote client setup feasible (where clients send requests from a non-web context, which does not include an Origin header).

Comment 3

[Gabriel Corona]

For the Host header, could we:

• allow all literal IP addresses and "localhost" by default;
• add an CLI option to enable extra values of the Host header.

Comment 4

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

While work is happening to get this bug fixed I'm going to add an entry to the release notes to mention this particular regression. As James suggested we will phrase it like: geckodriver restricts connections to local ip addresses. This can interfere with deployments in which geckodriver is running on a different network node to the tests e.g. some container or virtual-machine based setups.

Comment 5

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

Attachment: Bug 1732622 - [geckodriver] Update release notes for host check regression.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8c148e11dce
[geckodriver] Update release notes for host check regression. r=webdriver-reviewers,jgraham

Comment 7

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

I've also updated the upstream release notes and change.md file:
https://github.com/mozilla/geckodriver/releases/tag/v0.30.0

Adding the leave-open keyword given that we are still waiting for the fix.

Comment 8

[Cosmin Sabou [:CosminS]]

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

Comment 9

[James Graham [:jgraham]]

Attachment: Bug 1732622 - Enable allowing given hosts and origins for geckodriver,

The policy is now as follows:

• The --host command line flag can now be an actual hostname as well
  as in IP address.

• By default only requests with a Host header that is an IP address,
  or matching the value of the --host argument are accepted.

• If --host is a local IP address, we by default accept localhost
  in the Host header.

• When --allow-hosts is provided, only requests with a hostname
  matching the listed values, or an IP address, are accepted.

• By default any request with an Origin header is rejected.

• When --allow-origins is provided, only requests with no Origin
  header, or an origin matching the given values, are accepted.

Comment 10

[James Graham [:jgraham]]

I've put up the patch as a kind of proposal; this isn't a definite thing. Hopefully the commit message in the comment above explains the proposed policy well. Feedback welcome on a) whether this enforces the necessary properties for safety and b) whether this addresses the use cases.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 12

[Sebastian Tschan]

Thanks for the proposed solution James.
I think it covers a wide variety of use cases, but not the one I outlined in my comment above:

Could we consider providing a configuration that allows connections from all network hosts (with all Host headers) while still rejecting requests with Origin headers other than localhost or those explicitly allowed?

What I'm asking for is basically allowing the previously possible usage of --host 0.0.0.0 (as well as the IPv6 equivalent), while still rejecting requests with Origin headers (unless explicitly allowed).

A sample use case is the provision of this Docker image:
https://github.com/blueimp/geckodriver/blob/165c41f8640540729b377dd7c5052de05bd67e4f/Dockerfile#L41
By setting --host 0.0.0.0 in the image definition, users of the image can provision it without having to provide command-line arguments (ease of setup).

I think it would be beneficial to have a discussion and mutual understanding of potential attack vectors if containers with geckodriver are provisioned that way, see also my comment above:

From my understanding, this would protect from CSRF attacks (since these include an Origin header other than localhost), while making the typical remote client setup feasible (where clients send requests from a non-web context, which does not include an Origin header).

I would really like to understand what security aspect we're accomplishing by limiting the host header, beyond what restricting Origin headers already provides.

Comment 13

[James Graham [:jgraham]]

What I'm asking for is basically allowing the previously possible usage of --host 0.0.0.0 (as well as the IPv6 equivalent), while still rejecting requests with Origin headers (unless explicitly allowed).

In the current design that's possible if either a) requests happen by ip address (although that might be a bad idea, see below), or b) you add the hostname that will be used to connect to geckodriver to the --allow-hosts list. I'd assume that it must be possible to know the hostname of the container running geckodriver in a docker-compose (or similar) setup, so although it's some effort to pass in the hostname explictly it shouldn't actually be impossible.

I've heard two concerns with relying on the Origin header exclusively:

• A DNS rebinding attack might allow a website to perform a "same origin" request that actually ends up on a local ip. That could at least allow malicious sites to make GET requests without an Origin header.
• Old clients that might not correctly set the Origin header and so leave users vunerable (this is the one that still seems problematic if we allow any ip address).

Freddy might have more insight into how concerned we should be about these kind of attack, but in general I'm wary of a mode to disable all Host validation becoming an attractive nusicance in the sense that it makes life easier for people like yourself who are building software that uses geckodriver as a dependency, and so is widely used, but with the actual end users being unaware that they're running in a mode which turns off some of the security checks.

Comment 14

[Sebastian Tschan]

Thanks for your detailed reply James.

The DNS rebinding attack is a valid concern that I didn't consider before.
I am not sure it is a strong concern, since aside from the GET /status endpoint, all other GET endpoints (https://www.w3.org/TR/webdriver/#endpoints) require the session ID, which I think an attacker won't have access to.
(And POST endpoints would still send the Origin header for same-host requests.)

I'm also not sure if old clients are a strong concern, since I think it's unlikely an old browser client is used in a context where we're running a geckodriver version that requires a modern version of Firefox.

But yeah, the host check does add another layer of security with very little in terms of configuration overhead.
And it's definitely possible to support the Docker container use case with e.g. --allow-host=geckodriver or --host=geckodriver in a setup where geckodriver is the Docker hostname of the container running it.

Comment 15

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] (away 10/23 - 10/31) from comment #1)

There has also https://github.com/mozilla/geckodriver/issues/1936 been filed, which covers the Ipv6 part. ::1 should actually be allowed.

James, before I'm back and being able to review maybe you can make sure that this scenario works as well as the following issue gets fixed:
https://github.com/mozilla/geckodriver/issues/1947

Thanks

Comment 16

[James Graham [:jgraham]]

I don't think it makes sense to add additional information to the error response with details of the server configuration; that could leak some information to an attacker. But I've added warn! messages to the geckodriver logging with more detail about the rejected requests.

Comment 17

[James Graham [:jgraham]]

Attachment: Bug 1732622 - Enable passing webdriver binary and args to pytest,

This allows us to write some gecko-specific tests for command line argument
handling.

In addition use a file and single environment variable for all the data
we're passing into pytest. Having two separate mechanisms doesn't make
much sense.

Comment 18

[James Graham [:jgraham]]

Attachment: Bug 1732622 - Fix clippy errors,

Using cargo clippy --fix

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46f1fb7f6fc0
Enable passing webdriver binary and args to pytest, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/577a5b37f781
Enable allowing given hosts and origins for geckodriver, r=webdriver-reviewers,freddyb,whimboo
https://hg.mozilla.org/integration/autoland/rev/ab9d1e14386a
Fix clippy errors, r=webdriver-reviewers,whimboo

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32412 for changes under testing/web-platform/tests

Comment 21

[Marian-Vasile Laza]

Backed out 3 changesets (Bug 1732622) for causing build bustages on server.rs.
Backout link
Push with failures
Failure Log

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3f88a74d97c
Enable passing webdriver binary and args to pytest, r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/854151593a85
Enable allowing given hosts and origins for geckodriver, r=webdriver-reviewers,freddyb,whimboo
https://hg.mozilla.org/integration/autoland/rev/8b355e29a5ef
Fix clippy errors, r=webdriver-reviewers,whimboo

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/a3f88a74d97c
https://hg.mozilla.org/mozilla-central/rev/854151593a85
https://hg.mozilla.org/mozilla-central/rev/8b355e29a5ef

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot