Harald Ponce de Leon, on 03 December 2011 - 08:24 PM, said:
Hi Te..
Regarding the callback from payment modules, these also check against a secret signature so a request from a spoofed IP address is not going to get far.
Kind regards,
Sure, and there are 5 or 6 other callback scripts that use tep_get_ip_address() as well, some of which I have looked at.
The addon that immediately looked at risk to me was PHPIDS addon for which I
notified the developer.
The point of that issue though was that there is a false sense of security being employed by callback coders if they believe they can rely on HTTP_X_FORWARDED_FOR or HTTP_CLIENT_IP for self identification. More than likely though they are just depending on the function to provide them with an accurate ip address.
To an attacker it presents about 10 seconds worth of extra work to get around that first layer of security if ips are derived from the HTTP_X_FORWARDED_FOR or HTTP_CLIENT_IP without some other true test.
Harald Ponce de Leon, on 03 December 2011 - 08:34 PM, said:
Hi Te..
As there are valid reasons to check against proxies, an alternative method is to use the logic of v2.3.1 for general checks, and for more stricter checks when the origin of the request is known (eg, payment gateways), to bypass the proxies and use REMOTE_ADDR only.
Do you think that would be sufficient?
Kind regards,
The logic in 2.3.1 depends a lot on correct configuration of proxy servers ip string. The order of ips in the HTTP_X_FORWARDED_FOR for example is sometimes in reverse order.
Sometimes its client, proxy1, proxy2, and sometimes its proxy2, proxy1, client.
With the advent of 'cloud' serving, there is a growing assumption that HTTP_X_CLUSTER_CLIENT_IP = the clients ip, but as you can see from that rackspace information, that is not always the case (when HTTPS is on)...and again, HTTP_X_CLUSTER_CLIENT_IP can also be spoofed.
The reason I focussed on the HTTP_X_CLUSTER_CLIENT_IP in the example above is because I think in the near future the majority of osCommerce websites will be on distributed server clusters, because free and cheap webhosting is moving in that direction.
I didn't really intend on taking this much further code wise due to my own time constraints with other work on, but am hoping to get some debate going around the issue of hardening the getIPAddress() (some of which could also be applied to tep_get_ip_address() in any future updates to it in 2.4).
I can imagine the end result will still have its weaknesses but it would be a big step up from this type of approach which is common and also flawed:
if HTTP_X_FORWARDED_FOR is set and not empty then ip = validate( HTTP_X_FORWARDED_FOR )
if HTTP_CLIENT_IP is set and not empty then ip = validate( HTTP_CLIENT_IP )
if HTTP_X_CLUSTER_CLIENT_IP is set and not empty then ip = validate( HTTP_X_CLUSTER_CLIENT_IP )
if REMOTE_ADDR is set and not empty then ip = validate( REMOTE_ADDR )
return ( first ip in array)
The approach I think could be best would be:
1/ have a 'hardened' (or whatever other name) and normal switch option ex: function getIPAddress($hardened=false);
2/ use a function like validate_ipaddress() to check every ip in the HTTP headers including IPv6 formats for future proofing
3/ when the HTTP_X_CLUSTER_CLIENT_IP is present and HTTPS is not on, set the REMOTE_ADDR as the ip address (rather than the HTTP_X_CLUSTER_CLIENT_IP) except when HTTPS is on for which with hardened set to false, the untrustworthy HTTP_X_FORWARDED_FOR will have to be called upon to extract the clients ip (or proxy they are behind).
4/ for everything else, with hardened switched to false, use the logic from 2.3.1 tep_get_ip_address() minus the HTTP_X_CLUSTER_CLIENT_IP which would be already taken care of above.
5/ for gateways, they could register their proxy server ips (if they are behind them) in their callback code so that it can be verified via the REMOTE_ADDR. But the same rules would apply when the site is installed on a cluster/cloud therefore the REMOTE_ADDR would be accurate if HTTPS is off but the only other place you will find the client callback ip would be in the HTTP_X_FORWARDED_FOR if HTTPS is on (again going only by what is gleaned from
http://goo.gl/xfVls).
6/ with hardened on, any other method of accurately determining the client ip - registering the reverse proxies on install for example, failing that, REMOTE_ADDR for everything even if it reports wrong.
That would allow for some flexibility around clusters, which as mentioned, will be the norm, and therefore there is a higher need to get those right.
This issue with clusters and HTTPS could also explain why some current users are experiencing callback issues where their sites are hosted on clustered servers. Where HTTPS is on, tep_get_ip_address() would be reporting the wrong ip address of the client/call back server.
Taipo
0
