Database "validation" PHP

Question

Hi everyone, I'm trying to validate the password entered by the user with the password in the database. I've worked out that it checks the username fine (if the username doesn't exist it displays an error), however when it tries to validate the password with the mysql password it never works. The working 'example' is at http://scapersclearing.com/fansite/login.php; and this is the PHP (note base.php contains the database information and header.php, navigation.php and footer.php and all front-end). I'm planning on adding html entities and preventing SQL injection once this works. Username: Test - Password: password89 (md5 c1c2434f064da663997b1a2a233bf9f6)

<?php 
include("base.php"); //Include MySQL connection

$username = $_POST['username']; //Connect form username with strings
$password = $_POST['password']; //Connect form password with strings

$salt = "xia8u28jd0ajgfa"; //Define the salt string
$salt2 = "oqipoaks42duaiu"; //Define the second salt string
$password = md5($salt.$password.$salt2); //Encrypt the password

$result = mysql_query("SELECT * FROM members WHERE username = '".$username."'"); //Open the members table
while($row = mysql_fetch_array( $result )) { //Convert the members table into an array

if ( $username != $row['username'] ) { //If user entered username doesn't equal the database username
    include("header.php"); //Print the message
    include("navigation.php");
    echo "Invalid username or password!";
    include("footer.php");
}
else {

if ( $row['password'] == $password ) { //Validate username and password
    setcookie('c_username', $username, time()+6000);  //Set the username cookie
    setcookie('c_password', $password, time()+6000);  //Set the cookie
    header("Location:index.php"); //Redirect to home page
} else {
    include("header.php"); //Print the message
    include("navigation.php");
    echo "<div class=\"content\"><p>Invalid username or password!<p></div>";
    include("footer.php");
} } }
?>

Answer 1

A few remarks / suggestions:

• You should check in your datebase for a valid username / password combination and exactly one row should be returned in the case of success, a while loop for valid usernames can only lead to errors when usernames are not unique (I suppose they should be, but that's up to you) and the first row found doesn't match your password.

There aren't any duplicate users in your table, are there? That would lead to the problem you describe: first row returns false, output is sent to the browser and headers cannot be sent any more, although a later row might be valid.

• Your first if statement doesn't make sense, you have already selected all rows where $username == $row['username'] so you are basically testing for 1!=1

So I think you need to simplify your code and if there are still errors after that, they will be a lot easier to find.

Answer 2

First of all make sure the hash you're generating is the same with the one you have inserted in the database at the time of the registration.

Then, the way you should authenticate a user is this:

$res = mysql_query("SELECT just,the,needed,info FROM `members` WHERE `username` = '". esc($user) ."' AND `password` = '". esc($pass) ."'");

if ($hlp = mysql_fetch_assoc($res))
    echo 'ok';
else
    echo 'invalid username or password';

where esc() is your fav. escape function (mysql_escape_string or mysql_real_escape_string).

Now the validity of the password is checked inside the database, so you never get the real hash out.

Answer 3

Here are my comments:

<?php 
include("base.php"); //Include MySQL connection

The following will fail if the 'username' or 'password' indexes don't exist. Use array_key_exists() to test the $_POST array first.

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

A single salt is adequate, you gain nothing by adding one half of it to the start and the other half to the end.

Also: your code comments are utterly useless. Don't bother to clutter up your code with comments that make such obvious and uninformative statements.

$salt = "xia8u28jd0ajgfa"; //Define the salt string
$salt2 = "oqipoaks42duaiu"; //Define the second salt string

But it's more secure to store a different random salt per user. You can store this in the database, in the members table. The purpose of using a salt is to defeat dictionary attacks. If the attacker knows that you use a single salt for all member accounts, then he can create a single set of pre-calculated hash values from dictionary words using your salt.

MD5() is not a very strong hash function. Some methods to compromise MD5 have been published. Better is SHA1, but even better is SHA-256. SHA-256 is supported by the hash() function in PHP but it's not yet supported as a native function in MySQL.

$password = md5($salt.$password.$salt2);

There's a wide-open SQL injection vulnerability in your query. I caution strongly against "fixing security holes later" because it's too easy to forget to do this for some queries. Just code for security right up front and then you won't miss any. It's easy to use SQL query parameters, so do this instead of interpolating unsafe variables into queries.

There's really no reason to use the antiquated ext/mysql API these days, and it's bad practice because it doesn't support features like query parameters. Use ext/mysqli or else PDO.

$result = mysql_query("SELECT * FROM members WHERE username = '".$username."'");
while($row = mysql_fetch_array( $result )) {

Why would $username ever be different from $row['username'] if you just queried for rows where that matches? You should be testing for an empty result set, which would be what happens if a user enters a non-existent username -- in which case your while() loop would iterate zero times and the following code would never run.

if ( $username != $row['username'] ) {
    include("header.php"); //Print the message
    include("navigation.php");
    echo "Invalid username or password!";
    include("footer.php");
}
else {
. . .

I prefer to run a query like the following when I'm validating passwords:

SELECT (password = SHA1(CONCAT(?, salt))) AS password_matches
FROM members WHERE username = ?

If the query returns an empty result set, the username given was wrong. If it returns a row but password_matches is zero, then the password given was wrong. Of course you don't want to reveal which was wrong to users, but you might want to know for your own purposes. For example, a given account that receives many failed login attempts should be locked.

If you want to use SHA-256, you have to fetch the user's salt and then use hash() in PHP code:

$stmt = $pdo->prepare("SELECT salt FROM members WHERE username = ?");
$stmt->execute( array($username) );
$rows = $stmt->fetchAll();
if (count($rows) > 0) {
  $password_hash = hash('sha256', $password . $rows[0]['salt']);
  $stmt = $pdo->prepare("SELECT password=? AS password_matches 
                         FROM members WHERE username = ?");
  $stmt->execute( array($password_hash, $username) );
  $rows = $stmt->fetchAll();
  if ($rows[0]['password_matches'] > 0) {
    // username and password are correct
  } else {
    // password was wrong
  }
} else {
  // username was not found
}

Answer 4

PHP has a crypt function that is perfect for this kind of thing. www.php.net/crypt

It auto-generates random salts and stores it with the password, making it very easy to use. Just see code below.

Combine this answer with the other ones regarding prepared statements and you'll be golden.

This is how you'd do it (I'm cutting out a lot of code, and taking shortcuts, just so you'll get the essentials):

<?php
# to register:
query('INSERT INTO users (user, pass) VALUES (?, ?)', array($user, crypt($pass)));

# to verify:
$urow = query('SELECT user, pass FROM users WHERE user = ?', array($user));
if (rows() < 1) {
  die('No such user');
} 
else if (crypt($pass, $urow['pass']) != $urow['pass']) {
  die('Wrong password!');
} 
#once you're here, it passed, and so on...
?>