Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HostAuthorization rack-protection middleware #2053

Merged
merged 34 commits into from
Nov 18, 2024

Conversation

dentarg
Copy link
Member

@dentarg dentarg commented Nov 5, 2024

The Sinatra project received a security report with the following details:

Title: Reliance on Untrusted Inputs in a Security Decision
CWE ID: CWE-807
CVE ID: CVE-2024-21510
Credit: t0rchwo0d
Description: The sinatra package is vulnerable to Reliance on Untrusted
Inputs in a Security Decision via the X-Forwarded-Host (XFH) header.
When making a request to a method with redirect applied, it is possible
to trigger an Open Redirect Attack by inserting an arbitrary address
into this header. If used for caching purposes, such as with servers
like Nginx, or as a reverse proxy, without handling the
X-Forwarded-Host header, attackers can potentially exploit Cache
Poisoning or Routing-based SSRF.

The vulnerable code was introduced in fae7c01. Sinatra can not know whether the header value can be trusted or not without input from the app creator. This change introduce the host_authorization settings for that.

It is implemented as a Rack middleware, bundled with rack-protection, but not exposed as a default nor opt-in protection. It is meant to be used by itself, as sharing reaction with other protections is not ideal (see #2012).

@dentarg dentarg mentioned this pull request Nov 5, 2024
@dentarg dentarg force-pushed the host_authorization branch from 4b63d7d to 691a573 Compare November 8, 2024 07:39
The Sinatra project received a security report with the following
details:

> Title: Reliance on Untrusted Inputs in a Security Decision
> CWE ID: CWE-807
> CVE ID: CVE-2024-21510
> Credit: t0rchwo0d
> Description: The sinatra package is vulnerable to Reliance on Untrusted
> Inputs in a Security Decision via the `X-Forwarded-Host (XFH)` header.
> When making a request to a method with redirect applied, it is possible
> to trigger an Open Redirect Attack by inserting an arbitrary address
> into this header. If used for caching purposes, such as with servers
> like Nginx, or as a reverse proxy, without handling the
> `X-Forwarded-Host` header, attackers can potentially exploit Cache
> Poisoning or Routing-based SSRF.

The vulnerable code was introduced in fae7c01. Sinatra can not know
whether the header value can be trusted or not without input from the
app creator. This change introduce the `permitted_hosts` setting for
that.

It is implemented as a Rack middleware, bundled with rack-protection,
but not exposed as a default nor opt-in protection. It is meant to be
used by itself, as sharing reaction with other protections is not ideal,
see sinatra#2012.
@dentarg dentarg force-pushed the host_authorization branch from 691a573 to fb2d76d Compare November 8, 2024 08:18
@dentarg dentarg changed the title Add permitted_hosts setting Add HostAuthorization protection Nov 8, 2024
@dentarg dentarg changed the title Add HostAuthorization protection Add HostAuthorization rack-protection middleware Nov 11, 2024
If a forwarded header is present, that's what Rack::Request#host
returns. Applications may be making decisions based on the Host header,
even when the forwarded header is present, so we should ensure both
headers are permitted.

Reference: rails/rails#33145 (comment)
I doubt this can happen for real, but better safe than sorry.
@dentarg dentarg marked this pull request as ready for review November 14, 2024 10:59
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dentarg
Copy link
Member Author

dentarg commented Nov 19, 2024

No such plans exist.

csutter added a commit to alphagov/search-api that referenced this pull request Nov 20, 2024
Sinatra 4.1 ships with a breaking change that [enables host allowlisting
middleware][1] in local development, which in turn breaks running Search
API through GOV.UK Docker.

This adds an explicit `permitted_hosts` setting for the middleware,
which includes the `search-api.dev.gov.uk` host used by GOV.UK Docker.

[1]: sinatra/sinatra#2053
joshuafleck added a commit to meetcleo/avro_turf that referenced this pull request Nov 20, 2024
Per [this](sinatra/sinatra#2053) change, "Defaults to .localhost, .test and any IP address in development mode."

We want our fake server to be able to respond to any request, hence we allow all hosts.
csutter added a commit to alphagov/search-api that referenced this pull request Nov 20, 2024
Sinatra 4.1 ships with a breaking change that [enables host allowlisting
middleware][1] in local development, which in turn breaks running Search
API through GOV.UK Docker.

This adds an explicit `permitted_hosts` setting for the middleware in
the development environment, which allows access with arbitrary host
headers.

[1]: sinatra/sinatra#2053
benoittgt added a commit to benoittgt/webmock that referenced this pull request Nov 22, 2024
While working on this sinatra/sinatra#2053 in
our project. I noticed than when using Webmock, sinatra logs and
especially the enforced rack-protection were showing this kind of logs:

```
D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil
```

As you can see, `origin_host` is empty, because the header is missing.

When not using webmock, we fallback on `net/http` host header setup.
https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
benoittgt added a commit to benoittgt/webmock that referenced this pull request Nov 22, 2024
While working on this sinatra/sinatra#2053 in
our project. I noticed than when using Webmock, sinatra logs and
especially the enforced rack-protection were showing this kind of logs:

```
D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil
```

As you can see, `origin_host` is empty, because the header is missing.

When not using webmock, we fallback on `net/http` host header setup.
https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
benoittgt added a commit to benoittgt/webmock that referenced this pull request Nov 22, 2024
While working on this sinatra/sinatra#2053 in
our project. I noticed than when using Webmock, sinatra logs and
especially the enforced rack-protection were showing this kind of logs:

```
D, [2024-11-22T13:05:16.798156 #26673] DEBUG -- : Rack::Protection::HostAuthorization @all_permitted_hosts=[".company.com"] @permitted_hosts=["company.com"] @domain_hosts=[/\A(?-mix:[a-z0-9\-.]+)company\.com\z/i] @ip_hosts=[] origin_host="" forwarded_host=nil
```

As you can see, `origin_host` is empty, because the header is missing.

When not using webmock, we fallback on `net/http` host header setup.
https://github.com/ruby/net-http/blob/cfbbb50c931a78fc2b5c731b9abeda161e1dfdd1/lib/net/http.rb#L2482
@benoittgt
Copy link

For people who use webmock. You could have surprising result. I've open a patch to set the host in case of mock of net-http. bblimke/webmock#1084

YOU54F added a commit to pact-foundation/pact-ruby that referenced this pull request Nov 29, 2024
YOU54F added a commit to pact-foundation/pact-ruby that referenced this pull request Nov 29, 2024
TylerHelmuth added a commit to honeycombio/libhoney-rb that referenced this pull request Dec 2, 2024
## Which problem is this PR solving?

- [Sinatra 4.1.0 introduced a host authorization option to Sinatra
apps](sinatra/sinatra#2053). That authorization
check was failing requests made in tests.

## Short description of the changes

-  Update the Honeycomb test server stub to allow anybody to talk to it.

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
robbkidd added a commit to honeycombio/beeline-ruby that referenced this pull request Dec 2, 2024
## Which problem is this PR solving?

- [Sinatra 4.1.0 introduced a host authorization option to Sinatra apps](sinatra/sinatra#2053). That authorization check was failing requests made in tests.

## Short description of the changes

-  Update the test Sinatra server to allow anybody to talk to it.

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
robbkidd added a commit to honeycombio/beeline-ruby that referenced this pull request Dec 2, 2024
## Which problem is this PR solving?

- [Sinatra 4.1.0 introduced a host authorization option to Sinatra
apps](sinatra/sinatra#2053). That authorization
check was failing requests made in tests.

## Short description of the changes

-  Update the test Sinatra server to allow anybody to talk to it.

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
koetsier added a commit to GovWifi/govwifi-acceptance-tests that referenced this pull request Dec 16, 2024
….1.0

The acceptance tests failed because Sinatra 4.1.0 introduced `HostAuthorization` sinatra/sinatra#2053, which rejects connections from remote hosts by default unless explicitly allowed. This behavior affects environments where the app is accessed from remote addresses during tests.

Setting `APP_ENV` to `production` fixes that, allowing the tests to pass without further configuration. Updated the environment setup to include this fix.
koetsier added a commit to GovWifi/govwifi-acceptance-tests that referenced this pull request Dec 17, 2024
….10 (#165)

* Fix acceptance test failures caused by HostAuthorization in Sinatra 4.1.0

The acceptance tests failed because Sinatra 4.1.0 introduced `HostAuthorization` sinatra/sinatra#2053, which rejects connections from remote hosts by default unless explicitly allowed. This behavior affects environments where the app is accessed from remote addresses during tests.

Setting `APP_ENV` to `production` fixes that, allowing the tests to pass without further configuration. Updated the environment setup to include this fix.

* Replace docker-compose with docker compose

Updated to use `docker compose`, the newer and recommended way of managing multi-container Docker applications, replacing the deprecated `docker-compose` command.
mishina2228 added a commit to mishina2228/youtube-api-trial that referenced this pull request Dec 27, 2024
mishina2228 added a commit to mishina2228/youtube-api-trial that referenced this pull request Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants