A tiny little ban function

[vaaaska]

I was asked to write a little ban function for a project. Easy enough, I guess. But, I needed to do this so that the user could enter in any kind of an IP number...either in full or just part.

Like:

123.123.123.123
123.123

So, I hacked up a set of functions to do this. It seems too simple though. And so, I thought I'd run it through here out of curiousity.

Ok, here we go... ;)


function doStatsStuff() {

// gets the visitor IP number
$theip = $_SERVER['REMOTE_ADDR'];

// this is just an array with IP numbers as they
// come from the database...
$arr = getIPs();

// this is really what i'm curious about
// is this just too simple?
foreach ($arr as $ip) {

// counts the number of chars in the string
$count_ip = strlen($ip);

// compares db ip to visitor ip but only with
// the correct number of chars
if ($ip == (substr($theip, 0, $count_ip))) {

// if the ip matches
$x = 'equal';

// if it doesn't match do nothing
} else { }
}

// if anything matched then
if ($x == 'equal') {

// do whatever...banned...

}

// send visitor to the page...
}

[Shrike]

There is the obvious problem of clients not forwarding the client IP address - perhaps you should check that the string lengh is greater than 0 before going any further (and maybe generate an error saying IP address must be visible?).

[laserlight]

Couldnt you just run a single query such as:
SELECT COUNT(ip) as ip_matches FROM ipaddrs WHERE ip LIKE '$theip%';
then allow the user if ip_matches = 0?

[mrhappiness]

even if $_SERVER['REMOTE_ADDR'] is not empty it may be the ip adress of the proxy server of our university

if 1,000 people surf they will all have the very same ip-adress and if 1 of them acts in a bad way and you ban him, the 999 other people are banned too...

check $_SERVER['HTTP_FORWARED_FOR'] as well


--
i know this field is not existant in all cases, but if it exists, you should use it

[laserlight]

if 1,000 people surf they will all have the very same ip-adress and if 1 of them acts in a bad way and you ban him, the 999 other people are banned too...
That's an inherent problem in IP address based banning, unfortunately.

[vaaaska]

Originally posted by laserlight
Couldnt you just run a single query such as:
SELECT COUNT(ip) as ip_matches FROM ipaddrs WHERE ip LIKE '$theip%';
then allow the user if ip_matches = 0?

Ah...this is what I've been trying to come up with I believe...never thought to use COUNT() this way.

However, I can't quite figure out how the ip_matches would work.

if 123.123 LIKE 123.123.123.123
if 123 LIKE 123.123.123.123

If it were possible to reverse them around and...do what I had done above...in the query itself it would be possible.

It won't ever work...this is where the problem came into play.

Yes, there are inherent problems with banning an IP range, but this is something that they will have to manage on their own - they've been warned of the issues.

[laserlight]

However, I can't quite figure out how the ip_matches would work.
hmm... yeah, I didnt take into account that we're looking for a general case from a specific case, instead of the other way round.

We could split the IP address into 4 octets, but then for IP address a.b.c.d the query would be like:
SELECT COUNT(*) FROM ipaddrs
WHERE octet0=a AND octet1 IS NULL AND octet2 IS NULL AND octet3 IS NULL
OR octet0=a AND octet1=b AND octet2 IS NULL AND octet3 IS NULL
OR octet0=a AND octet1=b AND octet2=c AND octet3 IS NULL
OR octet0=a AND octet1=b AND octet2=c AND octet3=d;
Plus you have to explode() the IP address first.

[pohopo]

You could try this...

SELECT COUNT(ip) as ip_matches FROM ipaddrs WHERE LEFT(ip,'strlen($theip)') = '$theip';
then allow the user if ip_matches = 0?

So if you ban 123.345 then anything that starts with that will be banned. As people said banning by IP is a bad idea. Not only can it ban people you like, but it is extremely easy to change your IP. If anything trying to ban an IP only makes things worse.

[vaaaska]

For this to work we would need to have complete octets (we still can't use LIKE) - your solution is still searching for exact matches. Certainly that is possible, but I'm trying to dumb the thing down for the users (yes, I know that only a person who knows what this is all about should be dealing with it).

But, back to my original question...

I'm doing this with only one query (same as you), so why is the approach I'm using above not good? I don't believe it's any more foolproof than anything else (because we all know how easy it is to spoof an IP address).

One caveat to my fast method is that we have to iterate through all the banned IP's to determine the banned status - but it's still not a second query.

[laserlight]

I'm doing this with only one query (same as you), so why is the approach I'm using above not good?
It's okay, but we're trying to improve it.
The idea is to try and move as much logic that pertains to obtaining the data into the query.

[pohopo]

Sorry, I did my query backwards, it should be..

SELECT COUNT(ip) as ip_matches FROM ipaddrs WHERE ip = LEFT('$theip',LENGTH(ip));

So if in your table you have 123.765 then any full address that starts with 123.765 will be counted, thus banned.

This is the shortest, and easiest, way I can think of and does not use like, nor requires an exact match of the full IP.

And like laserlight is saying, just trying to put as much as possible in the query so the database does the work.

[vaaaska]

Sorry folks if I sounded offensive there...I'm really not. Just posing the question.

I posted this here to improve...your comments are VERY much appreciated.

SELECT COUNT(ip) as ip_matches FROM ipaddrs WHERE ip = LEFT('$theip',LENGTH(ip));

Wow...I think you got it. I've never been too handy with JOIN and such but I guess now is the time to do a little reading on the topic.

I'll give this a whirl tommorrow...

Actually, even though there really are a number of ways to do this, I think my question has been answered...was my approach viable? It was...even if it my solution was a little slow-brained I was on the right course. I just wasn't able to put together a smart enough query (but I did try to do so originally).

;)