get_host will use X-Forwarded-Host by default

[danielrichman]

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]

Yeah that's actually pretty terrible.

[untitaker]

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.

[danielrichman]

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]

Can't argue with that.

[mitsuhiko]

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]

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]

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]

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]

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]

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

[danielrichman]

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]

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]

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

[untitaker]

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

[danielrichman]

https://github.com/danielrichman/werkzeug/compare/trusted-hosts-docs ?

[untitaker]

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]

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 Wrong Location when doing a redirect behind a Proxy. #540 and X-Forwarded-Host may contain commas #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]

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]

@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]

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

[thewilkybarkid]

I argue that this violates the principle of least surprise.

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