How to improve login script??

Question

How can I ensure my login script is secure and make it better, This is my first code:

Help is most appreciated.

<?php

include ('../includes/db_connect.php');

$firstname = $_POST['firstname'];
$lastname = $_POST['lastname'];
$email = $_POST['email']; 
$mobile = $_POST['mobile']; 
$username = $_POST['username'];
$password = md5($_POST['password']);

// lets check to see if the username already exists

$checkuser = mysql_query("SELECT username FROM users WHERE username='$username'");

$username_exist = mysql_num_rows($checkuser);

if($username_exist > 0){
    echo "I'm sorry but the username you specified has already been taken.  Please pick another one.";
    unset($username);
    header("Location: /registration?registration=false");
    exit();
}

// lf no errors present with the username
// use a query to insert the data into the database.

$query = "INSERT INTO users (firstname, lastname, email, mobile, username, password)
VALUES('$firstname', '$lastname','$email', '$mobile','$username', '$password')";
mysql_query($query) or die(mysql_error());
mysql_close();

echo "You have successfully Registered";
header("Location: /registration?registration=true");   
// mail user their information

//$yoursite = ‘www.blahblah.com’;
//$webmaster = ‘yourname’;
//$youremail = ‘youremail’;
//    
//$subject = "You have successfully registered at $yoursite...";
//$message = "Dear $firstname, you are now registered at our web site.  
//    To login, simply go to our web page and enter in the following details in the login form:
//    Username: $username
//    Password: $password
//    
//    Please print this information out and store it for future reference.
//    
//    Thanks,
//    $webmaster";
//    
//mail($email, $subject, $message, "From: $yoursite <$youremail>\nX-Mailer:PHP/" . phpversion());
//    
//echo "Your information has been mailed to your email address.";

?>

Answer 1

That's not a login script. It's a registration script.

See SQL injection in the PHP manual. Your program is vulnerable to this kind of attacks.

Also, don't just or die(mysql_error()). This will expose information about your database that you may not want to expose (table names, etc.). Use proper error handling. For instance, you can throw an exception and define a uncaught exception handler that shows a "oops" page and logs the error.

Finally, use hashes strong than MD5, such as sha1.

Answer 2

Follow Artefacto's advice about SQL injection and Hashing passwords in the database. Other things ...

echo "I'm sorry but the username you specified has already been taken.  Please pick another one.";
unset($username);
header("Location: /registration?registration=false");

Wont work because you can't echo then send a header. Headers must be sent before any output.

Also, there is no point doing this:

header("Location: /registration?registration=false");
echo "I'm sorry but the username you specified has already been taken.  Please pick another one.";
unset($username);

The webbrowser will redirect straight away and the user won't see the handy message you've printed.

Also, it's usual to ask for 2 password fields on registration forms incase the user made a typo and didn't notice because all the text was *'s. You compare the 2 and if they are different you assume a typo was made and ask again.

Answer 3

As said by @Artefacto, that's not a login script. But if you intend to do a login script I would like to give you a suggestion. I've done this a while ago.

Instead of doing something like this:

$sql = "SELECT * FROM users WHERE username = '$username' AND password = '$password'";

I would do this:

$sql = "SELECT * FROM users WHERE username = '$username'";
$user = //use the php-sql (query, fetch_row) commands to fetch the user row.
if (strcmp($user['password'], $password) == 0) {
    //log in success
}

By doing this, you avoid SQL Injection in a simple and elegant way. What you guys think about it?

Answer 4

To reiterate what everyone else mentioned. It's important to protect yourself (and sever) from SQL injection. For example:

$checkuser = mysql_query("SELECT username FROM users WHERE username='$username'");

You're just simple taking the value from $_POST['username'] and placing it in the variable $username.

Some people aren't very nice and will try to break your program :( So it's always recommended to escape any data that was taken from a user, before placing it into an SQL query.

For instance...
This:

$checkuser = mysql_query("SELECT username FROM users WHERE username='$username'");

Becomes:

$checkuser = mysql_query("SELECT username FROM users WHERE username='" .mysql_real_escape_string($username). "'");