PHP login security Idea

Question

I have an idea for a system to log in users and validate their login on pages.
I realize that there are lots of systems out there, but I mainly was curious if the idea I had was any good. I've done some digging, but most results seem to leave out what I've always thought to be important practices (like password encryption, etc). I'll probably look harder for a pre-made solution, as it is probably more secure, but I haven't really worked with application security, and was hoping to get some feedback.

When a user logs in, their name and password are verified against a database, the password is encrypted using SHA256 and a randomly generated salt, the overall string (both the salt and the encrypted password is 128 chars. long). Here's the password validation code:

 function ValidatePassword($password, $correctHash)
 {
    $salt = substr($correctHash, 0, 64); //get the salt from the front of the hash
    $validHash = substr($correctHash, 64, 64); //the SHA256

    $testHash = hash("sha256", $salt . $password); //hash the password being tested
    //if the hashes are exactly the same, the password is valid
    return $testHash === $validHash;
}

If the login is valid, they are assigned a token. This token is similar to the password encryption, but stores the encrypted epoch as well as another random salt. The token, the login time, an expiration time, and the username are stored in a DB and the username and the token are transmitted as session information.

Here's the code that creates the token:

function loginUser($email)
{
  $thetime = time();
  $ip = $_SERVER['REMOTE_ADDR'];

  $dbuser="///";
  $dbpass="///";
  $dbtable="tokens";
  mysql_connect(localhost,$dbuser,$dbpass);
  mysql_select_db("///") or die( "Unable to select database");

  //Generate a salt
  $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
  //Hash the salt and the current time to get a random token
  $hash = hash("sha256", $salt . $password);
  //Prepend the salt to the hash
  $final = $salt . $hash;

  $exptime = $thetime + 3600;

  //Store this value into the db
  $query = "INSERT INTO `spanel`.`tokens` VALUES ('$final', $thetime, $exptime,    $thetime, '$ip', MD5('$email') )";
  mysql_query($query) or die ("Could not create token.");

  //Store the data into session vars
  $_SESSION['spanel_email'] = $email;
  $_SESSION['spanel_token'] = $final;

  return true;

}

When they reach a page, the token they have and the username are checked against the DB. If the check is good, the expiration time is updated and the page loads. Here's that code:

function validateUser($page)
{
  //Grab some vars
  $thetime = time();
  $ip = $_SERVER['REMOTE_ADDR'];
  $token = $_SESSION['spanel_token'];
  $email = $_SESSION['spanel_email'];

  $dbuser="///";
  $dbpass="///";
  $dbtable="tokens";
  mysql_connect(localhost,$dbuser,$dbpass);
  mysql_select_db("///") or die( "Unable to select database");

  //Global var
  //Get the var for token expire
  $token_expire = 3600;
  //Validate the token
  $query = "SELECT * FROM `tokens` WHERE `token` LIKE '$token' AND `user_id` LIKE   MD5('$email') AND `exp` > $thetime";
  $result = mysql_query($query) or die(mysql_error());
  //Check if we have a valid result
  if ( mysql_num_rows($result) != 1 ) {
    //Logout the user
    //Destroy the session
    session_destroy();
    //Redirect
    header("location: /spanel/login.php?denied=1");
    exit();
    //(Since the token is already invalid, there's no reason to reset it as invalid)
  }
  $row = mysql_fetch_assoc($result);

  //Update the token with our lastseen
  $newexp = $thetime + $token_expire;
  $query = "UPDATE `spanel`.`tokens` SET `exp` = $newexp, `lastseen_ip` = $thetime,    `lastseen_ip` = '$ip' WHERE `token` LIKE '$token'";
  mysql_query($query);

}

Feedback (good and bad) is appreciated. Like I said, I haven't done much security and was hoping to get pointers.

EDIT: I fear I overestimated my ability to effectively create a login system. Saying this, I understand if you decide to stop trying to figure out the jumbled mess that was this probably flawed idea.

Nevertheless, here's the php code from the login page. (After what's been said here, I realize just POST'ing the password is a big no-no).

    $email = $_POST['email'];
    $password = $_POST['password'];
    $dbuser="///";
    $dbpass="///";
    $dbtable="///";
    mysql_connect(localhost,$dbuser,$dbpass);
    mysql_select_db("spanel") or die( "Unable to select database");
    $query = "SELECT * FROM users WHERE `email` LIKE '$email'";
    $result=mysql_query($query) or die(mysql_error());
    $num=mysql_num_rows($result);
    $row = mysql_fetch_array($result);
    if ( ValidatePassword($password, $row['hash']) == true ) {
      loginUser($email);
      header("location: /spanel/index.php");
    } else {
      echo "<p>Login Failed.</p>";
    }

Here's the bit that generates the password salt and hash when the account is created.

 function HashPassword($password)
 {
    $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM)); //get 256 random bits in hex
    $hash = hash("sha256", $salt . $password); //prepend the salt, then hash
    //store the salt and hash in the same string, so only 1 DB column is needed
    $final = $salt . $hash; 
    return $final;
}

Thanks for the feedback, I'm glad the problems with my lack of knowledge were found here and not after an attack.

Answer 1

For one, hashing like that is insecure. sha256 can be broken easily, at least for short passwords. You must use some hash stretching. See if you can find some PBKDF2 implementations or use the wikipedia's suggestion for a "for loop" with sha256. Moreover I don't get what you achieve with having more session variables. I don't understand what validateuser() does, it still relies on session id, or am I missing something.

Answer 2

Similar to sivann, I can’t see the reason for the additional spanel_token either. All it seems to effectively do is to ensure that the session is no longer valid after the token’s expiration time. Since both values for token and user_id for the WHERE condition are stored in the session and are only set during the login, they won’t change. But session expiration can be implemented much easier.

---

But apart from that and much more important: your code is vulnerable to SQL injection. It would be easy with the knowledge you’ve posted here. All you need is to do the following steps:

• Find out the number of columns of users with a UNION SELECT injection in email:

  ' UNION SELECT null, …, null WHERE ''='
  

  If the wrong number of columns is entered, your script will throw a MySQL error, otherwise the “Login Failed.” will appear. Thanks for that.

• By using the following query:

  SELECT t1.* FROM users t1 RIGHT JOIN (SELECT email, '000000000000000000000000000000000000000000000000000000000000000060e05bd1b195af2f94112fa7197a5c88289058840ce7c6df9693756bc6250f55' hash FROM users LIMIT 1) t2 USING (email);
  

  the value 000000000000000000000000000000000000000000000000000000000000000060e05bd1b195af2f94112fa7197a5c88289058840ce7c6df9693756bc6250f55 is injected into each record instead of the original hash column value. The leading 0s are the salt and the remaining string is the salted SHA-256 hash value for an empty password string, which will result in a valid password.

  So we end up with entering an empty string for the password field and the following for the email field:

  ' UNION SELECT t2.* FROM users t1 RIGHT JOIN (SELECT email, '000000000000000000000000000000000000000000000000000000000000000060e05bd1b195af2f94112fa7197a5c88289058840ce7c6df9693756bc6250f55' hash FROM users LIMIT 1) t2 USING (email) WHERE ''='
  

This should suffice to get ‘authenticated’ as any user.

Answer 3

Just commenting on the salt/hash system: Storing the salt alongside the password in the database sort of defeats the purpose of salting--if you database is compromised the salt becomes useless from a security perspective, because its right there to aid in guessing / breaking the security. The purpose of a salt is to increase the time taken to guess a hashed word by adding a secret string of appropriate length to the value you are hashing.