Incorrect (?) handling of X-Forwarded-For

[koying]

The problem

From #38696 , the trusted_proxies is used both to actually reject upstream proxies that are not trusted, and as a filter to determine the originator of the request, the first upstream proxy in the chain not being trusted being considered as the originator.

There doesn't seem to be an RFC defining X-Forwarded-For, but both https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For and https://en.wikipedia.org/wiki/X-Forwarded-For define it as

X-Forwarded-For: client, proxy1, proxy2

So the originator would always be the first IP in the list.

With the aforementioned PR, the actual originator IP is lost and "Unauthorized access" messages will be logged with the address of a proxy unless the full list of possible proxies that could be used before reaching the actual upstream proxy is added to trusted_proxy, which is both a hassle and a security risk, as you are supposed to add proxies to the list which are not actually trusted, to get the actual originator of a request.

This seems an "in-between" and confusing situation.
To add to confusion, the documentation for trusted_proxies says "List of trusted proxies, consisting of IP addresses or networks, that are allowed to set the X-Forwarded-For header", which is not the actual behaviour (only the peer is considered, afaict).

I see 2 ways forward:

• As mentionned in the PR, reject a request when any proxy in the X-Forwarded-For list is not trusted.
• Remove the code that overrides the originator with an intermediate "untrusted" proxy.

What is version of Home Assistant Core has the issue?

2021.6.6

What was the last working version of Home Assistant Core?

?

What type of installation are you running?

Home Assistant Container

Integration causing the issue

http

Link to integration documentation on our website

https://www.home-assistant.io/integrations/http/

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

[probot-home-assistant]

http documentation
http source
_{^{(message by IssueLinks)}}

[frenck]

I think you miss the fact that the list provided is processed in reverse.

[koying]

Thanks for your answer, but I don't see your point.
In https://github.com/home-assistant/core/blob/dev/homeassistant/components/http/forwarded.py#L125, unless I read wrong, you iterate the list, then override "remote" and break as soon as one of the proxies is not in the trusted list.

[frenck]

Correct, if one of the forwarders is untrusted, it means the chain cannot be trusted. For example, it could be a man in the middle.

[koying]

Fair enough, but it should be flat out rejected as a security issue, then.
Just overriding the originator, without any warnings, doesn't seem to make sense, if for security reasons.

If you have an actual MITM, with correct authentication, you just have no notice at all...

[frenck]

Just overriding the originator, without any warnings, doesn't seem to make sense, if for security reasons.

That is a different concern and not a forwarding concern. The client is the first untrusted resource, that is the remote last know, as everything behind it, is well.. fake? untrusted?

There is a strict method to this, which is rarely implemented (and we don't implement it either) which defines a strictness in the required/fix number of hops and even implementations that require the trusted proxies to be in order. However, these implementations are rare and considered out of scope for our project.

If your concern is blocking untrusted clients, that is not what this header handling is about.

[koying]

The client is the first untrusted resource

Now you lost me. As you reverse the order (# Process X-Forwarded-For from the right side (by reversing the list)) the client is actually the last untrusted one as far as your loop go...

If your concern is blocking untrusted clients, that is not what this header handling is about.

My concern is that the actual client IP is lost, and replaced by the first untrusted proxy, nothing else.

[frenck]

My concern is that the actual client IP is lost, and replaced by the first untrusted proxy, nothing else.

The last untrusted proxy IS the client. Anything behind it is untrusted, there is no client IP for that matter. The untrusted proxy is the only thing we know as a client IP.

[frenck]

ok, so let say:

client IP -> untrusted proxy IP -> trusted proxy IP

In this case, the untrusted proxy IP is our client, because, we don't know if the client IP is real.
For example:

Spoofed fake client IP -> untrusted proxy IP -> trusted proxy IP

This makes it more clear, we cannot use the original client IP. The untrusted proxy is the only IP we can trust as it was provided by our trusted proxy.

If you are saying we lost the initial client IP: Yes, we did lose it. We never got it from our application perspective, as it might not be real in the first place.

[koying]

Ok. That makes your approach clear. Thank you.

You confirm the request is blocked in case:

• use_x_forwarded_for is enabled
• There is a X-Forwarded-For
• The immediate upstream (peer) of the request is not in the trusted list

Else:

• The client IP is set to the last untrusted proxy in the list

I will try to clarify the doc regarding this.

[frenck]

Well, shorter: The client IP used by Home Assistant is always the first untrusted IP.
With one exception: If all IP provided are trusted, the last in the chain is used (request has its origin from a trusted proxy, which is actually weird, but possible).

Even if everything is set up correctly (all proxies trusted), we pick the client because it is untrusted :)

[koying]

Yeah, but there is still a distinction made between

• "immediate upstream proxy untrusted" -> rejected
• "another upstream proxy untrusted" -> pass with last untrusted as client

[frenck]

Yes, the first case can be rejected safely and protects against common misconfiguration.
The second can be misconfiguration, however, could be something else 🤷

[koying]

Thanks for the explanations. Will close this.

[frenck]

No problem at all! Thanks for validating/raising it 👍