feat: Option to not trust X-Forwarded-* headers from clients

[rhansen]

If header values from a malicious client are passed to the backend server unchecked and unchanged, the client may be able to subvert security checks done by the backend server.

[thaJeztah]

There's some more details about common use-cases for this on this commit; 0028cda#commitcomment-10573134 so I expect that changing this could be a breaking change.

Do you have more details about the security checks? I'd expect that a security check that depends on headers that can be controlled by the client (in the non-proxy situation a client would also be able to set these) would already be affected by this

[rhansen]

It is common practice to have a "trust proxy" option in the backend server (example). If disabled, the backend server assumes that untrusted clients might connect directly to the server. If enabled, they assume untrusted clients cannot set certain headers; those headers are reserved solely for a trusted reverse proxy. If a client can manipulate those headers then the assumption is violated which an attacker might be able to exploit.

[thaJeztah]

Gotcha; so I guess in the example mentioned on the commit, there's a chain of proxies, and the first (AWS) proxy would be considered "trusted". Wondering if there should be an option to preserve that use-case (possibly opt-in/opt-out).

[rhansen]

There could be an option, but if so it should default to overwriting the client-supplied values for safety. The only legitimate use case for allowing client-supplied values is if there are multiple layers of trusted reverse proxies, which is rare. I think it would be better to keep the code simple rather than add a potentially dangerous option to support a rare use case. If a user does run multiple layers of reverse proxy then they can replace the template or use the nginx image directly.

[buchdag]

Relevant previous PR for context : #41 and #587. I'm not really confortable about calling it a "rare use case" (based on what metric ?), breaking backward compatibility and calling it a day.

[rhansen]

Relevant previous PR for context : #41 and #587. I'm not really confortable about calling it a "rare use case" (based on what metric ?), breaking backward compatibility and calling it a day.

OK. I changed this PR so that the default behavior is unchanged, but added a new option that disables the forwarding of the X-Forwarded-Proto and X-Forwarded-Port headers.

[rhansen]

Friendly ping @buchdag

[buchdag]

@rhansen I took a glance at the changes, looks good to me, I'll take a closer look and merge by the end of next week at the latest.

[SilverFire]

This is a pretty serious flaw that requires a CVE registration, as it happened in other OpenSource projects: CVE-2020-28483 for go/gin, CVE-2020-35590 for WordPress, CVE-2020-13485 for CraftCMS.

Making TRUST_DOWNSTREAM_PROXY property defaulted to false leaves nginx-proxy vulnerable, unless the user takes explicit actions to enable filtering. I would prefer to use a safe default instead.

Also, the application developers usually specify trusted proxy server IP addresses explicitly and frameworks provide properties for this kind of configuration. For example, Symfony – PHP framework, Express – JS framework, Gin – Go framework.

I would probably introduce a TRUSTED_DOWNSTREAM_PROXY_NETWORKS property with the default value of private networks, as defined in network_internal.conf.

[rhansen]

I would probably introduce a TRUSTED_DOWNSTREAM_PROXY_NETWORKS property

@SilverFire Instead of or in addition to TRUST_DOWNSTREAM_PROXY? If your suggestion is in addition to this PR then we can merge this PR as-is and open a separate PR for your suggestion.

[SilverFire]

I'd say instead.

TRUSTED_DOWNSTREAM_PROXY_NETWORKS="0.0.0.0/0" would be an equivalent of TRUST_DOWNSTREAM_PROXY=1

TRUSTED_DOWNSTREAM_PROXY_NETWORKS="" would be an equivalent of TRUST_DOWNSTREAM_PROXY=0

[rhansen]

TRUSTED_DOWNSTREAM_PROXY_NETWORKS="" would be an equivalent of TRUST_DOWNSTREAM_PROXY=0

The trouble with this approach is the empty string (and unset) should keep the current behavior for compatibility reasons (at least until the next major release). That means that if someone wants to completely turn it off (don't trust any client headers), they would need to set it to an invalid address like 0.0.0.0/32 or 255.255.255.255/32, which is awkward. Though I guess we could accept a special value like none.

[rhansen]

@SilverFire What do you think about having TRUST_DOWNSTREAM_PROXY accept either a boolean or networks? Then we could merge this PR as-is and extend it to accept networks in a future PR.

[SilverFire]

Damm, this pull-request is about DOWNSTREAM proxies, not UPSTERAM 😕
Yes, I think this one should be merged as-is.

I'll create a separate issue for upstream proxy trust lists.

[buchdag]

@SilverFire just to check that we're on the same page:

• this PR should be merged as is with no change to the default behaviour
• another PR should be made regarding UPSTREAM proxy trust, with a change to the default behaviour

Did I get that right ?

[SilverFire]

Absolutely

[rhansen]

Friendly ping @buchdag

[buchdag]

Thanks @rhansen 👍