PHP Login password hash / salt problem.

[Hazukiy]

Hi, recently I've created a login form and I've used the salt method (which I've not really used before) and everything is working great apart from the login. Basically what happens is I can login with any password. So if my password was 'hello1234' and I put 'fndsjnmfosd' it would state that as correct; try it yourself at www.harvy.info      Sign up and then try to login, you'll see that you can enter any password and it'll see that as correct. Thanks.

 

 

Login proccess (What happens when you try to login)

<?php 
include 'dbConfig.php';
include 'functions.php';

sec_session_start();
 
if(isset($_POST['email'], $_POST['p'])) { 
   $email = $_POST['email'];
   $password = $_POST['p'];
   if(login($email, $password, $mysqli) == true) {
  
	  header('Location: member.php?id=');
   } else {
      header('Location: login.php?error=1');
   }
} else { 
   echo 'Invalid Request';
}


?>

 

 

[mac_gyver]

your code in the login() function probably contains a logical error.

Edited April 3, 2013 by mac_gyver

[Psycho]

your code in the login() function probably contains a logical error.

 

Or it isn't returning false when the login check fails. Post the code for the function login()

[Hazukiy]

Or it isn't returning false when the login check fails. Post the code for the function login()

 

Sorry forgot to add the login function, here it is:

 

 

function login($email, $password, $mysqli) {
   if ($stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1")) { 
      $stmt->bind_param('s', $email);
      $stmt->execute();
      $stmt->store_result();
      $stmt->bind_result($user_id, $username, $db_password, $salt);
      $stmt->fetch();
      $password = hash('sha512', $password.$salt);
 
      if($stmt->num_rows == 1) {
         if(checkbrute($user_id, $mysqli) == true) { 
            return false;
         } else {
         if($db_password == $password) { 
 
 
               $user_browser = $_SERVER['HTTP_USER_AGENT'];
 
               $user_id = preg_replace("/[^0-9]+/", "", $user_id);
               $_SESSION['user_id'] = $user_id; 
               $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);
               $_SESSION['username'] = $username;
               $_SESSION['login_string'] = hash('sha512', $password.$user_browser);
               return true;    
         } else {
            $now = time();
            $mysqli->query("INSERT INTO login_attempts (user_id, time) VALUES ('$user_id', '$now')");
            return false;
         }
      }
      } else {
         return false;
      }
   }
}

[mac_gyver]

your code functioned as expected for me. a mismatch in passwords goes to login.php?error=1 and matching passwords goes to member.php?id=

 

i'm going to guess that when it redirects to login.php?error=1 that either the logic or output on that page makes it look like it logged in correctly or code on that page is redirecting to make it look like it logged in correctly or the absence of exit statements after your header() redirects allows some code to run that makes it look like it logged in correctly.

 

what have you done to debug what your code is actually doing?

[Psycho]

I think this may be your problem:

 

if ($stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1")) {

 

On that very first line, if the $mysqli->prepare returns a false, the entire rest of the function is skipped and the function returns nothing - which may be interpreted as true. Perhaps your connection to the database does not exist. But, overall, I don't like the logic anyway. For example, why do you attempt to fetch the results BEFORE you check the num_rows returned? Also, it makes it hard to read the code when you check for a success condition and put the error condition way down at the end in else statements. Lastly, I see you are apparently setting a unit timestamp in the "time" field. That is a bad idea. Use a MySQL timestamp field and you can create the field such that it is automatically set when a new record is created (or edited) without having to set it within your code or the query. And, regardin gthe "login_attempts" table. I don't see anything in your code that would 'clear' that table if a user has a successful login.

 

I would do something like this:

 

function login($email, $password, $mysqli)
{
    $stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1")
    //Check if prepared query was succesful
    if (!$stmt) { return false; }

    //Execute the query
    $stmt->bind_param('s', $email);
    $stmt->execute();
    $stmt->store_result();

    //Check if any matching records found
    if($stmt->num_rows != 1) { return false; }

    $stmt->bind_result($user_id, $username, $db_password, $salt);
    $stmt->fetch();

    //Check if too many attempts
    if(checkbrute($user_id, $mysqli) == true) { return false; }
 
    $passwordCheck = hash('sha512', $password.$salt);

    //Check if password matches
    if($db_password != $passwordCheck)
    {
        $now = time();
        $mysqli->query("INSERT INTO login_attempts (user_id, time) VALUES ('$user_id', '$now')");
        return false;
    }

    //Everythign checks out. Complete login
    $user_browser = $_SERVER['HTTP_USER_AGENT'];
    $user_id = preg_replace("/[^0-9]+/", "", $user_id);
    $_SESSION['user_id'] = $user_id;
    $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);
    $_SESSION['username'] = $username;
    $_SESSION['login_string'] = hash('sha512', $password.$user_browser);
}

EDIT: ON second thought, there's no reason to create a separate function to check for too many invalid logins. Just JOIN the login_attempts table on the users table in your initial select query and you can check the login attempts based upon the initial result. Personally, I would just have a login_attempts FIELD in the user table and increment the value by 1 for every invalid login attempt (and, of course, set to 0 after a successful attempt)

Edited April 4, 2013 by Psycho

[mac_gyver]

the null that is returned if the prepare fails is == to false (just tested) and the code goes to login.php?error=1 for that case.

[Hazukiy]

I think this may be your problem:

 

if ($stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1")) {

 

On that very first line, if the $mysqli->prepare returns a false, the entire rest of the function is skipped and the function returns nothing - which may be interpreted as true. Perhaps your connection to the database does not exist. But, overall, I don't like the logic anyway. For example, why do you attempt to fetch the results BEFORE you check the num_rows returned? Also, it makes it hard to read the code when you check for a success condition and put the error condition way down at the end in else statements. Lastly, I see you are apparently setting a unit timestamp in the "time" field. That is a bad idea. Use a MySQL timestamp field and you can create the field such that it is automatically set when a new record is created (or edited) without having to set it within your code or the query. And, regardin gthe "login_attempts" table. I don't see anything in your code that would 'clear' that table if a user has a successful login.

 

I would do something like this:

 

function login($email, $password, $mysqli)
{
    $stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1")
    //Check if prepared query was succesful
    if (!$stmt) { return false; }

    //Execute the query
    $stmt->bind_param('s', $email);
    $stmt->execute();
    $stmt->store_result();

    //Check if any matching records found
    if($stmt->num_rows != 1) { return false; }

    $stmt->bind_result($user_id, $username, $db_password, $salt);
    $stmt->fetch();

    //Check if too many attempts
    if(checkbrute($user_id, $mysqli) == true) { return false; }
 
    $passwordCheck = hash('sha512', $password.$salt);

    //Check if password matches
    if($db_password != $passwordCheck)
    {
        $now = time();
        $mysqli->query("INSERT INTO login_attempts (user_id, time) VALUES ('$user_id', '$now')");
        return false;
    }

    //Everythign checks out. Complete login
    $user_browser = $_SERVER['HTTP_USER_AGENT'];
    $user_id = preg_replace("/[^0-9]+/", "", $user_id);
    $_SESSION['user_id'] = $user_id;
    $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);
    $_SESSION['username'] = $username;
    $_SESSION['login_string'] = hash('sha512', $password.$user_browser);
}

EDIT: ON second thought, there's no reason to create a separate function to check for too many invalid logins. Just JOIN the login_attempts table on the users table in your initial select query and you can check the login attempts based upon the initial result. Personally, I would just have a login_attempts FIELD in the user table and increment the value by 1 for every invalid login attempt (and, of course, set to 0 after a successful attempt)

 

Ok I just did that and it's stopping me from entering anything now, which is good but it's still not reading the password correctly, so what I think it might involve in the salting maybe? Here's the login that I have, oh and about the login attempts, I have the function below (sorry, forgot to add them.)

 

 

Functions.php

<?php 

function sec_session_start() {
        $session_name = 'sec_session_id'; // Set a custom session name
        $secure = false; // Set to true if using https.
        $httponly = true; // This stops javascript being able to access the session id. 
 
        ini_set('session.use_only_cookies', 1); // Forces sessions to only use cookies. 
        $cookieParams = session_get_cookie_params(); // Gets current cookies params.
        session_set_cookie_params($cookieParams["lifetime"], $cookieParams["path"], $cookieParams["domain"], $secure, $httponly); 
        session_name($session_name); // Sets the session name to the one set above.
        session_start(); // Start the php session
        session_regenerate_id(true); // regenerated the session, delete the old one.     
}


function login($email, $password, $mysqli)
{
    $stmt = $mysqli->prepare("SELECT id, username, password, salt FROM users WHERE email = ? LIMIT 1");
    //Check if prepared query was succesful
    if (!$stmt) { return false; }

    //Execute the query
    $stmt->bind_param('s', $email);
    $stmt->execute();
    $stmt->store_result();

    //Check if any matching records found
    if($stmt->num_rows != 1) { return false; }

    $stmt->bind_result($user_id, $username, $db_password, $salt);
    $stmt->fetch();

    //Check if too many attempts
    if(checkbrute($user_id, $mysqli) == true) { return false; }
 
    $passwordCheck = hash('sha512', $password.$salt);

    //Check if password matches
    if($db_password != $passwordCheck)
    {
        $now = time();
        $mysqli->query("INSERT INTO login_attempts (user_id, time) VALUES ('$user_id', '$now')");
        return false;
    }

    //Everythign checks out. Complete login
    $user_browser = $_SERVER['HTTP_USER_AGENT'];
    $user_id = preg_replace("/[^0-9]+/", "", $user_id);
    $_SESSION['user_id'] = $user_id;
    $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);
    $_SESSION['username'] = $username;
    $_SESSION['login_string'] = hash('sha512', $password.$user_browser);
}

function checkbrute($user_id, $mysqli) {
   // Get timestamp of current time
   $now = time();
   // All login attempts are counted from the past 2 hours. 
   $valid_attempts = $now - (2 * 60 * 60); 
 
   if ($stmt = $mysqli->prepare("SELECT time FROM login_attempts WHERE user_id = ? AND time > '$valid_attempts'")) { 
      $stmt->bind_param('i', $user_id); 
      // Execute the prepared query.
      $stmt->execute();
      $stmt->store_result();
      // If there has been more than 5 failed logins
      if($stmt->num_rows > 5) {
         return true;
      } else {
         return false;
      }
   }
}

function login_check($mysqli) {
   // Check if all session variables are set
   if(isset($_SESSION['user_id'], $_SESSION['username'], $_SESSION['login_string'])) {
     $user_id = $_SESSION['user_id'];
     $login_string = $_SESSION['login_string'];
     $username = $_SESSION['username'];
 
     $user_browser = $_SERVER['HTTP_USER_AGENT']; // Get the user-agent string of the user.
 
     if ($stmt = $mysqli->prepare("SELECT password FROM users WHERE id = ? LIMIT 1")) { 
        $stmt->bind_param('i', $user_id); // Bind "$user_id" to parameter.
        $stmt->execute(); // Execute the prepared query.
        $stmt->store_result();
 
        if($stmt->num_rows == 1) { // If the user exists
           $stmt->bind_result($password); // get variables from result.
           $stmt->fetch();
           $login_check = hash('sha512', $password.$user_browser);
           if($login_check == $login_string) {
              // Logged In!!!!
              return true;
           } else {
              // Not logged in
              return false;
           }
        } else {
            // Not logged in
            return false;
        }
     } else {
        // Not logged in
        return false;
     }
   } else {
     // Not logged in
     return false;
   }
}


?>

 

login.php

 

<script type="text/javascript" src="sha512.js"></script>
<script type="text/javascript">

function formhash(form, password) {
   var p = document.createElement("input");
   form.appendChild(p);
   p.name = "p";
   p.type = "hidden"
   p.value = hex_sha512(password.value);
   password.value = "";
   form.submit();
}
</script>
<h1>Login:</h1>
<form action="process_login.php" method="post" name="login_form">
Email:<br>
<input class="login_form" type="text" name="email"/>
<br><br>
Password:<br>
<input class="login_form" type="password" name="password" id="password"/>
<br><br>
<button type="submit" value="Login" onclick="formhash(this.form, this.form.password);" class="form_submit_button" name="login">Login</button>
</form>

<?php

if(isset($_GET['error'])) { 
   echo '<br>Error Logging In!';
}
?>

 

login_process.php

 

<?php 
include 'dbConfig.php';
include 'functions.php';

sec_session_start(); // Our custom secure way of starting a php session. 
 
if(isset($_POST['email'], $_POST['p'])) { 
   $email = $_POST['email'];
   $password = $_POST['p']; // The hashed password.
   if(login($email, $password, $mysqli) == true) {
  
	  header('Location: member.php?id=');
   } else {
      // Login failed
      header('Location: login.php?error=1');
   }
} else { 
   // The correct POST variables were not sent to this page.
   echo 'Invalid Request';
}


?>

Edited April 4, 2013 by Hazukiy

[mac_gyver]

the login() function code needs to return a true value when there is a successful login.

[Hazukiy]

the login() function code needs to return a true value when there is a successful login.

 

 

Which part cause I'm looking at it atm and it seems ok?

[Psycho]

On the very last line of the function login(), where everything succeeded, put

 

return true;

[Hazukiy]

 

On the very last line of the function login(), where everything succeeded, put

 

return true;

 

Sorry, I'm confused, are you talking about the process_login.php, functions.php or the login.php?

[Hazukiy]

UPDATE:

 

Ok so at this point when I enter my correct email & password in the login form it keeps returning false no matter what?

Any ideas? I've tried putting 

return true;

 

Right at the end of the functions.php login function() but that didn't work; that just returned it true no matter what happend.

[mac_gyver]

how do you know it is always returning a true value? what output, error, url, or other symptom are you getting?

[Hazukiy]

how do you know it is always returning a true value? what output, error, url, or other symptom are you getting?

 

 

Oh well atm it's returning this:

 

login.php?error=1

[mac_gyver]

that means it is returning a false value, not a true value, and it's going to the code in the else part of the statement.

[Jessica]

Did you lock yourself out with too many failed attempts?

[Hazukiy]

Did you lock yourself out with too many failed attempts?

 

Lol would've laughed if it was because of that but no; I registered another account and retried and it still coming up with that error :/

[Christian F.]

Hazukiy: There is only one file which contain a function named "login", and it was at the end of that function you were supposed to put return true;. Not at the end of the file.

 

If a function returns nothing, it returns NULL. NULL is cast to false, when used in a boolean expression. Which is why your function doesn't work as intended.

[Hazukiy]

that means it is returning a false value, not a true value, and it's going to the code in the else part of the statement.

 

 

Ah ok so it's ignoring this:

 

if(isset($_POST['email'], $_POST['p'])) { 
   $email = $_POST['email'];
   $password = $_POST['p'];
   if(login($email, $password, $mysqli) == true) {
  
	  header('Location: member.php?id=');
   }

 

 

And going to this: 

 

else {
      header('Location: login.php?error=1');

[Hazukiy]

Oh sorry I misstyped what I said previously, what I meant was I put the return true; on the last line just after this:

 

 

$user_browser = $_SERVER['HTTP_USER_AGENT'];
    $user_id = preg_replace("/[^0-9]+/", "", $user_id);
    $_SESSION['user_id'] = $user_id;
    $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);
    $_SESSION['username'] = $username;
    $_SESSION['login_string'] = hash('sha512', $password.$user_browser);

Edited April 5, 2013 by Hazukiy

[Christian F.]

Also, I've got two more comments on your code.

 

First of all you should never silently manipulate the username, as you're doing here. Instead you should validate its legality against a regular expression, and then show the user an error message if it fails. This way the user knows exactly what his or her username is, and doesn't have to guess at what you changed it to in case of an illegal character. Guessing would be required, since you didn't tell the user what those illegal characters were.

Even if you're doing the same replacement on login. After all, bugs do occur.

 

Secondly: I would really recommend using Blowfish instead of SHA256. The reason is quite nicely explained in this video.

 

Other than that your code looks quite good, and you've clearly put some time and effort into understanding the security implications of a login system.

 

PS: preg_replace replaces every instance of a non-legal character, so no need for the + quantifier.

Edited April 5, 2013 by Christian F.

[Hazukiy]

Also, I've got two more comments on your code.

 

First of all you should never silently manipulate the username, as you're doing here. Instead you should validate its legality against a regular expression, and then show the user an error message if it fails. This way the user knows exactly what his or her username is, and doesn't have to guess at what you changed it to in case of an illegal character. Guessing would be required, since you didn't tell the user what those illegal characters were.

Even if you're doing the same replacement on login. After all, bugs do occur.

 

Secondly: I would really recommend using Blowfish instead of SHA256. The reason is quite nicely explained in this video.

 

Other than that your code looks quite good, and you've clearly put some time and effort into understanding the security implications of a login system.

 

PS: preg_replace replaces every instance of a non-legal character, so no need for the + quantifier.

 

Thanks ^^ And yeah there is no point in getting a job half done while doing it, so I'm trying to make it safe as possible. Also I'm using SHA512

[Christian F.]

Sorry about that, slight brain fart. Still, Blowfish is better than SHA512 as well.

 

That said, I think I might have found your problem: Why are you using JS to hash the password on the client side, and then PHP to hash it again on the server side?

[Hazukiy]

Sorry about that, slight brain fart. Still, Blowfish is better than SHA512 as well.

 

That said, I think I might have found your problem: Why are you using JS to hash the password on the client side, and then PHP to hash it again on the server side?

 

Oh tbh that's the only method I know Why is there another way to execute it?

 

Edit: And for now I think I'll just stick to 512 as I'm having quite a lot of problem as it is with hashing & salting :/

Edited April 5, 2013 by Hazukiy