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

Guard against DNS rebinding attacks by permitting hosts #33145

Merged
merged 2 commits into from
Dec 17, 2018

Conversation

gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Jun 16, 2018

  • Introduce guard against DNS rebinding attacks

    The ActionDispatch::HostAuthorization is a new middleware that prevent
    against DNS rebinding and other Host header attacks. It is included in
    the development environment by default with the following configuration:

     Rails.application.config.hosts = [
       IPAddr.new("0.0.0.0/0"), # All IPv4 addresses.
       IPAddr.new("::/0"),      # All IPv6 addresses.
       "localhost"              # The localhost reserved domain.
     ]
    

    In other environments Rails.application.config.hosts is empty and no
    Host header checks will be done. If you want to guard against header
    attacks on production, you have to manually whitelist the allowed hosts
    with:

     Rails.application.config.hosts << "product.com"
    

    The host of a request is checked against the hosts entries with the case
    operator (#===), which lets hosts support entries of type RegExp,
    Proc and IPAddr to name a few. Here is an example with a regexp.

     # Allow requests from subdomains like `www.product.com` and
     # `beta1.product.com`.
     Rails.application.config.hosts << /.*\.product\.com/
    

    A special case is supported that allows you to whitelist all sub-domains:

     # Allow requests from subdomains like `www.product.com` and
     # `beta1.product.com`.
     Rails.application.config.hosts << ".product.com"
    

EDIT:

This is an attack that was brought to me by @homakov a few years ago. You can learn more about it in this [lightning talk](https://www.youtube.com/watch?v=WnlgKWCt8wQ&t=2063) or this [tweet](https://twitter.com/homakov/status/1005098991946641409). The attack works on my computer to this day. It's also universal across languages and frameworks. As an example, here is how [Django](https://docs.djangoproject.com/en/2.0/ref/settings/#allowed-hosts) tackles it.

The fix is pretty easy, whitelist the addresses that a request can go to. However,
as simple as the fix is, it will introduce a backwards incompatible change that will
require all apps in Rails 6 to whitelists their public hosts. Currently, I have come
up with this configuration:

Rails.application.config.hosts << "product.com"

The default value of Rails.application.config.hosts is:

Rails.application.config.hosts = ['localhost', '127.0.0.1', '::1']

The host of a request is checked against the hosts entries with the case
operator (#===), which lets hosts support entries of type RegExp,
Proc and IPAddr to name a few. Here is an example with a regexp.

# Allow requests from subdomains like `www.product.com` and
# `beta1.product.com`.
Rails.application.config.hosts << /.*\.product\.com/

If a request to the host is not allowed, the following response will be
generated:

Requests to badhost.com are not allowed! To allow them:

  Rails.application.config.hosts << "badhost.com"

This can be customized through the Rails.application.config.hosts_response_app
option:

Rails.application.config.hosts_response_app = -> env do
  request = ActionDispatch::Request.new(env)

  ErrorTracker.send_error \
    ValueError.new("request from #{request.host} is not allowed")

  [403, { "Content-Type" => "text/plain" }, ["403 Forbidden"]]
end

My current worries are:

  • How do we introduce this as painlessly?

  • Is the configuration interface okay?

  • How do we document it head on?

  • Do we wanna raise an error instead of returning custom responses? I return a
    response, because of a technicality. ActionDispatch::HostAuthorization is
    the very first middleware, to cut those errors as soon as possible. Raising an
    error here wont be caught by ActionDispatch::DebugExceptions.

    However, returning a response isn't so obvious and may slip through in
    development. Maybe it can be caught in testing, staging environments, but an
    error will be much more explicit.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd
Copy link
Member

I'm happy to be convinced otherwise, but I doubt it needs to be so far up the middleware stack.

X-Forwarded-Host.

I do think it'd be nicer to have this far enough down-stack that it can raise an exception... but failing that, I'm inclined to YAGNI hosts_response_app in favour of a single built-in behaviour, unless you have a specific alternative handler in mind.

Watch the regexp anchoring: either the examples should be full of \z, or perhaps the safer option would be to add it automatically.

I see no reason we wouldn't allow all IP addresses by default; they are by definition not subject to DNS rebinding. (We could ask the OS for the machine hostname(s) too... those are technically subject to rebinding, but seem fine to trust by default. Further to that, is there a way, when resolving a name, to know whether it came from /etc/hosts or a DNS server?)

Should we do this in all environments by default, or only ones that grant special privileges? (For that matter, must an unknown host = blocked request, or could it just mean that local_request? will be false?)


I still keep expecting the browsers to find a fix for this, as it's an attack on their same-origin mechanism, but I do agree it's been a while and isn't looking great so far.

cc @rails/security

@homakov
Copy link
Contributor

homakov commented Jun 18, 2018

I still keep expecting the browsers to find a fix for this, as it's an attack on their same-origin mechanism, but I do agree it's been a while and isn't looking great so far.

I also believe it is the browser issue, but just as it happened with CSRF they turned a blind eye to this moving the responsibility down the stack to web developers. There are no talks about fixing DNS rebinding in the browsers and it is unlikely to be ever fixed in standards.

So yes, all websites must read Host header on their own.

@gsamokovarov
Copy link
Contributor Author

I do think it'd be nicer to have this far enough down-stack that it can raise an exception...

Agree, will try to move it down the stack and see if the exploit is still happening. My concern was that WebConsole::Middleware inserts itself close to ActionDispatch::DebugExceptions, so we could execute the malicious code before raising the error. If we raise an error, we may skip the response app as well.

Should we do this in all environments by default, or only ones that grant special privileges?

Personally, I'd prefer if we don't have exceptions per environments. Hitting this error in testing may remind you to whitelist the hosts for the other environments.

I see no reason we wouldn't allow all IP addresses by default; they are by definition not subject to DNS rebinding.

We can easily do this for IPv4 and IPv6 with a wide IPAddr, I will add it today.

Further to that, is there a way, when resolving a name, to know whether it came from /etc/hosts or a DNS server?

I'm not sure about this one, but will research it.

@rafaelfranca
Copy link
Member

Is there any reason why we are introducing a new middleware by default in rails given this problem only happens when webconsole or similar projects are enabled? Why not implement on the library?

In production for example I don't think we need this given most of production environments will run rails with a proxy in front.

@homakov
Copy link
Contributor

homakov commented Jun 19, 2018

given this problem only happens when webconsole or similar projects are enabled

Webconsole is just the one that leads to RCE straight. But even w/o that default rails app still leaks lots of ENV data and just all the info about local rails app. What if the hacker wants to steal your startup idea and leaks your entire :3000 app?

Or, remember rails RCE via XML/YAML? It used to be exploitable with DNS rebinding and any machine could have been pwned (luckily very few thought of this), but with this safeguards that would not be the case.

So fixing that higher in the stack would just cover more attack scenarios.

@matthewd
Copy link
Member

@rafaelfranca I think we're best positioned to handle the config to list valid hostnames, so at that point we might was well enforce the rule too. (Per above, I think there are open questions around how much we need to enforce by default, vs just providing the tool... but I think the tool is indeed in our remit.)

@gsamokovarov gsamokovarov force-pushed the host-authorization branch 2 times, most recently from 9bd5893 to 1fbb94e Compare June 19, 2018 09:00
@gsamokovarov
Copy link
Contributor Author

@matthewd raising an error leaves the current vulnerability in place, because WebConsole::Middleware is inserted before, ActionDispatch::DebugExceptions and you can still execute code from the console on the error page itself. 😅 I can render the default error page straight from ActionDispatch::HostAuthorization? It may not need to be the first middleware, but it still needs to be before ActionDispatch::DebugExceptions.

@gsamokovarov gsamokovarov changed the title Guard agains DNS rebinding attacks by whitelisting hosts Guard against DNS rebinding attacks by whitelisting hosts Jun 19, 2018
@rafaelfranca
Copy link
Member

Does this protection makes sense in production? Because right now the host configuration lives only in the proxy level (nginx, apache, heroku dashboard). Now we have to configure in two places and we need to deploy the application in order to change/allow a new host where before we could just change the proxy config.

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Jun 19, 2018

That's the drawback. 😕 Web Console is the most common source of RCE source, but production can still fall under Host header attacks.

@rafaelfranca
Copy link
Member

This is a huge drawback to me and I don't think Host header attack mitigation requires this drawback. I prefer to only enable this by default in development. Requiring deploy to be able to serve the application is a new host is too much overhead to me and will be a huge pain to support in most deploy platforms that exists.

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Jun 19, 2018

We can let all requests in if config.hosts is empty and include a default value in the newly generated Rails 6 applications. If you want to disable this check, you can delete the line and be done with it. That way we make the transition easier (it becomes opt-in, rather than mandatory) and we are still secure by the new default.

@rafaelfranca
Copy link
Member

What would be a good default for production?

@gsamokovarov
Copy link
Contributor Author

@rafaelfranca We can have it's current default added at least to config/environment/development.rb. Wondering whether an internal default for the development environent would act tricky, as this is the most voulerable env.

For production, we can put the hostname of the machine that runs rails new in config/environments/production.rb for newly generated apps. We could also introduce the option in a new initializer file with the current defaults. It's easy to check and conditionally disable it outside of development.

@homakov
Copy link
Contributor

homakov commented Jun 22, 2018

It's best idea to fix it in development only for starters and maybe in the future add an option for production where the bug is significantly less likely.

@gsamokovarov gsamokovarov force-pushed the host-authorization branch 2 times, most recently from 24e5a23 to 1d3a9b3 Compare June 25, 2018 09:14
@benmmurphy
Copy link
Contributor

benmmurphy commented Jul 11, 2018

rafaelfranca gave me the heads up on this. i've tested this on my local machine and I believe i've found an issue. so the problem is an attacker would have full control over all headers including 'X-Forwarded-Host'. (Note: For some reason there is a comment up the stack that mentions 'X-Forwarded-Host' ?). Anyway, attacker can just set X-Forwarded-Host to a whitelist host [localhost] and then bypass the protection. I haven't actually tested a full attack so I could be wrong about this.

But what I've done is:

  1. verify X-Forwarded-Host can be set in both Chrome/Firefox. You need to be a bit careful here because if you make a request that triggers a redirect then it might look like Chrome/Firefox blocks it but they are just blocking a cross-origin request to the redirect.

This can be done doing:

var host = "<domain in address bar>"; promise = fetch("http://" + host, {headers: {'x_forwarded_host': 'localhost'}}); promise.then((d) => console.log("fake_host", d));

  1. Make a CURL request that would look similar to what the DNS rebinding attacker would look like using X-Forwarded-Host to bypass the protection.

curl -Hhost:on.the.internet -v http://localhost:3000/not_found

and I receive:

<h1>Blocked host: on.the.internet</h1>

which is good!

However:

curl -Hx-forwarded-host:localhost -Hhost:on.the.internet -v http://localhost:3000/not_found

and I don't get the blocked page and instead get the web-console stuff

This happens because the host is pulled in from rack and rack will look at the X_FORWARDED_HOST header.

I've updated http://www.dnsrebinder.net/ to set x-forwarded-host and you should be able to trigger the vulnerability locally assuming your rails app is listening to localhost:3000.

@matthewd
Copy link
Member

Note: For some reason there is a comment up the stack that mentions 'X-Forwarded-Host' ?

Yeah, I was -- perhaps just a little too tersely -- noting that same issue.

@gsamokovarov gsamokovarov force-pushed the host-authorization branch 2 times, most recently from 8246501 to bc8eaca Compare July 11, 2018 16:26
lozette added a commit to UKGovernmentBEIS/beis-report-official-development-assistance that referenced this pull request Mar 24, 2020
During a recent pen test, our application was found to be vulnerable to a Host
header poisoning atttack, in which a "bad" hostname can be injected into the
request headers and passed back to the client, where it may be used to redirect
users (or in any scenario where `url_for` is used or the `location` is passed
back to the client)

Using a built-in feature in Rails 6 rails/rails#33145
we are able to only allow known hostnames to be passed on from a request header
into the application.
zurbergram added a commit to department-of-veterans-affairs/gibct-data-service that referenced this pull request Mar 9, 2021
zurbergram added a commit to department-of-veterans-affairs/gibct-data-service that referenced this pull request Mar 10, 2021
…eturning results #21123 (#796)

* escape postgres regex characters

* rails 6 config.hosts issue

* update config to handle changes from rails/rails#33145
dsteelma-umd added a commit to dsteelma-umd/umd-handle that referenced this pull request Apr 23, 2021
Rails 6 has a new feature to prevent "DNS Rebinding" attacks, described
in rails/rails#33145, by allowing specific
hostnames to be added to an allowed list, "config.host" in the
"config/application.rb" file. If the hostname of the request does not
match a name in "config.host" the request is rejected.

By default, the "config.host" allow list is not set, and allows
connections for any host.

As part of the LIBITD-1870, the "config.host" allow list was added to
use the "HOST" environment variable. Added a "K8s_INTERNAL_HOST"
environment variable, which is added to the "config.host" in the
"config/application.rb" file, if present. This enables the
Kubernetes internal host name to be set in the k8s-umd-handle
configuration.

In the "env_example" file, provided a default value of "umd-handle-app"
for the "K8S_INTERNAL_HOST", as that seems likely to be correct
(and does not matter in the local development environment).

https://issues.umd.edu/browse/LIBITD-1906
p8 added a commit to p8/brakeman that referenced this pull request Aug 23, 2021
The Host Authorization middleware protects against DNS rebinding.
This middleware is primarily targeted at the development environment:

> It is included in the development environment by default ... In other
environments Rails.application.config.hosts is empty and no Host header
checks will be done.
rails/rails#33145

If someone decides to call `config.hosts.clear` because it's "only
development", we should warn them they are vulnerable to DNS rebinding.
p8 added a commit to p8/brakeman that referenced this pull request Aug 23, 2021
The Host Authorization middleware protects against DNS rebinding.
This middleware is primarily targeted at the development environment:

> It is included in the development environment by default ... In other
environments Rails.application.config.hosts is empty and no Host header
checks will be done.
rails/rails#33145

If someone decides to call `config.hosts.clear` because it's "only
development", we should warn them they are vulnerable to DNS rebinding.
hartsick added a commit to meedan/pender that referenced this pull request Jan 31, 2023
Configure config.hosts in development to avoid DNS binding attacks

This is configured only in development based on recommendation of the
Rails guide, and to allow us to manage whitelisting via our infrastructure
in hosted environments.

More here:
* https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#new-config-hosts-setting
* rails/rails#33145 (comment)
hartsick added a commit to meedan/pender that referenced this pull request Feb 2, 2023
Includes the following:
* Upgrade sqlite to be compatible with Rails version

We had an old version of sqlite pinned, seemingly due to a previous
incompatibility with Rails 5.2.2. This has since been fixed and we
need a new version of sqlite for Rails 6, so it gets unpinned and updated.

* Begin autoloading via zeitwork

In Rails 6, there is a new autoloader called zeitwerk. It is more
strict about where it expects constants to live - one class per file,
and each file should live in a directory structure that matches its
constant name (e.g. "Pender::Store" should be at "pender/store.rb", not
"pender_store.rb".

This commit begins using the Zeitwork autoloader by setting the following
in the application config: `config.load_defaults 6.0` and then updates
our lib constants to a directory structure that can be autoloaded. Since
we had an inconsistent module name, I also consolidated LapisConstants
into Lapis::, and created Pender::Exception::x as a bucket for all of our
Pender-related exceptions module, since they had to be split into their own
files but it made sense to give a way for them to be loaded as a set
(eg "pender/exception" by creating "pender/exception.rb").

The autoloading can be checked by running: `bin/rails zeitwerk:check`

More here: https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-2-to-rails-6-0

* Remove represent_boolean_as_integer = true, since it is now default and will be removed in 6.1

* Remove usage of ActionView::Base.new and replace with ApplicationController.render

This was causing errors because we were passing a Pathname of views to ActionView::Base.new,
and the constructor has changed to expect a lookup context. We were also receiving related
deprecation warnings (`DEPRECATION WARNING: ActionView::Base instances should be constructed
with a lookup context, assignments, and a controller.`)

We have two options to address this: passing in a lookup context to ActionView::Base, or
using ApplicationController.render. It seemed to me like we could use the latter and it
was a bit simpler of an interface, so I decided to move forward with that approach.

https://github.com/rails/rails/blob/v6.0.3.3/actionview/lib/action_view/base.rb#L258
https://stackoverflow.com/a/63865988

* Bundles URI-related errors so that we can more easily catch them

* Silences warnings from constant redefinition

Upgrades Bundler and explicitly declares Net HTTP gem so that
we stop getting so many warnings. Might be able to remove gem
declaration in Ruby 3.0.

References:
* ruby/net-imap#16
* codeforamerica/vita-min@835591e

* Add Spring to speed up development and tests

* Avoid autoloading constants during initialization

In Rails 6.0, autoloading constants during initialization has been deprecated.
This means that we should be explicitly requiring any constant that is used
in config/initializers rather than relying on autoload.

We were receiving warning signs about PenderConfig, ApiKey, and ApplicationRecord,
all of which were triggered by the inclusion of PenderConfig in the Airbrake
initializer. To address, I explicitly required each.

More information here:
* https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-2-to-rails-6-0
* https://bill.harding.blog/2021/07/22/rails-6-1-deprecation-warning-initialization-autoloaded-the-constants-what-to-do-about-it/

* Configure config.hosts in development to avoid DNS binding attacks

This is configured only in development based on recommendation of the
Rails guide, and to allow us to manage whitelisting via our infrastructure
in hosted environments.

More here:
* https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#new-config-hosts-setting
* rails/rails#33145 (comment)

* Use RequestHelper.request_url for all requests

In the Ruby 2.7 upgrade, we began using Addressable for URIs. This
resulted in some unexpected behavior when Net::HTTP.get_request was
used, beacuse Addressable::URI goes down a different code path than
URI because it is not recognized as a URI. THis was causing warnings
in Open Telemetry (because Addressable::URI made its way to target,
as opposed to the path) and potentially caused us to make incorrect
requests.

This standardizes the way we make requests across the application
when we don't need to set custom headers, using the non-shortcut
version of constructing Net::HTTP requests.

I'd like to further refactor this in the future - skipping unnecessary
lines (like teasing apart responsibilities of html_options, since we
often only use the User Agent) - and allowing for the pass-in of custom
headers, since we still use Net::HTTP to construct reqeusts elsewhere.

CV2-2668
hartsick added a commit to meedan/pender that referenced this pull request Feb 6, 2023
Includes the following:
* Upgrade sqlite to be compatible with Rails version

We had an old version of sqlite pinned, seemingly due to a previous
incompatibility with Rails 5.2.2. This has since been fixed and we
need a new version of sqlite for Rails 6, so it gets unpinned and updated.

* Begin autoloading via zeitwork

In Rails 6, there is a new autoloader called zeitwerk. It is more
strict about where it expects constants to live - one class per file,
and each file should live in a directory structure that matches its
constant name (e.g. "Pender::Store" should be at "pender/store.rb", not
"pender_store.rb".

This commit begins using the Zeitwork autoloader by setting the following
in the application config: `config.load_defaults 6.0` and then updates
our lib constants to a directory structure that can be autoloaded. Since
we had an inconsistent module name, I also consolidated LapisConstants
into Lapis::, and created Pender::Exception::x as a bucket for all of our
Pender-related exceptions module, since they had to be split into their own
files but it made sense to give a way for them to be loaded as a set
(eg "pender/exception" by creating "pender/exception.rb").

The autoloading can be checked by running: `bin/rails zeitwerk:check`

More here: https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-2-to-rails-6-0

* Remove represent_boolean_as_integer = true, since it is now default and will be removed in 6.1

* Remove usage of ActionView::Base.new and replace with ApplicationController.render

This was causing errors because we were passing a Pathname of views to ActionView::Base.new,
and the constructor has changed to expect a lookup context. We were also receiving related
deprecation warnings (`DEPRECATION WARNING: ActionView::Base instances should be constructed
with a lookup context, assignments, and a controller.`)

We have two options to address this: passing in a lookup context to ActionView::Base, or
using ApplicationController.render. It seemed to me like we could use the latter and it
was a bit simpler of an interface, so I decided to move forward with that approach.

https://github.com/rails/rails/blob/v6.0.3.3/actionview/lib/action_view/base.rb#L258
https://stackoverflow.com/a/63865988

* Bundles URI-related errors so that we can more easily catch them

* Silences warnings from constant redefinition

Upgrades Bundler and explicitly declares Net HTTP gem so that
we stop getting so many warnings. Might be able to remove gem
declaration in Ruby 3.0.

References:
* ruby/net-imap#16
* codeforamerica/vita-min@835591e

* Add Spring to speed up development and tests

* Avoid autoloading constants during initialization

In Rails 6.0, autoloading constants during initialization has been deprecated.
This means that we should be explicitly requiring any constant that is used
in config/initializers rather than relying on autoload.

We were receiving warning signs about PenderConfig, ApiKey, and ApplicationRecord,
all of which were triggered by the inclusion of PenderConfig in the Airbrake
initializer. To address, I explicitly required each.

More information here:
* https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#upgrading-from-rails-5-2-to-rails-6-0
* https://bill.harding.blog/2021/07/22/rails-6-1-deprecation-warning-initialization-autoloaded-the-constants-what-to-do-about-it/

* Configure config.hosts in development to avoid DNS binding attacks

This is configured only in development based on recommendation of the
Rails guide, and to allow us to manage whitelisting via our infrastructure
in hosted environments.

More here:
* https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#new-config-hosts-setting
* rails/rails#33145 (comment)

* Use RequestHelper.request_url for all requests

In the Ruby 2.7 upgrade, we began using Addressable for URIs. This
resulted in some unexpected behavior when Net::HTTP.get_request was
used, beacuse Addressable::URI goes down a different code path than
URI because it is not recognized as a URI. THis was causing warnings
in Open Telemetry (because Addressable::URI made its way to target,
as opposed to the path) and potentially caused us to make incorrect
requests.

This standardizes the way we make requests across the application
when we don't need to set custom headers, using the non-shortcut
version of constructing Net::HTTP requests.

I'd like to further refactor this in the future - skipping unnecessary
lines (like teasing apart responsibilities of html_options, since we
often only use the User Agent) - and allowing for the pass-in of custom
headers, since we still use Net::HTTP to construct reqeusts elsewhere.

CV2-2668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants