X-Forwarded-For header chaining IP addresses preventing successful name resolving.
jondycz opened this issue ยท 8 comments
Issue Description: X-Forwarded-For header should only save the first IP address when resolving username. Since the header can be tempered with by the client, there is no security downside to doing this. Trusted proxies should be checked by the IP address of the incoming packet and the X-Forwarded-For header should be truncated to the first IP address when the origin IP address is a trusted proxy. dynmap/DynmapCore/src/main/java/org/dynmap/servlet/SendMessageServlet.java:100 - I suggest simplifying the function and get rid of the loops. return first index right away. If there arent any checks for IP address of the packet origin, i suggest checking for that too.
- Dynmap Version: core=3.3.1-683, plugin=3.3.1-683
- Server Version: git-Paper-148 (MC: 1.18.1)
[x] I have looked at all other issues and this is not a duplicate
[x] I have been able to replicate this
Indeed but isn't it still unnecessary? Dynmap doesn't care about proxies in the chain. If my home or school has a proxy, which isn't in the allowed proxy list, then instead of my name, i will appear as a string of IP addresses. Network misconfigurations happen all the time and you can never trust the client with the information they are sending you. That's why I'm proposing that the chain checking is scrapped and instead dynmap checks the "exit" proxy only and uses the first IP address for looking up the player name.
With my proposal, since the "exit" proxy is always in the allowed proxy list, the header can not be modified by the user, meaning you can trust the header. Any other appended proxies in the list can be either set by the ISP, organization or the user itself so there really isn't a benefit to checking the whole chain. Instead it opens up for user errors, since, as i said before, you can never trust the user. And the only TRUE information that can not be tampered with is the origin IP address (the exit proxy) and the client IP
The logic checks the remote address (IP source) as a proxy: the other proxies in the X-Forwarded-For header, per spec, are inserted by other proxies as they are passed though (in the event of more than one level of proxy). The X-Forwarded-For is ignored if the remote address is not a trusted proxy, so the concern of the 'client just hacking it' is a non issue - that's the point of trusted proxy checking.
These other fields are supplied and validated by the intermediate proxies (e.g. proxy A will not accept claim of an address from an untrusted external source at all; it will pass on a result with X-Forwarded-For embedded and including the remote IP (and, depending on the proxy, its own address appended to the chain); proxy B, behind proxy A, will validate both the remote address of proxy A, and will pass on X-Forwarded-For with the original IP remote from A (and, sometimes, its IP appended to the chain). Nothing is incorrect or improper about validating that the proxies in the chain are themselves valid - you're only seeing the X-Forwarded-For and considering it at all because the latest remote address matches a trusted proxy. Not seeing how this has any negative impact on lookup, except in that it requires the trusted proxy address list to include the other proxies in the chain.
No - you're right: the proxy validation should be a trust chain: each proxy is responsible for proxy trust relative to the proxy before it. I'll alter the code to only check the validity of the remote address of the immediate proxy, and to ignore the addresses on the X-Forwarded-For chain beyond the first one.
Also, I'm adding the support for the tristed-proxies to be defined using subnet notation (e.g. 192.168.1.0/24).
A 3.4 SNAPSHOT is running for these changes right now, so please feel free to verify it!
https://dynmap.us/builds/dynmap (build 750 or later)
Thanks! Both these code changes help me a lot! Thanks for your time you put into the project! Have a nice day.
Actually hold on, we both might have been wrong.
Lets imagine this scenario: I run a browser extension that can create X-Forwarded-For header. That header gets passed through an actual proxy. What that proxy does is keep the chain as it is, since that proxy isn't configured to trust the proxy the packet came from. So you actually can modify the IP to whatever you like. I couldn't exploit this though cloudflare as it seems like it detected that i modified the value, but other proxies might not be so smart.
So on second thought, what you should do is pick the FIRST IP that appears AFTER the last trusted proxy. Meaning if the chain is 8.8.8.8, 89.58.48.65. TRU.STED.PROX.Y1 TRU.STED.PROX.Y2 then dynmap should take 89.58.48.65 as the client IP as 8.8.8.8 can be tampered value. With this setting, you are giving better odds at resolving the username compared to the old method of showing a chain of IPs.
so if you don't mind doing one more small modification, I would be grateful. Sorry for confusing you.
So easiest way in my opinion is reverting the old code and exiting out of the loop when you encounter the first UNTRUSTED proxy (when checking from the end to the start) and use that as the user IP as anything beyond that is irrelevant.
The standard says that the first address is the remote address, and proxies can be chained after that (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For ). The ones further in the list are NEVER the source address - only a breadcrumb trail of proxies passed through on the way to the current receiver (the server). If your proxy is accepting X-Forwarded-From from a source that is not a trusted proxy (relative to THAT proxy), your proxy is unsecure/defective and/or incorrectly configured: proxies are responsible for validating X-Forwarded-For as much as any server might be.
I am also thinking of adding support for the more recent 'formally standard' header, 'Forward' - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded
Ironically, while this is a formal standard, the vast majority of proxies still don't use it. It's not better, per se, but it does seem like it'd be good to support.