Skip to content

Improve handling of x-forwarded-for header (fixes #14345)#14379

Closed
mvn23 wants to merge 2 commits intohome-assistant:devfrom
mvn23:mvn23-x-forwarded-for
Closed

Improve handling of x-forwarded-for header (fixes #14345)#14379
mvn23 wants to merge 2 commits intohome-assistant:devfrom
mvn23:mvn23-x-forwarded-for

Conversation

@mvn23
Copy link
Copy Markdown
Contributor

@mvn23 mvn23 commented May 10, 2018

Improved handling of x-forwarded-for header to prevent spoofing attacks when using use_x_forwarded_for and trusted_networks.
Updated tests to reflect changes in the http component.

Description:

This PR scans the x-forwarded-for header for the first IP address which is not in one of the trusted_networks ranges and uses that as the external IP. If no untrusted IP is found it reverts to the old behaviour (first IP in the x-forwarded-for header)
This will prevent a spoofing attack as described in #14345, provided the reverse proxy is configured correctly.

Related issue (if applicable): fixes #14345

Checklist:

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

If the code does not interact with devices:

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

…ks when using use_x_forwarded_for and trusted_networks.

Updated tests to reflect changes in the http component.
@mvn23 mvn23 requested a review from a team as a code owner May 10, 2018 18:13
@ayavilevich
Copy link
Copy Markdown

@mvn23 I understand your approach of looking for "non trusted networks" IPs as a solution to this issue. This has the advantage of supporting more than one reverse proxy in a chain without an additional configuration field.

I do have some feedback:

You need to take account for ban control as well as for authentication.
The IPs on the left of the xff header are spoof-able (which allows for the bug in the first place).
The way it is now, "A" can spoof some random IP and make you think communications are from "X" and not from "A". If A changes "X" every request then it can attempt bruteforce and IP rate control might not work. So there won't be a password bypass but there will be something else.
You should look for non-trusted IPs from right to left rather than from left to right. This way you will try to validate the "real" IPs first. "Real" being IPs reported by a trusted party (closer to HA in the chain).

Also, it seems that for multi proxy configurations, the IP of the proxy must be in the trusted IPs list so that the proper external IP is detected. This in turn means that anyone with access to the proxy can connect without a password to the HA UI. This is not a biggie, but you should mention this in the documentation.

@mvn23
Copy link
Copy Markdown
Contributor Author

mvn23 commented May 15, 2018

The way it is now, "A" can spoof some random IP and make you think communications are from "X" and not from "A". If A changes "X" every request then it can attempt bruteforce and IP rate control might not work.

This is what I mentioned in #14345 but it is not so easy to prevent without unwanted side effects. If we start scanning the addresses from the right, all expected proxies MUST be in the trusted_networks list or they will be banned if an attack takes place. This is not always what you want.

Also, it seems that for multi proxy configurations, the IP of the proxy must be in the trusted IPs list so that the proper external IP is detected.

Not true, as long as the first proxy in the chain adds it's client's IP to the header as it should (this will then be the first untrusted IP from the left).

The reason I made this patch is simply to provide a first line of protection for users against passwordless login with header spoofing. If there is a need to get the actual address of the client connecting to the first proxy in the chain - e.g. for IP banning - then the only reliable way to do this is in the proxy configuration.

@ayavilevich
Copy link
Copy Markdown

If we start scanning the addresses from the right, all expected proxies MUST be in the trusted_networks list or they will be banned if an attack takes place. This is not always what you want.

Requiring that your second+ proxies are in the trusted list is acceptable. Anyway it is better than breaking the banning feature.
Scanning from the right will only potentially require change for people with more than one proxy, which is rare. Breaking the banning will affect everybody with any proxy and make them less secure.

@mvn23
Copy link
Copy Markdown
Contributor Author

mvn23 commented May 17, 2018

Requiring that your second+ proxies are in the trusted list is acceptable. Anyway it is better than breaking the banning feature.
Scanning from the right will only potentially require change for people with more than one proxy, which is rare. Breaking the banning will affect everybody with any proxy and make them less secure.

The banning feature is already "broken" in this scenario. Making it mandatory to add all expected proxies to the trusted_networks list would make this a breaking change with hardly any advantages. It will not provide additional security and it is already possible to fix in the proxy config.
If you want to fix the banning feature in this scenario then that should be another issue with another PR. This PR was made to protect against inadvertently opening your home assistant install to the world, which it does. As it is now it is not a breaking change, but it does provide additional security to people who are unaware of this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security issue, unauthorized trusted access by spoofing x-forwarded-for header

3 participants