Jump to content

PHP Login password hash / salt problem.


Hazukiy

Recommended Posts

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';
}


?>

 

 

Link to comment
Share on other sites

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;
      }
   }
}
Link to comment
Share on other sites

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?

Link to comment
Share on other sites

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 by Psycho
Link to comment
Share on other sites

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 by Hazukiy
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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');
Link to comment
Share on other sites

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 by Hazukiy
Link to comment
Share on other sites

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

 

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 by Christian F.
Link to comment
Share on other sites

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

 

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 :)

Link to comment
Share on other sites

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

 

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 by Hazukiy
Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.