Proxy headers don't work properly

[Arantor]

Description

Proxy detection doesn't actually work properly

Steps to reproduce

1. Have a proxy
2. Observe the IP address isn't respected properly

Environment (complete as necessary)

• Version/Git revision: 2.1.3
• Database Type: n/a
• Database Version: n/a
• PHP Version: n/a

Additional information/references

Bug report: https://www.simplemachines.org/community/index.php?topic=585020.0 Related PR: https://github.com/SimpleMachines/SMF/pull/7368

The issue is that $_SERVER['REMOTE_ADDR'] isn't actually updated correctly to be used (something the PR sort of handles, but incorrectly)

The intent should be that the real IP as per the proxy headers gets dumped into $_SERVER['REMOTE_ADDR'] (and thus into $user_info['ip']), with the next IP up the chain (if present, that isn't the proxy server) into $_SERVER['BAN_CHECK_IP'] (and thus into $user_info['ip2']) as the fallback ban check so that proxies that aren't expected can be banned if necessary.

[LexArma]

To try and visualize the issue. PHPInfo: SMF User Profile:

[jdarwood007]

https://www.rfc-editor.org/rfc/rfc7239.html https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

[Arantor]

You don't need to explain to me what the header is intended for, that's not the problem here, nor am I unfamiliar with the point of the headers.

The rest of SMF relies on REMOTE_ADDR being correct as the primary IP stored in posts etc. - if you're telling SMF 'hey there's a proxy, and this is where to get the real IP address', that's the address that should be flagged in posts and in the user's profile.

SMF is capable of detecting this - but it's then not using it. Which means the proxy server detection is pointless if you're not actually using the (correct) value you get out of it!

[LexArma]

I was commenting, and I assume Jeremy as well, only to add some background to the description based on earlier discussions on the topic now that we've seen this is actually an issue. For those who might not be as familiar with the headers and their usage and might want to investigate.

[Arantor]

I'm sorry, that's not how it came across to me, it came across as 'you are interpreting this wrong, the headers have these meanings, go away and read these docs'.

[sbulen]

jdarwood007 was answering a question I had raised elsewhere, and leaving it here for my benefit.

[sbulen]

I've been looking into this, & it looks like the following is pretty consistent: $_SERVER['REMOTE_ADDR'] = $user_info['ip'] = member_ip (on the member record) = the proxy $_SERVER['BAN_CHECK_IP'] = $user_info['ip2'] = member_ip2 (on the member record) = the user IP

My suspicion is that where we are seeing the "wrong" IP, we are using member_ip or $user_info['ip']. But that's the proxy.

My current thinking is that QueryString.php is working well, that's not the problem. It's just that folks have intuitively been using member_ip/$user_info['ip'] where if we want the actual user IP we should be using member_ip2/$user_info['ip2'].

So... What's the fix? I see three options:

• identify all the places where it's wrong (profile, error log) & cut over to displaying/storing ip2
• swap the loading of ip and ip2 in Load.php, so ip has the user & ip2 has the proxy
• abandon the use of ip2 altogether & just use the real client ip everywhere

The root problem here is that "ip" and "ip2" were terrible names...

[jdarwood007]

Yea my information was to reference the spec as it seems we may be applying logic incorrectly according to what the spec says. The spec says we need to pop the first address off as the client address. Any additional addresses are additional proxies. So we first need to handle splitting any possible additional addresses before parsing them as a valid IP. Along with validating that the proxy IP (which is in the current REMOTE_ADDR) is valid.

SMF comes from the old days where overwriting globals is ok. In reality, we should not touch REMOTE_ADDR, but only reference and use $user_info['ip']/['ip2'] checks everywhere. I Would say for beyond patching this, we should make user $user_info['ip'] is the users ip, even when parsed from the additional headers, rename $user_info['ip2'] to $user_info['ip_proxy'] and $user_info['ip_proxy'] should contain the IP we originally obtained from REMOTE_ADDR prior to parsing. Which would clarify things and clean it up as we expect it.

[Arantor]

$user_info['ip'] is always expected to be the user's real IP - everything expects that, $user_info['ip2'] is expected to be the same as ip in most cases, but on cases of a proxy we didn't know about, it can go there.

To me the answer is a little more involved, but I'm thinking about the rationale of ip2 in this.

1. $user_info['ip'] should be the real user IP however it gets populated. Yes that means popping IPs off the proxy stack, but that's not dramatic - most of the work is already done, we're just not getting it over the line.
2. $user_info['ip2'] should be either the real user IP or the first proxy that we don't immediately recognise in case of people using VPNs to bypass bans, that's what the original intent was and I think this is still valid. The caveat is that people can inadvertently ban their CDN - so, in my mind the correct behaviour is: if the admin puts IP range(s) into config, those aren't candidates for ip2/BAN_CHECK_IP, those are ones we know are friendly and we skip them as targets - if there's another proxy after the CDN, that's a candidate for ip2 because it's likely a proxy on the other side of the CDN. If they haven't put something in, we should take the logical answer of 'whatever the next proxy is, is the ban check IP', or if they have put something in, that's an exclusion. The alternative is 'if the user hasn't put in config for reverse proxy IPs, don't make any assumptions' and just reuse the user's real primary IP for their secondary IP.

Of course if somoene has better stats on the uses of VPN/VPN-like behaviour as a secondary proxy, would love to see them.

[sbulen]

The core logic hasn't really been altered in quite a while. I've compared the behavior across 2.0 & 2.1, and they are similar, but not the same...

This is true for both 2.0 & 2.1: $_SERVER['REMOTE_ADDR'] = $user_info['ip'] = member_ip (on the member record) = the proxy $_SERVER['BAN_CHECK_IP'] = $user_info['ip2'] = member_ip2 (on the member record) = the user IP

And in fact, in most places, it is member_ip displayed (the proxy one!), even in 2.0, just like 2.1.

ALSO... If given: $_SERVER['HTTP_X_FORWARDED_FOR'] = '8.8.8.8, 9.9.9.9, 10.10.10.10';

In both 2.0 & 2.1, $user_info['ip2'] & $_SERVER['BAN_CHECK_IP'] - become 10.10.10.10

Jeremy's point above is that the docs say it should be 8.8.8.8.

[Arantor]

I'd give you that 2.1 should have changed this to meet the RFC, but I don't know if whatever RFC that one replaces supported multiple proxies; that's a 2014 RFC which is after 2.0's go-live date...

Yeah, it's not been right for a while but it wasn't ever a huge problem because CloudFlare published notes on how to munge the CF IP address into $_SERVER['REMOTE_ADDR'] which for everyone using it back in the day was 'good enough'.

[JetzeMellema]

For forums that do not use CloudFlare or do not manage their reverse proxy this issue is actually a big deal. Not only does this impact rate limiting (all users now posting from the same IP address) but also means any admin accidentally including the IP address in a ban would lock everyone out of the forum.

Appreciate the work on this issue.

[JetzeMellema]

Can confirm the issue is still present in 2.1.4.

[jdarwood007]

This was not addressed in 2.1.4, which is why it is still open.

As mentioned the core logic here hasn't really changed in regards to w $user_info['ip'] and $user_info['ip2']. How we get to those has. I personally thing it is wrong and we need as Arantor said for $user_info['ip'] to have their real IP (the original non proxied one, while $user_info['ip2'] should have the IP of the proxy (if one). Such change like that in 2.1 may not be possible as it changes a core functionality of the system. So to fix this for 2.1, we need something simple. A proper fix can be addressed in the future.

[Sesquipedalian]

If I am reading both the code and the discussion here correctly:

1. Our current logic populates $_SERVER['BAN_CHECK_IP'] with the user's real IP (meaning, the IP whose connection was forwarded through the proxy), whereas $_SERVER['REMOTE_ADDR'] contains the IP that directly connected to our server (meaning, the proxy's IP when a proxy is in use).
2. Our current logic populates $user_info['ip'] with $_SERVER['REMOTE_ADDR'] and $user_info['ip2'] with $_SERVER['BAN_CHECK_IP'].
3. But code elsewhere consistently assumes that $user_info['ip'] is the user's real IP.

It would seem like the simple solution would just be to populate $user_info['ip'] with $_SERVER['BAN_CHECK_IP'] and $user_info['ip2'] with $_SERVER['REMOTE_ADDR'].

Since everyone else in this discussion believes that a viable solution needs to be more complex than that, I assume that I am missing something here. Can someone enlighten me?