Repository: home-assistant/core
Issue category: Security Vulnerability

Issue:
Security issue, unauthorized trusted access by spoofing x-forwarded-for header
**Home Assistant release with the issue:**
0.68.1 but probably older versions too

**Last working Home Assistant release (if known):**
N/A

**Operating environment (Hass.io/Docker/Windows/etc.):**
Hassbian but should be applicable to other deployment methods

**Component/platform:**
http

**Description of problem:**
There is a bug in HA that in certain conditions will allow a person to spoof a trusted source address and bypass password authentication.

Conditions:

- “trusted_networks” is defined with IP values
- “use_x_forwarded_for: True”
- A reverse proxy is used (i.e nginx)
- Attacker can guess one of the IPs in the trusted list (i.e. 127.0.0.1)
- API password is set (or else there is nothing to bypass)

Use case:
Attacker adds a spoofed x-forwarded-for header with an IP that is in the trusted list. HA grants access without asking for a password.

Cause: 
https://github.com/home-assistant/home-assistant/blob/master/homeassistant/components/http/real_ip.py#L22
Uses the first IP in the header which can be spoofed.
Only the last IP in the header is guaranteed to be non-spoofed assuming a valid reverse proxy is in place.

https://en.wikipedia.org/wiki/X-Forwarded-For : Format : 
_“where the value is a comma+space separated list of IP addresses, the left-most being the original client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed through proxy1, proxy2, and then proxy3 (not shown in the header). proxy3 appears as remote address of the request.
Since it is easy to forge an X-Forwarded-For field the given information should be used with care. 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. X-Forwarded-For data can be used in a forward or reverse proxy scenario.”_

**Problem-relevant `configuration.yaml` entries and (fill out even if it seems unimportant):**
```yaml
http:
  # Secrets are defined in the file secrets.yaml
  api_password: !secret http_password
  use_x_forwarded_for: True
  trusted_networks:
    - 127.0.0.1
```

**Traceback (if applicable):**
```

```

**Additional information:**
Work around: Disable trusted_network setting, remove reverse proxy or limit network access to authorized individuals only.

Problem first noticed first by https://community.home-assistant.io/u/NightRanger . Reference: https://community.home-assistant.io/t/trusted-networks-and-x-forwarded-for/20850 

I would issue a PR but I don’t have the right environment to test this. Hopefully somebody who uses this setup can apply the fix and test it.



Hints:
This used to work (last known working version 0.62)...but after upgrading recently and checking it in the latest dev version I can confirm that it is an issue
The thing is that I didnt even hacked the headers and still no password is asked when connecting from external network 
@rofrantz if you are not spoofing the headers then I believe you are having a different issue.
Do you use a reverse proxy? What is your "http" HA configuration?
I use nginx as reverse proxy
My http config uses api password and trusted network
Do you have ``use_x_forwarded_for: True`` in your config?
```
api_password: [redacted]
server_port: [redacted]
base_url: [redacted]
ssl_certificate: [redacted]
ssl_key: [redacted]
cors_allowed_origins:
  - https://google.com
  - https://home-assistant.io
use_x_forwarded_for: True
trusted_networks:
  - 127.0.0.1
  - ::1
  - 192.168.4.0/24
  - 2001:DB8:ABCD::/48
ip_ban_enabled: True
login_attempts_threshold: 3
```
@rofrantz

I made several tests in the past with different configurations, Here are the results:

Test 1:

use_x_forwarded_for: True
Using reverse proxy - Bypassed Login
Without reverse proxy - Bypassed Login

Test2:
use_x_forwarded_for: False
Using reverse proxy - Bypassed Login
Without reverse proxy - No bypass

Test3:
No use_x_forwarded_for in configuration file at all
Using reverse proxy - Bypassed Login
Without reverse proxy - No bypass


I would avoid using the trusted networks configuration when exposing HA to the internet (specially when using reverse proxy)
But it used to work and I had the same network setup :((((
Only HA changed
**Proposed solution:**

In https://github.com/home-assistant/home-assistant/blob/master/homeassistant/components/http/real_ip.py#L22 , change

``request.headers.get(X_FORWARDED_FOR).split(',')[0])``

 to

``request.headers.get(X_FORWARDED_FOR).split(',').reverse()[0])``

A more complete solution would be to allow the user to specify, in the configuration, which index (from the end) in the X-Forwarded-For header to use. This will also allow cases where there are two or more reverse proxies inline. Such a use case requires deep understanding of reverse proxies.
What's the difference between Test2 and Test3? The are booth the same as `use_x_forwarded_for` is `False` by default.

At the moment I don't see how replacing the client's source IP with the IP address of the last proxy in the chain would increase the security. The requests are coming from the proxy anyway. Usually the proxy is in the trustworthy network environment but not the client if we are talking about external access from the outside. At first glance it looks like it would get even worse.

Also, the implication about using `use_x_forwarded_for` and `trusted_networks` are well documented already.


@fabaff  Where is it documented properly?

It looks like most people that use reverse proxy are not aware of this documentation, browsing some sample users configurations on the HA forums shows that this configuration is enabled and that they have no clue they are exposed to auth bypass.

Here's an example:

https://community.home-assistant.io/t/my-home-assistant-configuration-it-s-alive/52670/7

https://github.com/ntalekt/homeassistant/blob/6f5b927701c89c2be54bacbb294082f4abd726e3/configuration.yaml#L36

I can confirm this issue as well. The solution @ayavilevich suggested would work, but - as noted - will have some problems when multiple proxies are chained together (either by design or as part of an attack).
A more elaborate solution would be to use the first untrusted IP in the x-forwarded-for header if there are any, and otherwise keep the current behaviour.

As a workaround for the simplest setup with only one proxy, you can change your nginx config as below:
Replace all occurrences of
`proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;`
with
`proxy_set_header X-Forwarded-For $remote_addr;`
This will replace the header the client provides with only the ip that nginx sees instead of appending to the provided header.
> @fabaff Where is it documented properly?

Like [here](https://www.home-assistant.io/components/http/).

To reproduce it with no proxy:

```yaml
http:
  api_password: !secret http_password
  trusted_networks:
    - 192.168.100.0/24
  use_x_forwarded_for: false
```

```bash
$ curl http://192.168.0.1:8123/api/
401: Unauthorized
```

```yaml
http:
  api_password: !secret http_password
  trusted_networks:
    - 192.168.100.0/24
  use_x_forwarded_for: true
```

```bash
$ curl http://192.168.0.1:8123/api/
401: Unauthorized
$ curl --header "X-Forwarded-For: 192.168.0.1" http://192.168.0.242:8123/api/
401: Unauthorized
$ curl --header "X-Forwarded-For: 192.168.100.1" http://192.168.0.1:8123/api/
{"message": "API running."}
```
@fabaff I think you are missing the point, This is not documented properly, Properly is BIG BOLD RED WARNING MESSAGE  explaining the DANGER of using  trusted networks with reverse proxy and the risk.

"Therefore in a reverse proxy scenario, this option should be used with extreme care." or " You should only enable this in a trustworthy network environment" for standard users is not clear what is "extreme care" ? what is a "trustworthy network environment" ?

Not explaining the security risks PROPERLY to users in the age of "Cyber" is making them vulnerable targets.
Feel free to improve the formatting or wording. But bigger warnings usually bring nothing because people tend to ignore them anyway. 

A "regular/standard" user will never setup a reverse proxy. It's too much work and just enabling port forwarding is easier. For the advance users we need to assume that they know what they are doing and understand the description of the configuration entries. Till now this was the case as there were no complains about that.
> A "regular/standard" user will never setup a reverse proxy. It's too much work and just enabling port forwarding is easier. For the advance users we need to assume that they know what they are doing and understand the description of the configuration entries. Till now this was the case as there were no complains about that.

There's also the people who tend to find some guide or howto on the internet and adapt it to their needs until it works. Those are the people who don't dive into the official docs and read every word written about the options they are enabling.
I'm not saying that it's a good idea to do things this way, but I know for a fact that it happens. If there is a way to make software a bit more "fool-proof" to protect the users then I'm all for it, hence my PR.
In the majority of cases it will not change anything. In a normal reverse proxy setup with no spoofing going on it will still use the first ip in the header. In a trusted environment where all proxies are trusted (e.g. reverse proxy in your home network), it will still use the first ip in the header. Only when the first n addresses are trusted and there is also an untrusted proxy in the chain will it change the current behaviour.
The PR does not mitigate all the risks though, for example it is still possible for an attacker to trigger an ip ban on any untrusted ip if the ip banning function is enabled (this was already possible anyway), but at least the most common and dangerous pitfall in using a reverse proxy is mitigated.
@fabaff 

> At the moment I don't see how replacing the client's source IP with the IP address of the last proxy in the chain would increase the security. The requests are coming from the proxy anyway. Usually the proxy is in the trustworthy network environment but not the client if we are talking about external access from the outside. At first glance it looks like it would get even worse.

Happy to explain further. LMK if I am understanding your terminology correctly:
client's source IP: is this the IP of the actual browser client? Is this his private or public IP? Or is this the IP of the party that connects to HA? Which may be a proxy or a real browser client?
IP address of the last proxy: Is this the IP address of the last proxy server or the client IP address that the last proxy server reports?

The current implementation takes an IP address from the header which it assumes is coming from a trusted proxy however due to a bug it is not a trusted IP address and can be spoofed. Therefore the current implementation allows for authentication bypass (under some conditions described above). If you agree to this base understanding then it is clear that fixing this will increase the security.

I understand that there are warnings in the documentation, but the current situation prevents even a person who is an experienced network engineer from setting up a proper secure configuration. So no amount of knowledge or reading of the warnings will be an alternative to fixing this bug if a person wishes to use proper reverse proxy networking.
The warnings are there for other reasons, one of which is to prevent people who don't have a proxy from enabling this configuration as it will also create a securty problem for them.

I don't see how it would get worse. Please provide a use case so we can discuss. If needed I can provide use case for the current state and suggested solution.
Wow, this is a nasty flaw.  The current implementation of `setup_real_ip()` blindly trusts the `X-Forwarded-For` header regardless of who sent it.  As a result, the following are possible:

 - Gaining access (without knowing the password) by guessing a valid `trusted_network` IP and sending that in the header
 - Brute-forcing the `api_password` with unlimited attempts, even if `ip_ban_enabled` is on (since the attacker can spoof/change the IP which gets banned)
 - DoS attack to block user access by sending multiple bad password guesses using a spoofed IP of a legit user

It is **extremely important** that the `X-Forwarded-For` header is **only* used if the connection is coming from a whitelisted IP of a known/trusted upstream proxy.

I'll create a pull request with a potential implementation to resolve this issue, along with a PR to the docs explaining the change.

Patch:
diff --git a/homeassistant/components/http/__init__.py b/homeassistant/components/http/__init__.py
index d8c877e83a205..f769d2bc4ffba 100644
--- a/homeassistant/components/http/__init__.py
+++ b/homeassistant/components/http/__init__.py
@@ -180,7 +180,7 @@ def __init__(self, hass, api_password,
             middlewares=[staticresource_middleware])
 
         # This order matters
-        setup_real_ip(app, use_x_forwarded_for)
+        setup_real_ip(app, use_x_forwarded_for, trusted_networks)
 
         if is_ban_enabled:
             setup_bans(hass, app, login_threshold)
diff --git a/homeassistant/components/http/real_ip.py b/homeassistant/components/http/real_ip.py
index c394016a683c4..401a09dc3066f 100644
--- a/homeassistant/components/http/real_ip.py
+++ b/homeassistant/components/http/real_ip.py
@@ -11,18 +11,22 @@
 
 
 @callback
-def setup_real_ip(app, use_x_forwarded_for):
+def setup_real_ip(app, use_x_forwarded_for, trusted_networks):
     """Create IP Ban middleware for the app."""
     @middleware
     async def real_ip_middleware(request, handler):
         """Real IP middleware."""
+        connected_ip = ip_address(
+            request.transport.get_extra_info('peername')[0])
+        request[KEY_REAL_IP] = connected_ip
+
+        # Only use the XFF header if enabled, present, and from a trusted proxy
         if (use_x_forwarded_for and
-                X_FORWARDED_FOR in request.headers):
+                X_FORWARDED_FOR in request.headers and
+                any(connected_ip in trusted_network
+                    for trusted_network in trusted_networks)):
             request[KEY_REAL_IP] = ip_address(
                 request.headers.get(X_FORWARDED_FOR).split(',')[0])
-        else:
-            request[KEY_REAL_IP] = \
-                ip_address(request.transport.get_extra_info('peername')[0])
 
         return await handler(request)
 


Test patch:
diff --git a/tests/components/http/test_auth.py b/tests/components/http/test_auth.py
index a44d17d513db9..dd8b2cd35c46c 100644
--- a/tests/components/http/test_auth.py
+++ b/tests/components/http/test_auth.py
@@ -41,7 +41,7 @@ def app():
     """Fixture to setup a web.Application."""
     app = web.Application()
     app.router.add_get('/', mock_handler)
-    setup_real_ip(app, False)
+    setup_real_ip(app, False, [])
     return app
 
 
diff --git a/tests/components/http/test_real_ip.py b/tests/components/http/test_real_ip.py
index 61846eb94c242..b6af81592078f 100644
--- a/tests/components/http/test_real_ip.py
+++ b/tests/components/http/test_real_ip.py
@@ -1,6 +1,7 @@
 """Test real IP middleware."""
 from aiohttp import web
 from aiohttp.hdrs import X_FORWARDED_FOR
+from ipaddress import ip_network
 
 from homeassistant.components.http.real_ip import setup_real_ip
 from homeassistant.components.http.const import KEY_REAL_IP
@@ -15,7 +16,7 @@ async def test_ignore_x_forwarded_for(aiohttp_client):
     """Test that we get the IP from the transport."""
     app = web.Application()
     app.router.add_get('/', mock_handler)
-    setup_real_ip(app, False)
+    setup_real_ip(app, False, [])
 
     mock_api_client = await aiohttp_client(app)
 
@@ -27,11 +28,27 @@ async def test_ignore_x_forwarded_for(aiohttp_client):
     assert text != '255.255.255.255'
 
 
-async def test_use_x_forwarded_for(aiohttp_client):
+async def test_use_x_forwarded_for_without_trusted_proxy(aiohttp_client):
     """Test that we get the IP from the transport."""
     app = web.Application()
     app.router.add_get('/', mock_handler)
-    setup_real_ip(app, True)
+    setup_real_ip(app, True, [])
+
+    mock_api_client = await aiohttp_client(app)
+
+    resp = await mock_api_client.get('/', headers={
+        X_FORWARDED_FOR: '255.255.255.255'
+    })
+    assert resp.status == 200
+    text = await resp.text()
+    assert text != '255.255.255.255'
+
+
+async def test_use_x_forwarded_for_with_trusted_proxy(aiohttp_client):
+    """Test that we get the IP from the transport."""
+    app = web.Application()
+    app.router.add_get('/', mock_handler)
+    setup_real_ip(app, True, [ip_network('127.0.0.1')])
 
     mock_api_client = await aiohttp_client(app)