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

get_host will use X-Forwarded-Host by default #609

Closed
danielrichman opened this issue Oct 3, 2014 · 21 comments

Comments

Projects
None yet
6 participants
@danielrichman
Copy link
Contributor

commented Oct 3, 2014

Here: https://github.com/mitsuhiko/werkzeug/blob/cbd049d88727936173386d2e80bb5ffa51fedd6e/werkzeug/wsgi.py#L141

I don't think that this is a safe default.
When using flask it gives any client the ability to set an arbitrary hostname in request.url.

This surprised me, since http://werkzeug.pocoo.org/docs/0.9/contrib/fixers/#werkzeug.contrib.fixers.ProxyFix exists to explicitly enable support for these headers (X-Forwarded-For, X-Forwarded-Host, ...).

And I think that this is misleading: http://werkzeug.pocoo.org/docs/0.9/wrappers/#werkzeug.wrappers.BaseRequest.trusted_hosts since a web server must not only “not route invalid hosts to the application”, but it must also delete any foreign X-Forwarded-Host headers.

@untitaker

This comment has been minimized.

Copy link
Member

commented Oct 4, 2014

Yeah that's actually pretty terrible.

@untitaker untitaker added the blocker label Oct 4, 2014

@untitaker untitaker added this to the 1.0 milestone Oct 20, 2014

@untitaker

This comment has been minimized.

Copy link
Member

commented Oct 20, 2014

It actually turns out this isn't a security issue.

  • Both X-Forwarded-Host and Host can already be forged by the user, since they are user-provided headers.
  • X-Forwarded-For, which contains the user's IP address, can be forged (and therefore ProxyFix should only be used behind, well, proxies), but get_host doesn't need or use that anyway.

@untitaker untitaker closed this Oct 20, 2014

untitaker added a commit that referenced this issue Oct 20, 2014

@untitaker untitaker removed this from the 1.0 milestone Oct 20, 2014

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2014

I disagree: while the user can specify arbitrary Host and X-Forwarded-Host headers, Werkzeug—by its own admission—“trusts” the Host header in some sense:

This is the recommended setup as a webserver should manually be set up to not route invalid hosts to the application.
(http://werkzeug.pocoo.org/docs/0.9/wrappers/#werkzeug.wrappers.BaseRequest.trusted_hosts)

This documentation is at odds with the behaviour of the get_host function. Either

  • it is correct to trust the X-Forwarded-Host header by default, in which case the above documentation for trusted hosts is wrong, since it implies that if your web server sanitises Host you are safe; (it should mention that the web server should not route requests with invalid X-Forwarded-Host to the application?)
  • or, get_host shouldn't trust X-Forwarded-Host by default.

There are cases when you need to be able to trust the Host header. For example:

  • some SSO systems
  • generating external urls to be used in, say, emails (potential phishing attacks?)
  • etc...

Therefore, I personally believe that it should not trust X-Forwarded-Host default. However, as I've explained above, I believe there to be a documentation bug even if you disagree with me.

@untitaker untitaker added this to the 1.0 milestone Oct 20, 2014

@untitaker

This comment has been minimized.

Copy link
Member

commented Oct 20, 2014

Can't argue with that.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Oct 22, 2014

I'm a bit confused about the exact issue here but I'm not opposed to changing it. In any case the documentation should clearly mention this behavior.

The way the system works is this: the server has two sources for where the host can come from: the X-Forwarded-Host and the Host header. Both of which can be modified by the client. If you have a proxy in front you can sanitize them. Alternatively to sanitizing you can use the trusted_hosts parameter which Werkzeug will use to ensure that the host is in the list of trusted hosts. If it is not, a SecurityError is raised which will abort the request.

There are cases when you need to be able to trust the Host header. For example:

You cannot trust the host parameter. That's entirely impossible because nothing stops a client from forging the request. What you can do however is to make sure you only allow a single host value (by setting the trusted hosts to a list of that one host that is valid).

I'm not ruling out I'm wrong on this but I would love to hear where the mistake in thinking is.

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2014

So the 'mistake' is the documentation; AFAICT nothing mentions this behaviour, and documentation for other things (ProxyFix and Request.trusted_hosts) imply untrue things (that is, the presence of X-Forwarded-Host in ProxyFix implies that it is not used by default, and trusted_hosts should mention that if the webserver is to sanitise the request it must also sanitise X-Forwarded-Host).

However, this 'mistake' would also be fixed by changing the behaviour of get_host; that is, ignoring X-Forwarded-Host. Indeed, if those two lines were removed, then ProxyFix could be enabled if you desired the old behaviour (which kind of makes more sense to me).

I guess I've been simultaneously arguing both points in the same issue.

If you have a proxy in front you can sanitize them. Alternatively to sanitizing you can use the trusted_hosts parameter which Werkzeug will use to ensure that the host is in the list of trusted hosts.

I think that ultimately what I'm saying is that while people are likely to sanitise the Host header (as a side effect of vhosting / if they need it to be sanitised), most people won't think of X-Forwarded-Host.

I believe that it's quite likely that this could catch someone out, if they don't happen to read that section of the docs. Having to explicitly enable support for X-Forwarded-Host feels like a safer default.

@untitaker

This comment has been minimized.

Copy link
Member

commented Oct 22, 2014

AFAICT Werkzeug's default behavior seems to be problematic if no proxy is used, but the hostname is still used for delegating the requests to the correct application (i think that's the case when using Apache vhosts and mod_wsgi, though i never used such a setup). In that case the user could send a Host header for Apache, but send X-Forwarded-Host for Werkzeug.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Oct 22, 2014

I believe that it's quite likely that this could catch someone out, if they don't happen to read that section of the docs. Having to explicitly enable support for X-Forwarded-Host feels like a safer default.

The problem with that is that you break all the things that currently rely on this behavior which I assume are many. I know that I generally do not use a middleware to fix it up. It also has been behavior for many years and is documented on the get_host function. Changing this is a big thing.

I think that ultimately what I'm saying is that while people are likely to sanitise the Host header

Out of curiosity: how do you sanitize the host header?

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2014

My Host header is sanitised as a side-effect of nginx's virtual hosting. The whitelist is in server_name and then another server is set as the default_server; it issues redirects to the correct host. I believe I'm doing what trusted_hosts recommends

I believe this has been discussed before:
#540 (comment)
#371 (comment)

I'm curious as to the situations in which X-Forwarded-Host is necessary: with X-Forwarded-For the act of proxying necessarily changes the remote address and so this header is necessary. Is it not normal for the proxy server to pass requests on with the original Host header?

@untitaker untitaker removed the blocker label Jan 22, 2015

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

Removing blocker label because the fix is breaking backwards-compat.

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

Could you at least fix the documentation here https://github.com/mitsuhiko/werkzeug/blob/master/werkzeug/wrappers.py#L191 to mention that both

  • a webserver should manually be set up to not route invalid hosts to the application
  • a webserver should clear the X-Forwarded-Host header if it does not use it

...and we can discuss whether or not this is a sane default behaviour or not later?

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

I think we should document this in the docstring of get_host and refer to it in this param's docstring. What do you think?

Also, if you have concrete ideas about the wording, patches welcome! :)

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2015

Do you think that the documentation for the host, url, ... properties of BaseRequest should refer to trusted_hosts?

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

I have no opinion or idea about this at all... I find it hard to predict how users will read your documentation.

@danielrichman

This comment has been minimized.

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

Yeah looks great! I merged your changes for now, and hopefully we'll be able to figure out something better for 1.0. Thanks, not only for the patch, but for bringing this issue up @danielrichman!

@danielrichman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2015

Okay, so: the current documentation correctly describes the current behaviour, however, I believe this issue should stay open since I do not think that using X-Forwarded-Host is a sensible default.

To summarise arguments so far:

@mitsuhiko (correctly) says

The way the system works is this: the server has two sources for where the host can come from: the X-Forwarded-Host and the Host header. Both of which can be modified by the client. If you have a proxy in front you can sanitize them.

I argue that this violates the principle of least surprise.

  • I do not think the typical person would think about or expect werkzeug to use X-Forwarded-Host by default.
  • The fact that the old documentation read "this is the recommended setup as a webserver should manually be set up to not route invalid hosts to the application" lends weight to my claim that most people wouldn't think of this---even you guys didn't!
  • Setting up the web server to "not route invalid hosts to the application" is quite a common thing to do, whereas sanitising X-Forwarded-Host is not(?).
  • The typical proxy simply passes on the Host header unmodified; it has no need for X-Forwarded-Host.
  • Moreover, while the need for X-Forwarded-For is obvious; you only need X-Forwarded-Host in weird cases?
  • The fact that fixers.ProxyFix exists implies that one has to specifically enable support for X-Forwarded-* headers, which is the case for X-Forwarded-For, etc., but is not the case for -Host.
  • In fact, ProxyFix itself will read the X-Forwarded-Host header (as well as the others) and overwrite the Host header! That is, the feature "use X-Forwarded-Host" is implemented twice.

Miscellaneous

  • Backwards compat: would you not expect someone that needs this header to be using ProxyFix anyway, for X-Forwarded-For, etc.?
  • This has been discussed before in #540 and #371. Above, I provide arguments that this is not a sensible default. These two issues go one further: they provide examples where the current X-Forwarded-Host behaviour actually breaks things. In both it is suggested that this should be the job of ProxyFix only.
@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 24, 2015

I believe this issue should stay open since I do not think that using X-Forwarded-Host is a sensible default.

I did not close it! I just postponed it to version 1.0, because at this version we're allowed to make compatibility-breaking changes.

would you not expect someone that needs this header to be using ProxyFix anyway, for X-Forwarded-For, etc.?

I can imagine that developers don't do anything if the current behavior seems to "work".

Thanks for the summary.

@davidism

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

@untitaker I have marked this as a blocker. We should fix this in the next feature release. Waiting for 1.0 is too long. People should be using ProxyFix for this behavior.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

👍 for not doing any implicit HTTP_X_FORWARDED overrides and having people use ProxyFix for it

@thewilkybarkid

This comment has been minimized.

Copy link

commented Sep 21, 2017

I argue that this violates the principle of least surprise.

Yep, we've just been surprised to find this is the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.