A quick favour - Hack My Registration Script! (PHP)

[Freakish_05]

Hi guys,

I was just wondering if someone had a few spare minutes to check over my 252 lines of registration script. Don't worry, its only 252 lines long because I over-organise my code lol

This is the first 'proper' PHP script I've written and I've tried to implement security etc as best I can. Really I'm looking for the answers to two questions:
1) Is this script secure? Are there any holes? If so, what are they and how should I plug them?
2) Is this a good method of coding. I realise that this is very much like OOP which most PHP users tend to stay away from. If there's a better way of doing things then please let me know

Without further ado Here is the offending script!

Thanks in advance,
Freakish_05

 

[Gaverick]

//Check that the username is less than 20 characters and more than 4 characters
if((strlen($_POST['username']) >= 4) && (strlen($_POST['username']) <= 20))

I have no clue about PHP at all, but I think that means less than 21 and more than 3 characters due to the equals sign. Not really a security flaw but something I spotted. I may be wrong though since I know nothing about PHP

 

[marc2003]

i think it's kind of pointless checking each field twice. first you're checking to make sure the strlen > 0 and then again to make sure it's meets your validation requirements. why not drop the first part and simply check it's > x and <y?

and this....

You should only have characters 'a-z' and 'A-Z' in your name!

not very nice is it?

 

[meghatronic]

i think it's kind of pointless checking each field twice. first you're checking to make sure the strlen > 0 and then again to make sure it's meets your validation requirements. why not drop the first part and simply check it's > x and <y?

and this....



not very nice is it?

C3PO is crying...

 

[Dfhaii]

//Check that the username is unique
$username_query = mysql_query("SELECT * FROM users WHERE username = '$_POST[username]'");

Make the variables safe before using them in a query (this occurs later in the script too). Also you use a random salt each time and save it, this seems a bit :|, may as well choose a salt and use that throughout, as then when a user tries to log in you need to get their salt from the db, hash the supplied password then compare with the stored version, and if an unscrupulous person gets hold of the db then the salt is useless anyway.

 

[Augmented]

2) Is this a good method of coding. I realise that this is very much like OOP which most PHP users tend to stay away from. If there's a better way of doing things then please let me know

That's not anything like OOP, it's all flat and procedural. An OOP version might, for example, make use of an authentication class for handling passwords and logins, a user class for dealing with credentials and registering the user, a class to interface with the database (e.g. an abstraction layer) and so on.

Neither method is necessarily better than the other - it all depends on context. In this case, using OOP techniques would benefit you as you'll likely need to authenticate and deal with users many times, using the same functions throughout the application (and in many other applications!).

A few thoughts from a quick glance:

Instead of using

if (strlen($foo) > 0) { }

to check if a variable is empty, use the empty() function.

if (empty($foo)) { 
  // $foo is empty and may or may not be set (you should use isset() to check)
} else {
  // Do stuff with $foo
}

For this bit of code:

if($_POST['firstname'] == preg_match("/[^a-zA-Z]/", $_POST['firstname'])) { }

You only need to check the return value of preg_match(), and not compare it to the variable i.e.

if (!preg_match("|[^a-z]|i", $_POST['firstname'])) { 
  // firstname contains only letters
  // (I've use the "i" flag for a case-insensitive regex)
}

But better still, if you simply want to check a variable contains only letters, use the ctype_alpha() function, as it's much faster than using a regex:

if(ctype_alpha($_POST['firstname'])) { 
  // firstname contains only letters
}