Are there any security vulnerabilities in this PHP registration code?

Question

Can you experts give me some thougths on this code? Some security hole i have missed? Can you see any potential threats? Something i can do better?

I'm still learning :) Thanks

<?php

if (isset($_POST['username'])) {

$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
$password2 = mysql_real_escape_string($_POST['password2']);
$encrypted_password = md5($password);


// remove eventuakl space
foreach($_POST as $key => $val) $_POST[$key] = trim($val);


// check if username is taken
$query = mysql_query("SELECT COUNT(*) FROM users WHERE username = '$username'");
if (mysql_result($query, 0) > 0) {
$reg_error[] = 0;
}

// make sure username only cosist of at least 3 letters, numbers or _ -
if (!preg_match('/^[a-zA-Z0-9_-]{3,}$/', $username)) {
$reg_error[] = 4;  
}


// check for empty fields
if (empty($username) || empty($password) || empty($password2)) {
$reg_error[] = 2;
}

// check if the passwords match
if ($password != $password2) {
$reg_error[] = 3;
}

// save if error is unset
if (!isset($reg_error)) {
mysql_query("INSERT INTO users (username, password, registered, registration_ip)
             VALUES('$username', '$encrypted_password', '".time()."', '".$_SERVER['SERVER_ADDR']."')");

$_SESSION['id'] = mysql_insert_id();
header('refresh: 3; url=/home');

}


}
?>

Login.php

if (isset($_POST['username'])) {

$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);
$md5_password = md5($password); 

$query = mysql_query("SELECT id FROM users WHERE username = '$username' and password = '$md5_password'");


if (mysql_num_rows($query) == 0) {
header("Location: ".$_SERVER['REQUEST_URI']."");
exit;
}

// set session
$_SESSION['id'] = mysql_result($query, 0, 'id');
header("Location: /");
exit;

Answer 1

You didn't salt the password.

Also, md5() is considered not strong enough for hashing passwords.

Use hash('sha256', $password) instead.

Answer 2

I assume you're serving this on https, though you don't mention whether you do -- if you're not, username and password travel on the open net in the clear, and that's definitely not very safe.

There's a race condition -- you check whether the username is taken, first, and only later do you insert it. You should use a transaction, a least, and ideally just try to insert (with the uniqueness constraint imposed by the database) and catch the error in case of duplicates. And, you should do this only after all other sanity checks, i.e. when you've convinced yourself that, apart from possible duplicates, the registration attempt is OK.

Answer 3

Little bobby tables will give you a lot of headaches since you do not check for username validity.

Answer 4

You need to salt the password.

This is placed in the wrong place. Move it up a few lines before the $_POST vars are used.

// remove eventuakl space
foreach($_POST as $key => $val) $_POST[$key] = trim($val);

You are escaping the password fields for no reason. They are not being sent to the database. md5($password) is going to the database and it is not escaped.

EDIT: On the login side, you should be trimming anything you are trim on the registrations side.

Answer 5

Your error checking is out of order. I'd do the error checking in this order:

1. Check for empty fields
2. Check that the fields have valid values (filtering input)
3. Escape the fields with mysql_real_escape_string before using the fields in SQL
4. Check for the user in the SQL table

If you find an error don't continue with further checks. Guard each error check similar to your guard on the final INSERT statement.

You have no edits on the password fields besides using mysql_real_escape_string?

You should do mysql_connect before using mysql_real_escape_string. mysql_real_escape_string will use the connection to determine the character set of the connection. The character set will identify which characters to escape.

Answer 6

You should use parameters instead of building your sql dynamically. It will help prevent sql injection attacks. Little bobby tables will get you.