Closed Bug 1732622 Opened 8 months ago Closed 4 months ago

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

Categories

(Testing :: geckodriver, defect, P3)

Default
defect

Tracking

(firefox-esr91 unaffected, firefox93 wontfix, firefox94 wontfix, firefox95 wontfix, firefox96 wontfix, firefox97 wontfix, firefox98 fixed)

RESOLVED FIXED
98 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox98 --- fixed

People

(Reporter: whimboo, Assigned: jgraham)

References

()

Details

(Keywords: regression)

Attachments

(4 files)

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?

Flags: needinfo?(james)

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

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).

Assignee: nobody → james
Whiteboard: [webdriver:triage]

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.

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.

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

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.

Status: NEW → ASSIGNED
Keywords: leave-open

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.

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.

Flags: needinfo?(james)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)

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.

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.

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.

(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

Type: defect → enhancement
Flags: needinfo?(hskupin) → needinfo?(james)
Priority: -- → P3
Type: enhancement → defect

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.

Flags: needinfo?(james)
Severity: -- → S3

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.

Using cargo clippy --fix

Blocks: 1747306
Pushed by james@hoppipolla.co.uk:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32412 for changes under testing/web-platform/tests

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

Flags: needinfo?(james)
Upstream PR was closed without merging
Pushed by james@hoppipolla.co.uk:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Flags: needinfo?(james)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Summary: Add opt-in support for hosts other then local loopback interfaces → Add opt-in support for hosts and origins other then local loopback interfaces
See Also: → 1750689
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.