Can this be exploited?

if ($ip == $_SERVER['REMOTE_ADDR'])

[brokaddr]

I imagine anytime a string is sanitized it uses a tiny bit of processing power. Why use it if it's not necessary?

Hence my question, can this be exploited; or can only database inputs be exploited? (Just trying to be extra-cautious!)

//check the ip
if ($ip !== $_SERVER['REMOTE_ADDR']){
header('login.php');
exit();
}

($ip is already sanitized and stored in the database, the comparison operator $_SERVER['REMOTE_ADDR'] is not sanitized and is not stored in the database; nor is it passed to the database.)

[Matthew1980]

Hi there brokaddr,

When you say that $ip is already sanitised, how do you mean? What have you done to it to 'make it safe' I guess that you can separate everything using the '.' and then ensuring that everything between is numerical, then stitch it back together.

You could run the same function over $_SERVER['REMOTE_ADDR'] assuming that it's set and not forgetting proxies either, you can't just assume that you being on just one IP address because from behind proxies your address is added to the previous.

I generally use this:-

function get_ip()
{
$ip_address = array();

if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && strpos($_SERVER['HTTP_X_FORWARDED_FOR'],','))
{
$ip_address += explode(',',$_SERVER['HTTP_X_FORWARDED_FOR']);
}
elseif (isset($_SERVER['HTTP_X_FORWARDED_FOR']))
{
$ip_address[] = $_SERVER['HTTP_X_FORWARDED_FOR'];
}
$ip_address[] = $_SERVER['REMOTE_ADDR'];

return $ip_address;
}

Note that $_SERVER['HTTP_X_FORWARDED_FOR'] can only be used IF it is set (from a proxy) and is supported.

Then is this is the case, this little function will add these IP's together so you can check multiples - though I have coded for it to happen, I don't think I have ever seen it logged in my records...

Hope that makes sense.

Cheers,
MRb

[brokaddr]

Sanitization:

$ip = isset($_SERVER['REMOTE_ADDR']) ? preg_replace("/[^.:()a-zA-Z0-9\/]/", "", $_SERVER['REMOTE_ADDR']) : '';

The : and a-z are present in preparation for IPv6.
Which is more efficient for blocking proxies? Htaccess or your php code?
[perishablepress.com...]

You could run the same function over $_SERVER['REMOTE_ADDR'] assuming that it's set and not forgetting proxies either

Is sanitizing such data important, though? Even if it never touches a form or database and is only used for comparison?

[SteveWh]

Aside from whatever method of sanitization you use (and the IPv6 aspect is beyond my experience), the amount of processing required for the test in your original post is so utterly trivial that I don't think it should even be taken into consideration. If that code (or a replacement code snippet, whatever you decide on) helps improve security for an obvious or non-obvious reason, I'd say use it and don't give it another thought.

The code snippet in your original post is not exploitable. Whether your subsequent use of the ip is exploitable depends on what you do with it. Basically, if you only test it against another string, and if the text of the user-sent string is never actually used in a db query or as the name of an include file, echoed in a form field or other part of a page, or actually used as-is in some other way, it's not exploitable.

----

As a separate concern, though, I'd be more inclined to use preg_match for the sanitization (which turns it into a validation rather than a sanitization). What you really want to know is whether the incoming value IS valid. If the user-provided value is not valid from the outset, their bad intent is clear. There's no point trying to turn it into a valid IP address by preg_replacing its chars. That will just create a technically sanitized string that isn't really their IP address, or anyone's, most likely.

If the preg_match for a valid IP address fails, I'd just reject the request.

[brokaddr]

Thanks for your explanation, SteveWh.

Regarding preg_match, how could I work that into my original code:
$ip = isset($_SERVER['REMOTE_ADDR']) ? preg_replace("/[^.:()a-zA-Z0-9\/]/", "", $_SERVER['REMOTE_ADDR']) : '';

I assume you do an if() and then some sort of exit() with an error, to stop processing of the script?

[sundaridevi]

Hence my question, can this be exploited; or can only database inputs be exploited? (Just trying to be extra-cautious!)

If you are following the convention that no input should be trusted, then you should definitely validate the IP and as stated above, reject the submission if it is not a valid IP. Otherwise, if you store the IP it is the real IP, unless of course as pointed out above, they are using a proxy, in which case you won't see the user's real IP.

So to check for malicious behavior, the real issue is what you want to allow and when you are checking.

If you are checking the IP on every page and logging them out, as some super secure sites do, if their IP changes; then you will need to decide when to kick them. My host provider for example will only log you out if the IP has changed "significantly". I suppose this could mean the top 3 blocks of the IP.

On the other hand if you just want to store their IP then with what you have above, what you are storing is going to be the IP used to access the referrer page to that php script.

If you want to know if something strange is going on, you could store a session variable and store the IP when the session is created, then compare the initial IP with the referrer page IP that you are about to write to the DB. If they are different then MAYBE something is up. But it is not necessarily malicious behavior

If the visitor is on a dial up connection, and they lost the connection, then when they return they will most likely have a different IP, but it should not be "signficantly" different. To help prevent a false positive in this situation you could check the first 3 blocks of the IP and if they are the same you might say, OK he's logging in from the same ISP.

If they are on aol, the IP can change, but probably not "significantly". In any event if you are looking at X-Forwarded-For this is a non-issue, because you will not see the AOL proxy, you will see the real user IP.

That .htaccess code will be a lot slower than a php script that does the same checks. In any event those checks won't detect any serious proxy user who is using a so-called elite proxy or a botnet.

If they are using an anonymous proxy that changes their IP every x seconds, then you will quite possibly see two different IPs with this situation and they would most likely be significantly different IPs.

If they are a sophisticated hacker using a botnet, then their IP could be static for the whole session, but next time they come back they could quite likely have a completely different IP.

[edited by: sundaridevi at 1:46 pm (utc) on Mar 7, 2011]

[SteveWh]

Keeping in mind that I don't understand the IPv6 aspects, I'd use something like this:

The regex means "consists entirely, from beginning to end, of 1 or more legal characters".

if(isset($_SERVER['REMOTE_ADDR']) && preg_match("/^[.:()a-zA-Z0-9\/]+$/", $_SERVER['REMOTE_ADDR']))
 $ip = $_SERVER['REMOTE_ADDR'];
else
 exit();

I've never had to abort processing in the middle of a script, so I'm unclear whether exit() is the best way to do it gracefully. If there's a better way, hopefully someone will mention it. What I'm concerned about is that the user might get a half-constructed page or something ugly like that. Or does exit() always just cause nothing to be output at all?

"1 or more legal characters" doesn't really define a valid IP address, although it's a step in the right direction.

For IPv4, the preg_match could be made more explicit to better detect a truly valid IP (nnn.nnn.nnn.nnn):

preg_match("/^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$/", $_SERVER['REMOTE_ADDR'])

which can be condensed to

preg_match("/^([0-9]{1,3}\.){3}[0-9]{1,3}$/", $_SERVER['REMOTE_ADDR'])

A similar method could probably be used for IPv6, but I don't know what the valid forms of IPv6 are.

[sundaridevi]

You can use the PHP5 function: FILTER_VALIDATE_IP to validate IP addresses in IPv4 or IPv6. It can also detect private IP addresses which could also indicate a proxy.