Having problem while storing data in SESSIONS & COOKIES

[khurram001]

I have created a login form. I am sending values through Ajax for form validation. However, I am having problem with the code that I am unable to store values in Sessions & Cookies.

 

I have added a "Remember me" checkbox into login form. I want to validate Boolean value using Javascript Checked property and send the data to PHP for validation.

 

 If user clicks on remember me checkbox then the data should be stored in either Sessions & Cookies. If it is not checked then data should be stored only in Sessions. I am posting here my login form code, Ajax code & PHP code.

 

Could you guys help me to point out my mistake what I am doing wrong in this code?

 

Login Form:

<input type="checkbox" id="cb" name="cb">
<label for="cb">Remember me</label>

Ajax Code:

function login(){var e = _("email").value;
var pass = _("password").value;
var cb = _("cb").value;
if(e == "" || pass == ""){
_("status").innerHTML = "Please fill out the form";
} else {
_("loginbtn").style.display = "none";
_("status").innerHTML = 'please wait ...';
var ajax = ajaxObj("POST", "handlers/login_handler.php");
        ajax.onreadystatechange = function() {
       if(ajaxReturn(ajax) == true) {
           if(ajax.responseText == "login_failed"){
_("status").innerHTML = "Login failed, please try again.";
_("loginbtn").style.display = "block";
} else {
window.location = "message.php?msg=Hello "+ajax.responseText;
}
  }
    } 
        ajax.send("e="+e+"&pass="+pass+"&cb="+cb);
}
} 

PHP Code:

$cb = cleanstr($_POST['cb']);
if(isset($cb) && ($cb == true)) {
// IF USER CLICKED ON REMEMBER ME CHECKBOX CREATE THEIR SESSIONS AND COOKIES
$_SESSION['userid'] = $db_id;
$_SESSION['username'] = $db_username;
$_SESSION['password'] = $db_pass;
setcookie("id", $db_id, strtotime( '+30 days' ), "/", "", "", TRUE);
setcookie("user", $db_username, strtotime( '+30 days' ), "/", "", "", TRUE);
     setcookie("pass", $db_pass, strtotime( '+30 days' ), "/", "", "", TRUE); 
// UPDATE THEIR "IP" AND "LASTLOGIN" FIELDS
$sql = "UPDATE users SET ip='$ip', lastlogin=now() WHERE id='$db_id' LIMIT 1";
            $query = mysqli_query($con, $sql);
echo $db_username;
   exit();
} else {
// IF USER HAS NOT CLICKED ON REMEMBER ME CHECKBOX CREATE THEIR SESSIONS ONLY
$_SESSION['userid'] = $db_id;
$_SESSION['username'] = $db_username;
$_SESSION['password'] = $db_pass;
// UPDATE THEIR "IP" AND "LASTLOGIN" FIELDS
$sql = "UPDATE users SET ip='$ip', lastlogin=now() WHERE id='$db_id' LIMIT 1";
            $query = mysqli_query($con, $sql);
echo $db_username;
   exit();
}

Edited December 23, 2014 by khurram001

[Jacques1]

None of this makes any sense, and almost every line is a gaping security hole. Do you honestly store the plaintext password in your database and the session and a frigging cookie? Why on earth would you do that? The password is secret data. You don't store it anywhere, especially not in a session or cookie!

 

Since you're obviously new to web programming, I strongly recommend that you learn the basics of security before you go any further. Things like: What do I do with user passwords? How do I pass PHP values to an SQL query?

 

This remember-me stuff is particularly nasty and should be avoided at all cost. If you absolutely must have it, then you need to learn twice as much and actually think before you write code. One possible approach is to use temporary random access tokens:

• You generate a strong random number. For example, read 16 bytes from /dev/urandom or a similar device.
• You store this number (not the password or the username) in a special remember-me cookie. Since this cookie is critical, make sure that it's protected. For example, set the HTTPOnly and the Secure flag (the latter only works if your site supports HTTPS).
• Then you store the current timestamp and a hash(!) of the random number in your database. Do not store the actual number. This would allow anybody to log-in as any user once they've gained access to your database. The random number is equivalent to a password, so it must be protected just as well.
• When a user sends you a remember-me cookie, you hash the value and check if it's a valid. The hash must be present in your database, and it must not be older than, say, 30 days (if that's the time limit of the remember-me feature).
• The hash also tells you the user, so you go through the standard log-in procedure which creates a session etc.
• When the user logs out, you delete the hash in the database and ask the user's browser to delete the remember-me cookie.
• IP checks like you did in your code make no sense, because users typically change their IP address very often. This would make the entire feature useless.

Sounds complicated? The implementation will be even more complicated. Like I said, avoid this feature unless you cannot live without it.

[NotionCommotion]

Merry Christmas

 

Store the hash in the users table?  Will overwrite if user uses two PCs, but I suppose that is acceptable.

[Jacques1]

It's necessary.

 

A single remember-me session which is valid for 30 days is bad enough. Starting two, three, four, ... of them at the same time is suicidal, especially when you consider the naivity of many users. For example, people happily use HTTP over a public Wi-Fi without realizing that anybody with access to the same network can read their entire traffic (which includes the remember-me cookies).

 

The whole remember-me approach is actually rather poor. This should be done client-side. For example, KeePass has an auto-type feature which copies the password into the right field and submits the log-in form when you press a certain key sequence. Some plugins are even more comfortable and automatically fill out the log-in form as soon as you visit the page.

 

Why not focus on those secure alternatives instead of promoting insecure behaviour?

[hansford]

Jacques1 is way on the mark here and it's serious business in our profession.

I've put together registration and login systems, but I'm never sure if it's secure or even makes sense in a professional opinion.

It's just one of those areas that we really need to research and get other's opinions on before we are set on any given system.

 

If you look in the user's table you will find a human-readable username, but the password is some jacked up long hash.

 

$2a$07$zUEQ7QtcEwO6NXSNQLpncOcDksTfykrtfhseVotDH4sEMxlfD9kJO

 

When authenticating I grab the salt from the users table and the password supplied by the login form and pass it to a function.

$pass = $_POST['password'];    
    
// authenticate password
$row = $stm->fetch(PDO::FETCH_ASSOC);
$salt = $row['salt'];
$hashed_pass = generate_hash($pass, $salt);

if ($hashed_pass === $row['password'])
{
   // password is authentic
}

function generate_hash($password, $salt, $rounds = 7)
{
    // crypt might not be the best approach for this anymore
    return crypt($password, sprintf('$2a$%02d$', $rounds) . $salt);
}

[Jacques1]

It might make sense to discuss the password hashing topic in a separate thread ...

 

Either way, using crypt() directly is not recommended and rarely works out. Unfortunately, your code is no exception:

• You have no error checks in place, which means you keep going even if bcrypt has failed. In the worst case, you'll end up with no password check at all.
• Generating and encoding the salt is also a common source for errors. Are you sure you have 16 strong random bytes encoded with that special Base64 variant used by crypt()?
• It makes no sense to store the salt in a separate column, because it's already included in the bcrypt “hash”. The output of bcrypt contains all hash parameters (algorithm identifier, cost, salt) together with the actual hash:
  
  
   
• A cost factor of 7 is on the very low end. You'll want something around 14 on current hardware. A common recommendation is to start with 10 and then increase the cost until the hash calculation takes around 1 second.
• Your code doesn't support increasing the cost during operation – but that's actually the whole point of bcrypt. The cost should be in a configuration file, and whenever it's changed, the current hashes should be updated as the users log in.
• The “2a” prefix is obsolete. There was a critical bug in the low-level bcrypt implementation, so a new prefix was introduced for the updated version: “2y”.

 

Long story short, don't use crypt() directly. Since PHP 5.5, there's a new Password Hashing API built on top of crypt(). If you have an older version, you can use the compatibility library from the same author.

Edited December 26, 2014 by Jacques1

[hansford]

Thanks Jacques. I actually counted on your critique so I could update this old code.

I'll open a new thread once I write some new code and allow you guys to chip away the cracks.

[NotionCommotion]

The cost should be in a configuration file, and whenever it's changed, the current hashes should be updated as the users log in.

 

Implying that the application should update the users hashed password in the database every time they log in?  I've never done so, but it seems to be a good idea.  Not only will the cost be updated, but should there be any bugs in the algorithm, the hashes will be updated, and furthermore the algorithm could be updated if changed.  Do you recommend using PASSWORD_DEFAULT (which is currently the same as PASSWORD_BCRYPT and CRYPT_BLOWFISH)?  If so, what datatype would you recommend in the database?  http://php.net/manual/en/function.password-hash.php recommends 255 characters.  Would this be varchar(255), char(255), or maybe some sort of binary?

 

Would the flow be the following?  Note my  inline question regarding trimming and removing of invalid characters in username and password.

<?php
//GIVEN: $POST=array('username'=>'john.doe','password'=>'my password')

//Maybe don't do this as whitespace should be acceptable in username and password?
//Also, are there any characters which should be removed from either username or password?
$username=trim($_POST['username']);
$password=trim($_POST['password']);

//Actually use atomic counter as described http://forums.phpfreaks.com/topic/293061-validate-username-and-password/?p=1499458
$sql='SELECT hash FROM users WHERE username=?';
$stmt=db::db()->prepare($sql);
$stmt->execute(array($username));
if(password_verify ($password, $stmt->fetchColumn() )) {
    $cost=12;   //Actually get from configuration file
    $hash=password_hash($password, PASSWORD_DEFAULT, ['cost' => $cost]);
    $sql='UPDATE users SET hash=? WHERE username=?';
    $stmt=db::db()->prepare($sql);
    $stmt->execute(array($hash,$username));
    //redirect as applicable
}
else {
    //display invalid username/password error and display login prompt
}
?>

[Jacques1]

It makes no sense to use a concrete cost factor for the generic PASSWORD_DEFAULT, because the underlying algorithm is unspecified and may change at any time. For example, one of the next PHP releases might switch to scrypt, and then your cost factor of 12 isn't even valid. scrypt has three hash parameters, so a plain integer will blow up the code.

 

It's either-or: Either you rely on the default settings, or you set a concrete algorithm with its specific parameters (like a single cost factor in the case of bcrypt). This depends on whether or not there's somebody to adjust the configuration as time goes by. If you're going to maintain the software throughout its lifetime, then definitely fine-tune the hash settings and update them whenever necessary. This will provide optimal security for your specific hardware. But if the application will be written once and never be touched again, use PASSWORD_DEFAULT and let the PHP core devs take care of the hash parameters. Of course the default values are necessarily weak, because they need to work on any platform.

 

If you go with PASSWORD_DEFAULT, I'd use a TEXT column. That's more than enough for any output length now and in the future. Either way, use password_needs_rehash() to check whether a hash must be updated.

 

The password must not be trimmed or altered in any way. Hash it as is. Every byte counts, including spaces, exotic Unicode characters and whatnot. The only limitation is that the password must not be longer than 56 bytes, because that's the input limit of bcrypt.

 

With usernames, it's a very different story. They identify accounts, so it's very important that they're unambiguous. Allowing arbitrary whitespace or Unicode characters would be a bad idea, because you may end up with similar-looking names. In fact, an attacker could exploit the similarity of certain characters to impersonate, say, an admin.

 

It's best to stick to the printable ASCII chars and add some rules:

• no case-sensitivity; if “John” is already taken, you can't have “john”
• similar-looking characters are considered equal (e. g. the uppercase “i” and the lowercase “L”)
• at least one non-whitespace character
• no whitespace left or right, only single space characters in between; you may correct this automatically

 

 

 

For the general logic of the log-in, I'd use something like the following:

<?php

require_once __DIR__ . '/include/basic.php';



$database = get_database_connection();

if (isset($_POST['user_name'], $_POST['password']))
{
    $user_data_stmt = $database->prepare('
        SELECT
            user_id
            , password_hash
        FROM
            users
        WHERE
            user_name = :user_name
    ');
    $user_data_stmt->execute([
        'user_name' => $_POST['user_name'],
    ]);
    $user_data = $user_data_stmt->fetch();

    if ($user_data && password_verify($_POST['password'], $user_data['password_hash']))
    {
        echo 'Welcome!';

        // update hash if necessary
        if (password_needs_rehash($user_data['password_hash'], constant(get_config_value('password_hash_algorithm')), ['cost' => get_config_value('password_hash_cost')]))
        {
            $hash_update_stmt = $database->prepare('
                UPDATE
                    users
                SET
                    password_hash = :password_hash
                WHERE
                    user_id = :user_id
            ');
            $hash_update_stmt->execute([
                'user_id' => $user_data['user_id'],
                'password_hash' => password_hash($_POST['password'], constant(get_config_value('password_hash_algorithm')), ['cost' => get_config_value('password_hash_cost')]),
            ]);
        }
    }
    else
    {
        client_error('Incorrect username or password.');
    }
}
else
{
    client_error('Parameter "user_name" or "password" is missing.');
}

Note that the above code will tell everybody whether or not the username exists, even if the message just says “wrong username or password”. The hash computation takes a long time and is only done if the username has been found, so that's a clear indicator. If you want to hide the usernames, you'll need a much more complicated control flow to eliminate obvious time differences. But even then it's probably not feasible.

[NotionCommotion]

Thank you Jacques1,  Good explanation.

 

If usernames are case insensitive, why worry about needing similar-looking characters to be considered equal?

 

In regards to hiding the fact that a username exists, wouldn't the only users who witness the long hash computation time be those who successfully logged in with their credentials?  I suppose I can do a fake hash for non-authenticated users, but doesn't seem to bring any value.  Also, based on my general webbrowsing experience, some requests just take longer due to server load, network traffic, etc.  How can one attribute a delay to a specific cause such as a password hash?

[Jacques1]

If usernames are case insensitive, why worry about needing similar-looking characters to be considered equal?

 

What do you mean? Case insensitivity doesn't help when two different characters look similar. For example “I” (uppercase “i”) and “l” (lowercase “L”).

 

 

 

In regards to hiding the fact that a username exists, wouldn't the only users who witness the long hash computation time be those who successfully logged in with their credentials?  I suppose I can do a fake hash for non-authenticated users, but doesn't seem to bring any value.  Also, based on my general webbrowsing experience, some requests just take longer due to server load, network traffic, etc.  How can one attribute a delay to a specific cause such as a password hash?

 

The delay of the hash computation is massive, it's usually supposed to be a full second. So anybody can easily see this in their browser: If they get a response immediately, the hash calculation has been skipped; if they always have to wait for one second when they enter a specific username, they can be pretty sure it's due to the calculation.

 

Sure, you can do a dummy calculation, but that just means the time differences get more subtle. The control flow of the script is still different depending on whether or not the username exists. Yes, this can be measured, even over an Internet connection. You just need to take lots and lots of samples to filter out the noise. And there may be other information leaks as well, for example a certain error which is only possible in a certain branch.

 

Personally, I've given up trying to conceil the accounts. I simply go with public usernames (instead of e-mail addresses).