views:

1458

answers:

3

How does one check to see if a user has typed in the right password to log in?

This is what (out of a bunch of combinations...) I am doing:

<?

$login = $_POST['login'];
$password = $_POST['password'];

mysql_connect('localhost', 'root', 'abc123');

mysql_select_db('aun_vox') or die(mysql_error());

$q = mysql_query("SELECT password FROM customer WHERE login='$login'");
$db_pass = mysql_result($q, 0);

if(md5($password) == $db_pass)
{
    echo "You did it.";
}

else echo "Wrong.";

?>

As I can see from the ouput, there's something wrong in the mysql_result bit, but I can't figure out the right way.

Can someone please help.

+2  A: 

some points:

  • Do NOT insert string data in the query string. Especially user-provided data. what would happen if i login with "Robert' ;drop table customer"?. Use either the escaping routines, or (far better) use prepared statements and bind parameters.
  • Do NOT store cleartext passwords in your database. (you're ok on this)
  • Do NOT retrieve passwords from your database.

what i usually do is something like:

$q = preparestatement ("SELECT id FROM customer WHERE login=? AND password=?");
bindvalue ($q, $_POST['login']);
bindvalue ($q, md5($_POST['password']));
$id = execprepared ($q);

if($id) {
    echo "You did it.";
} else {
    echo "Wrong.";
}
Javier
We call him little Bobby Tables.
R. Bemrose
You should probably point out that you're not using native PHP Code here. bindvalue() had me a bit confused, and even more so after failing to find it on php.net. Finally found PHP Data Objects (PDO) online which covered the information in your example.
Jonathan Sampson
+2  A: 

Firstly, make sure you properly escape your variables before using them in the query - use mysql_real_escape_string().

Then, why not use the MySQL MD5 function to check for a valid login in your query?

SELECT login FROM customer WHERE login='$login' AND password = MD5('$password')

Then just use mysql_num_rows() to count the number of returned rows.

BrynJ
+11  A: 

I see you are storing a hash of the password in the database, but for the benefit of other readers, never store passwords in plain text in the database. You don't want to be like Monster.com.uk!

You should use a stronger hashing function than MD5(). Ideally you should use SHA256. This hash method is available in PHP using the hash() function.

You should also apply a random salt to the password. Store a different salt value for each user's account. This helps to defeat dictionary attacks and rainbow table attacks.

You should learn to use the mysqli extension instead of the old mysql extension. Mysqli supports parameterized queries, so you can reduce vulnerability to some SQL injection attacks.

Here is some example code. I haven't tested it, but it should be pretty close to working:

$input_login = $_POST['login'];
$input_password = $_POST['password'];

$stmt = $mysqli->prepare("SELECT password, salt FROM customer WHERE login = ?");
$stmt->bind_param("s", $input_login);
$stmt->execute();
$stmt->bind_result($password_hash, $salt);

while ($stmt->fetch()) {
  $input_password_hash = hash('sha256', $input_password . $salt);
  if ($input_password_hash == $password_hash) {
    return true;
  }
  // You may want to log failed password attempts here,
  // for security auditing or to lock an account with
  // too many attempts within a short time.
}
$stmt->close();

// No rows matched $input_login, or else password did not match
return false;


Some other people suggest the query should test for login = ? AND password = ? but I don't like to do that. If you do this, you can't know if the lookup failed because the login didn't exist, or because the user provided a wrong password.

Of course you shouldn't reveal to the user which caused the failed login attempt, but you may need to know, so you can log suspicious activity.


@Javier says in his answer that you shouldn't retrieve the password (or password hash in this case) from the database. I don't agree.

Javier shows calling md5() in PHP code and sending that the resulting hash string to the database. But this doesn't support salting the password easily. You have to do a separate query to retrieve this user's salt before you can do the hash in PHP.

The alternative is sending the plaintext password over the network from your PHP app to your database server. Anyone wiretapping your network can see this password. If you have SQL queries being logged, anyone who gains access to the logs can see the password. Motivated hackers can even dumpster-dive to find old filesystem backup media, and might read the log files that way!

The lesser risk is to fetch the password hash string from the database into the PHP app, compare it to the hash of the user's input (also in PHP code), and then discard these variables.

Bill Karwin
There was just one thing missing in your code.mysqli_prepare() takes two parameters - the first being the result of the mysqli_connect() statement. Since I was using mysql_connect() instead, it wasn't working. But after the modification, it works fine. Thanks for your help.
Mussnoon
Thanks for the catch, I have fixed it. I didn't show connecting to the database, though. Just assumed you have a live $mysqli object.
Bill Karwin
mysqli is too horribly bugg to use (sadly)
cletus
@cletus: As opposed to what? PDO_mysql? The mysql extension? PHP itself?
Bill Karwin
@Bill: mysqli has two serious issues, one of which (seg faulting on LONGTEXT columns) hasn't been fixed in THREE YEARS. the other is random "canary problem" errors (or something like that; but I gaveup at that point. worked pefectly with mysql). Yes mysql or PDO.
cletus
@cletus: Well, since ext/mysql doesn't support parameterized queries or transactions, I would recommend PDO. I know about the LONGTEXT limitation, you can use MEDIUMTEXT instead. And it appears from the bug that the other issue is only detected by Suhosin-patched PHP.
Bill Karwin