X-Forwarded-For improvements and bug fixes

[colinodell]

Breaking change: Anyone who has enabled use_x_forwarded_for will need to explicitly whitelist their proxy/proxies using the new trusted_proxies setting.

---

This PR builds upon #15182 by making four changes:

• Enhancement: Adds a new trusted_proxies setting exclusively for the X-Forwarded-For whitelist
• Bug fix: Only trust the last IP in the X-Forwarded-For header
• Bug fix: Ignore nonsense X-Forwarded-For header values instead of issuing HTTP 500 errors
• Adds two additional tests to cover some cases mentioned in Security issue, unauthorized trusted access by spoofing x-forwarded-for header #14345 (comment)

Description:

New trusted_proxies setting

PR #15182 fixed issues where X-Forwarded-For headers were blindly trusted by HA. That PR used the trusted_networks setting (originally designed to bypass password authentication) to also determine whether the request was sent from a trusted proxy.

While this did resolve the security issue, it also introduced a potential downside: If (for some reason) the HTTP requests originates from the same IP as the proxy server, then authentication will be bypassed. (Note that this does not affect traffic originating from outside the proxy server, which is the typical use case.)

For example, let's say you're running HA on your local machine with this configuration:

http:
  api_password: YOUR_PASSWORD
  use_x_forwarded_for: True
  trusted_networks:
    - 127.0.0.1

And let's say you've also got nginx running locally as your proxy. Well, nginx is going to send an X-Forwarded-For: 127.0.0.1 header, so that'll be your real IP, and that'll also match the trusted_networks used by the auth middleware.

Some users may need to set a wider range of trusted proxies without allowing proxied requests from those same IPs to bypass password authentication. This PR therefore adds a new trusted_proxies setting exclusively for the X-Forwarded-For whitelist to use.

This change allows the user to trust a wide range of proxies (perhaps they're using a cloud proxy like Cloudflare with multiple IP ranges) while keeping the auth-bypass range extremely small.

See #15182 (comment) for further information.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5624

Use last IP in header

The X-Forwarded-For header usually only contains a single IP. However, it's possible that some reverse proxies may respect an existing header and append the IP they detect at the end. According to Wikipedia:

The last IP address is always the IP address that connects to the last proxy, which means it is the most reliable source of information

If multiple IPs are present, we should not trust any previous ones in the string since those come from potentially untrusted sources - only the last one is from our trusted proxy. The second commit in this PR therefore ensures that we only look at this IP.

(It may be possible to add support for checking those intermediate proxies in the future. However, because HA has previously only supported a single upstream proxy via the use_x_forwarded_for feature, we're doing the same here and via #15182 - just in a safer way.)

Example entry for configuration.yaml (if applicable):

Let's pretend our home IP is 12.34.56.78 and we're using proxying through Cloudflare. We don't want to be prompted for a password if we're connecting from home.

Before:

http:
  api_password: YOUR_PASSWORD
  use_x_forwarded_for: True
  trusted_networks:
    # Pretend this is your home IP
    - 12.34.56.78
    # IPs from https://www.cloudflare.com/ips/
    - 103.21.244.0/22
    - 103.22.200.0/22
    - 103.31.4.0/22
    - 104.16.0.0/12
    - 108.162.192.0/18
    - 131.0.72.0/22
    - 141.101.64.0/18
    - 162.158.0.0/15
    - 172.64.0.0/13
    - 173.245.48.0/20
    - 188.114.96.0/20
    - 190.93.240.0/20
    - 197.234.240.0/22
    - 198.41.128.0/17

If a malicious attacker is able to issue HTTP requests from within Cloudflare's infrastucture (maybe they hacked a CF server), those requests would bypass password authentication. Remember, this does NOT affect proxied traffic originating outside of Cloudflare.

After:

http:
  api_password: YOUR_PASSWORD
  use_x_forwarded_for: True
  trusted_networks:
    # Pretend this is your home IP
    - 12.34.56.78
  trusted_proxies:
    # IPs from https://www.cloudflare.com/ips/
    - 103.21.244.0/22
    - 103.22.200.0/22
    - 103.31.4.0/22
    - 104.16.0.0/12
    - 108.162.192.0/18
    - 131.0.72.0/22
    - 141.101.64.0/18
    - 162.158.0.0/15
    - 172.64.0.0/13
    - 173.245.48.0/20
    - 188.114.96.0/20
    - 190.93.240.0/20
    - 197.234.240.0/22
    - 198.41.128.0/17

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

• Tests have been added modified to verify that the new code works.

[colinodell]

I have tested this functionality locally and on a VPS with a proxy, and everything seems to work correctly. Like before, if anyone else would like to test this to confirm my results I'd appreciate it. Otherwise, this should be ready to go once the Travis tests pass.

[ayavilevich]

@colinodell thanks for the work. This is very good. The only issue I can see is backward compatibility. If trusted_proxies is not defined, where doe sit fall back to? [] or trusted_network?
To be effective, this should also be well documented.

[ayavilevich]

@colinodell also, what happens if a middleware module throws an exception? For example of somebody passes nonsense to the header and the ip_address call throws.

[houndci-bot]

line too long (98 > 79 characters)

[colinodell]

@ayavilevich I really appreciate your continued feedback on this!

I have added a couple new tests in commit 6bf513d to cover some of the scenarios you outlined in the other thread. (Because trusted_networks is now completely disconnected from determining the real IP and therefore not relevant here, I have not implemented those exact tests.)

The only issue I can see is backward compatibility. If trusted_proxies is not defined, where doe sit fall back to? [] or trusted_network?
To be effective, this should also be well documented.

It falls back to []. Anyone who has enabled use_x_forwarded_for will need to explicitly whitelist their proxy/proxies using the new trusted_proxies setting. While this does technically break backward compatibility, the previous behavior is a security risk and so I feel this BC break is justified. I completely agree that this should be noted in the changelog / release announcement.

We don't want to fall back to trusted_networks due to the downsides we discussed in that thread. This PR completely remove all references to trusted_networks when determining the "real IP".

what happens if a middleware module throws an exception? For example of somebody passes nonsense to the header and the ip_address call throws.

Good call, I have added a try...except block via 8b2796b so that nonsense values are ignored instead of resulting in HTTP 500 errors. The commit also includes a test for this.

Let me know if this looks good or if there's anything else you'd like to see here.

[ayavilevich]

@colinodell looks good, here are some comments which you need to check potentially.

You are right, trusted_networks should not affect the real_ip algorithm.
It is only relevant in the context of a spoofed real_ip and the authentication bypass.

What does pass do in the middleware? Will the KEY_REAL_IP get a value is pass is called? What will the KEY_REAL_IP value be on nonsense header?

I think that you need to have trusted_proxies to default to trusted_networks.
Imagine a user who uses use_x_forwarded_for today. He doesn't have a trusted_proxies setting but might have a trusted_networks setting with the IP of the proxy inside.
After the release, assuming he will not add the new field, what will be the KEY_REAL_IP in his requests? If this will now becomes peername then it will always be the IP of the proxy which could be in the trusted list and allow authentication bypass for everybody.

Is any use case with more than one trusted proxy in chain is not supported at this time? I think that is fine and would be too much of a corner case. If somebody has such a config they could config the deeper proxies to forward (as-is) the header of the external facing proxy rather than rewrite it.

[balloob]

@ayavilevich the assignment is the last statement in the code, so if an exception is raised while calculating the value to assign, the assignment itself is not done.

I've updated the PR description with the breaking change that we will include in the release notes.

[balloob]

I think that we should not default trusted proxies to the value of trusted networks. That seems like an unexpected coupling. It is better to do a breaking change.

[balloob]

Thanks for all the work on this Colin, this looks great.

[ayavilevich]

@balloob @colinodell

I think that we should not default trusted proxies to the value of trusted networks. That seems like an unexpected coupling. It is better to do a breaking change.

I understand and agree that a breaking change is acceptable in some cases, but the way it breaks matters. If it breaks by "not working" that is one thing but if it breaks with an "authentication bypass", meaning anybody on the internet will be able to access a person's HA instance, that is another thing completely. You might be potentially creating a bigger security issue than we had in the first place.

I have another suggestion on how to resolve backward compatibility without coupling the two fields.
Add a check, either on initialization or each request, to see if use_x_forwarded_for==true and trusted_proxies is empty in that case, log an error and stop any further request processing.
As far as I can see at this time, If you fail to do so you will incorrectly assume that the real IP is the IP of the proxy, which is most likely a trusted IP for the purpose of authentication bypass.

use_x_forwarded_for and trusted_proxies are both part of the same mechanism, so no problem to couple them IMO. Plus, with a log entry, it will be easier for a user of use_x_forwarded_for to know that the needs to add a new field to his configuration now. After all, not everybody reads release notes.