PHP Login password hash / salt problem.

[Christian F.]

Don't hash it on the client side, as you're ending up with double-hashing the password when a user tries to log in.

 

As for sticking to SHA512 because you're having problems with the hashing, the problem you're having is a logical one. Not related to the actual hashing algorithm you're using. Solve the problem, and replace SHA512 with Blowfish, and you won't notice any difference (except in security).

[mac_gyver]

there's 4 different conditions in the login() function that return a false value and since the code isn't reporting in any way why it is failing you need to debug at what point the code is returning the false value to find out why it is failing.

 

the first one is if the prepare fails. that's a fatal application error. during development your code should be screaming at you at that point telling you exactly why it failed.

 

the second one is if the email is not found. that's an application warning and it means that someone tried to log in using an email that doesn't exist. you should be logging everything about that occurrence and during development your code should be screaming at you at that point actively telling you why the function is returning a false value.

 

the third one is if the checkbrute() function doesn't pass. same comment as for the second one.

 

the fourth one is if the passwords don't match. same comment as for the above two cases.

 

 

[Hazukiy]

there's 4 different conditions in the login() function that return a false value and since the code isn't reporting in any way why it is failing you need to debug at what point the code is returning the false value to find out why it is failing.

 

the first one is if the prepare fails. that's a fatal application error. during development your code should be screaming at you at that point telling you exactly why it failed.

 

the second one is if the email is not found. that's an application warning and it means that someone tried to log in using an email that doesn't exist. you should be logging everything about that occurrence and during development your code should be screaming at you at that point actively telling you why the function is returning a false value.

 

the third one is if the checkbrute() function doesn't pass. same comment as for the second one.

 

the fourth one is if the passwords don't match. same comment as for the above two cases.

 

 

I think the reason for this is because someone previously gave me the code to put in and said that this would fix it (I think it was on this post?) but it seems to of made it worse.

[Hazukiy]

there's 4 different conditions in the login() function that return a false value and since the code isn't reporting in any way why it is failing you need to debug at what point the code is returning the false value to find out why it is failing.

 

the first one is if the prepare fails. that's a fatal application error. during development your code should be screaming at you at that point telling you exactly why it failed.

 

the second one is if the email is not found. that's an application warning and it means that someone tried to log in using an email that doesn't exist. you should be logging everything about that occurrence and during development your code should be screaming at you at that point actively telling you why the function is returning a false value.

 

the third one is if the checkbrute() function doesn't pass. same comment as for the second one.

 

the fourth one is if the passwords don't match. same comment as for the above two cases.

 

 

What do you think about this code?

 

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]

no matter what code you use, you must find out why it isn't working, because the cause of the problem could be somewhere else, such as in the registration code that is producing the values and inserting them into the database table or in the database table definition itself.

 

if all you are doing is copying code and praying it will work (copy-n-pray is not a useful programming pattern), you are not going to get very far very fast.

[Hazukiy]

no matter what code you use, you must find out why it isn't working, because the cause of the problem could be somewhere else, such as in the registration code that is producing the values and inserting them into the database table or in the database table definition itself.

 

if all you are doing is copying code and praying it will work (copy-n-pray is not a useful programming pattern), you are not going to get very far very fast.

 

Ok but I really can't see any issues with the code (Apart from whats mentioned) and I'm mainly on here so I can other opinions on what the problem could be; so far in total I've spent about 3 weeks trying to resolved the login / registerion problem. As far as I can see the register is fine cause in the database both hash and salt are correct along with all the information.

[mac_gyver]

when you have code that's producing the wrong result (in your case, you expect it to return a true value when the entered password is correct), it's not a matter of getting opinions on what could be causing the problem. it's a matter of actually finding which of the multiple possible problems is causing that wrong result.

 

your first step will be to determine which of the four points in the code that can return a false value is the one that actually is doing it, because that determines where to look to fix the problem.

[Psycho]

mac_gyver is right. If code is not producing the results you expect, do not jump to the conclusion that you need to completely change the logic. Besides, there shouldn't be anything wrong with that code (I provided it ). You just need to learn how to DEBUG. Debugging is not a hard concept and it amazes me how many people can't do simple validations. But, a couple of things you should always think of doing in these types of situations:

 

1) Verify what values contain. Don't assume a variable in your script contains what you *think* it does. Use an echo or var_dump() to help.

2. Verify that your if/else conditions are doing what you think they are. I like to output the values used in the condition before the condition is executed and then put an echo within the condition block to verify if the condition was true.

 

There are plenty of techniques to use. Just determine the most logical point of failure and add some code to check what really is happening. Then based upon those results you may need to test something else. But, eventually you will narrow down the problem.

 

In this case you have four points of possible failure. You could add a few echo statements to see which one is "failing" and then you can work on determining why it is failing. Look at the four points of failure where I've put an echo statement. You will now get a message on screen as to why the function is returning false. Note, I put an exit() after each echo to stop the script from doing the redirect. You should remove or comment out those lines after you resolve the problem. There are much better techniques than this, but this will get you started.

 

 

<?php


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)
    {
        echo "Failed trying to prepare query"; exit();
        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)
    {
        echo "No matching records on email '{$email}'"; exit();
        return false;
    }

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

    //Check if too many attempts
    if(checkbrute($user_id, $mysqli) == true)
    {
        echo "The user is locked."; exit();
        return false;
    }
 
    $passwordCheck = hash('sha512', $password.$salt);

    //Check if password matches
    if($db_password != $passwordCheck)
    {
        echo "The passwords do not match."; exit();
        $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);
    return true;
}

?>

[Hazukiy]

 

mac_gyver is right. If code is not producing the results you expect, do not jump to the conclusion that you need to completely change the logic. Besides, there shouldn't be anything wrong with that code (I provided it ). You just need to learn how to DEBUG. Debugging is not a hard concept and it amazes me how many people can't do simple validations. But, a couple of things you should always think of doing in these types of situations:

 

1) Verify what values contain. Don't assume a variable in your script contains what you *think* it does. Use an echo or var_dump() to help.

2. Verify that your if/else conditions are doing what you think they are. I like to output the values used in the condition before the condition is executed and then put an echo within the condition block to verify if the condition was true.

 

There are plenty of techniques to use. Just determine the most logical point of failure and add some code to check what really is happening. Then based upon those results you may need to test something else. But, eventually you will narrow down the problem.

 

In this case you have four points of possible failure. You could add a few echo statements to see which one is "failing" and then you can work on determining why it is failing. Look at the four points of failure where I've put an echo statement. You will now get a message on screen as to why the function is returning false. Note, I put an exit() after each echo to stop the script from doing the redirect. You should remove or comment out those lines after you resolve the problem. There are much better techniques than this, but this will get you started.

 

 

<?php


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)
    {
        echo "Failed trying to prepare query"; exit();
        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)
    {
        echo "No matching records on email '{$email}'"; exit();
        return false;
    }

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

    //Check if too many attempts
    if(checkbrute($user_id, $mysqli) == true)
    {
        echo "The user is locked."; exit();
        return false;
    }
 
    $passwordCheck = hash('sha512', $password.$salt);

    //Check if password matches
    if($db_password != $passwordCheck)
    {
        echo "The passwords do not match."; exit();
        $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);
    return true;
}

?>

 

That's good thanks, I understand now. Ok so I did some testing and what it seems to be doing is it passes all the checks and goes to the final part but this is where it gets weird, it doesn't check the password correctly as my password as hello1234 and I entered jimfdsfds and it still passed through. So far these are the following things that I can see wrong with it.

 

1) Login form doesn't check password to database password correctly.

2) When it does login it's not checking login correctly cause it's returning false on this:

 

 

if(login_check($mysqli) == true) {
 
   include 'member_profile.php';
 
} else {
   echo '<span style="color: white; float: right; font-size: 25px; margin-top: 250px; width: 100%; max-width: 1200px; direction: ltr;" text-align: center>You must be logged in to access this page.</span>';
}

 

So to conclude I think it might be something to do with the hash / salt or the login check?

 

 

 

Edit: Here's the login_check function.

 

 

function login_check($mysqli) {
   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'];
 
     if ($stmt = $mysqli->prepare("SELECT password FROM users WHERE id = ? LIMIT 1")) { 
        $stmt->bind_param('i', $user_id);
        $stmt->execute();
        $stmt->store_result();
 
        if($stmt->num_rows == 1) {
           $stmt->bind_result($password);
           $stmt->fetch();
           $login_check = hash('sha512', $password.$user_browser);
           if($login_check == $login_string) {

              return true;
           } else {
              return false;
           }
        } else {
            return false;
        }
     } else {
        return false;
     }
   } else {
     return false;
   }
}

Edited April 6, 2013 by Hazukiy

[Jessica]

Before each return add a die("test 1"); but change the number.

[Jessica]

Why are you only letting people login from the same browser they registered with?

[Hazukiy]

Why are you only letting people login from the same browser they registered with?

 

Oh when I was learning how to make a safe login form I was told to make it so that they can only login from the same browser. You think that I should remove it?

[Jessica]

How on earth is that logical? I use 4 different browsers every day on 5 different devices.

[Hazukiy]

How on earth is that logical? I use 4 different browsers every day on 5 different devices.

 

I'm only doing what I was taught.

[Psycho]

I'm only doing what I was taught.

 

Seriously?! Learning to do something is not about simply storing a bunch of facts that you regurgitate back. You need to understand WHY something is the best approach. Did you ever bother to ask why you should verify why someone is on the same browser as they registered on? There are some security concerns with verifying someone is on the same PC as the one they registered on and giving them a way to register a new PC, this probably isn't one of them

[jcbones]

The person that told you that, probably meant that you should validate each page request to make sure that it is from the same browser.  This would eliminate session hijacking.  But that would just be held in $_SESSION, and never hit the database.  It would be part of a token system, not a login system.

[Hazukiy]

The person that told you that, probably meant that you should validate each page request to make sure that it is from the same browser.  This would eliminate session hijacking.  But that would just be held in $_SESSION, and never hit the database.  It would be part of a token system, not a login system.

 

Ah ok, thanks.

[Hazukiy]

So I really do think it's the hash not working so I'm just going to test some alternative methods for login & register forms and see if they work because I'm getting no where with this method.