critique: user validation script.

sijis

I created a failry simple script that basically tries to restrict pages from unauthorized access. If the user isn't logged in or is trying to log in, it shows them a form, otherwise it shows them the page.

I just implemented an anti_sql_injection() function (based on some code in phpfreaks.com) and i wanted to know if this implementation does a good job of preventing sql injections.

I would appreciate any feedback.

(i also have older versions of this script located here: http://www.phpbuilder.com/snippet/detail.php?type=snippet&id=939)

Shrike

Nice too see so much validation put in...to the point that perhaps it's a little excessive...what if people use the keywords which you remove as their password?

Nice clean code and well documented 🙂

sijis

Thanks for the feedback and taking a look at it.

I've thought about giving the ability to override the default words that are being filtered so that it can be specific for a particular situation. On the other hand, how many people really have 'select' 'delete'
'update' 'insert' 'asc' 'desc' 'where' ... etc as their password. Not many.
The code searches for exact matches of those words not part of them.
so 'select123' would not match but 'select' would.

Shrike

Agreed, to an extent. To be honest though, all you really need to watch out for with SQL injections are quotes and semi-colons, select, update, insert don't mean anything to a DB out of context.

Having a list of valid characters is a nice way to ensure clean data before going to the database though.

Weedpacket

Of course, if you are encrypting your passwords before storing them, there's no need to check what exactly they are - only the encrypted version is put in the query. If you MD5 them first, then they're only ever 32-char chuncks of hex digits.

This means of course you have to use PHP's MD5() function rather than MySQL's (otherwise the query will be getting the unencrypted password). But that's okay: I've seen MySQL do strange and worrying things with text which cause its MD5 to return strange results. It's partly for that reason that I don't use MySQL 🙂

I'm also not a fan of "trying to guess what an attack might look like, and preventing it", because I might not guess every possible attack. Instead, I prefer to allow only input I know is safe: in other words, usernames, for example, should contain only valid characters, where I decide what is "valid", and "invalid" is everything else. This is instead of trying to remember every possible "invalid" character and then saying that usernames should not contain invalid characters.

Oh, and on a technical note in relation to your validateLogin function; if you want to see if there are records that match a query, instead of returning the entire record, it's more efficient to have a query that just COUNT()s them and returns the count. Replace "" (which alone should only be used as a last resort) with "COUNT()" and you'll get back one record with one field, containing what mysql_num_rows() is giving you now.

sijis

I'm bringing this back from the dead.

I was using this app from this project (videodb on sourceforge.net) and I noticed how they prevented sql injections with the following code:

...
// $password and $username are the fields from the form
$row = runSQL("SELECT passwd, id FROM users WHERE name='$username'");
if (md5($password) == $row[0]['passwd'])
{
.......

full code located:
http://cvs.sourceforge.net/viewcvs.py/videodb/videodb/login.php?rev=2.14&view=markup

since the query is only returning the password of the valid username and its checking it against the form's password, it seems really difficult to gain access trying an sql injection.
What do you think?

Weedpacket

That's not the bit where they're preventing SQL injection. The bit where they're preventing SQL injection is a little bit earlier, where they only allow $username to contain certain safe characters. That, after all, is the bit of user-supplied data that's being inserted into the query.

They also don't check that the value of $username is actually in the database. I haven't looked at RunSQL in detail, but I did notice that they run a pconnect() and close() for every single query (sorry: mysql_pconnect() etc.). Since they're using pconnect(), the close() is a no-op (but they can't use connect() because it would make for a whopper of a performance hit). They could store the db connection in a static variable to save repeated attempts to connect to an already-connected database. But then, they could do that with connect() as well.

Hmmm... first time I've seen "overhaul" spelled with a 't'....

But this is supposed to be a critique of your code, isn't it? 🙂