6
\$\begingroup\$

I'm trying to improve the security for my backend:

/**
 *  This is the script that is executed when I add a new user - after the input has passed the validation.
 */
    $salt = base64_encode(mcrypt_create_iv(24,MCRYPT_DEV_URANDOM));  //  generate a random 32 character salt
    $hash = hash('sha256',$salt.$_POST['secret']);  //  create a 64 character hash based on the salt and the password

    $ins = $con['site']->prepare('INSERT INTO users (handle, salt, hash) VALUES (:handle, :salt, :hash)');
    $ins->execute(array(':handle'=>$_POST['handle'], ':salt'=>$salt, ':hash'=>$hash));
    if($ins){
        KD::addNotice('success','...message...');
    } else {
        KD::addNotice('error','...message...');
    }

/**
 *  This is the script that is executed when the user is logging in - also after the input has passed the validation.
 */
    $qry = $con['site']->prepare('SELECT handle, salt, hash FROM users WHERE handle = :handle');
    $qry->execute(array(':handle'=>$_POST['handle']));  //  look up the handle (username)
    if($qry->rowCount()==1){
        $get = $qry->fetch();
        if(hash('sha256',$get['salt'].$_POST['secret'])==$get['hash']){
            KD::addNotice('success','...message...');
            session_regenerate_id();
            $_SESSION['backend']->login($get['handle']);
        } else {
            KD::addNotice('error','...message...');
        }
    } else {
        KD::addNotice('error','...message...');
    }

This is as far as I've gotten and it works. I'm generating an unpredictable salt, creating a hash and saving it to a database. Although this is an improvement from what it used to be, I'm fully aware of that there's more work to be done here.

I've been reading a bit about iterations. Or, rehashing the password, or the salt, or the hash, a couple of thousand times in order to prevent different forms of attacks. Could some of you please tell me how something like that could be implemented into my code?

Is it just putting some part of the code inside a for-loop when creating and verifying the password?

\$\endgroup\$
1

4 Answers 4

11
\$\begingroup\$

I'm no expert on security, but I believe this is not current best practice. I'll copy from an another answer of mine.

Cryptographic Right Answers (2009, Colin Percival, author of scrypt)

Password handling: As soon as you receive a password, hash it using scrypt or PBKDF2 and erase the plaintext password from memory.

Do NOT store users' passwords. Do NOT hash them with MD5. Use a real key derivation algorithm. PBKDF2 is the most official standard; but scrypt is stronger. Please keep in mind that even if YOUR application isn't particularly sensitive, your users are probably re-using passwords which they have used on other, more sensitive, websites -- so if you screw up how you store your users' passwords, you might end up doing them a lot of harm.

How To Safely Store A Password (2010)

Use bcrypt

Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt. Use bcrypt.

...

Why Not {MD5, SHA1, SHA256, SHA512, SHA-3, etc}?

These are all general purpose hash functions, designed to calculate a digest of huge amounts of data in as short a time as possible. This means that they are fantastic for ensuring the integrity of data and utterly rubbish for storing passwords.

...

Salts Will Not Help You

It’s important to note that salts are useless for preventing dictionary attacks or brute force attacks. You can use huge salts or many salts or hand-harvested, shade-grown, organic Himalayan pink salt. It doesn’t affect how fast an attacker can try a candidate password, given the hash and the salt from your database.

Salt or no, if you’re using a general-purpose hash function designed for speed you’re well and truly effed.

How to securely hash passwords? (2013)

Conclusion

Use bcrypt. PBKDF2 is not bad either. If you use scrypt you will be a "slightly early adopter" with the risks that are implied by this expression; but it would be a good move for scientific progress ("crash dummy" is a very honourable profession).

\$\endgroup\$
3
  • 1
    \$\begingroup\$ So, it's the use of sha256 your pointing to then? Regardless of the use of mcrypt_create_iv() for the salt, the hashing method I'm using is still too vulnerable? Quick question - before I've read the links you provided - Where in my code do I insert and use the bcrypt, or PBKDF2? \$\endgroup\$
    – ThomasK
    Sep 24, 2014 at 6:52
  • 1
    \$\begingroup\$ Yes, as far as I understand it (and mentioned in the second quote) sha256 is not recommended. Sorry I can't help you with the implementation, maybe try StackOverflow. \$\endgroup\$
    – mjolka
    Sep 24, 2014 at 7:01
  • 1
    \$\begingroup\$ Salts Will Not Help You: I would disagree with this. If you have a thousand passwords, with salts an attacker can check one password at a time. Without them, they can check a thousand passwords at a time. \$\endgroup\$
    – tim
    Sep 24, 2014 at 18:59
6
\$\begingroup\$

Security - Password Hashing

I've been reading a bit about iterations. Or, rehashing the password, or the salt, or the hash, a coulple of thousand times in order to prevent different forms of attacks. Could some of you please tell me how something like that could be implemented into my code?

Is it just putting som part of the code inside a for-loop when creating and verifying the password?

Theoretically, yes. It's what for example PBKDF2 does:

PBKDF2 applies a pseudorandom function, such as a cryptographic hash, cipher, or HMAC to the input password or passphrase along with a salt value and repeats the process many times to produce a derived key, which can then be used as a cryptographic key in subsequent operations. The added computational work makes password cracking much more difficult, and is known as key stretching.

Practically, no, don't do that.

Use bcrypt, PBKDF2, or scrypt instead (check the link for a nice overview to help you decide).

So, it's the use of sha256 your pointing to then? the hashing method I'm using is still too vulnerable?

sha256 is still considered save (as in, there are no known security holes (as opposed to sha1 for example). Bruteforcing is the fastest way to attack it). The problem is that sha256 is pretty fast, so bruteforcing it is fast.

Where in my code do I insert and use the bcrypt, or PBKDF2?

Your current Code

Using Early Returns

    } else {
        KD::addNotice('error','...message...');
    }
} else {
    KD::addNotice('error','...message...');
}

I think that having these kind of else statements at the end can be a bit confusing (it's hard to see what if they are closing. With two statements it's not that bad, but when the code grows it gets a lot worse).

You could rewrite your code like this using early returns (or just use the early return for the first if statement, not the second):

$qry = $con['site']->prepare('SELECT handle, salt, hash FROM users WHERE handle = :handle');
$qry->execute(array(':handle'=>$_POST['handle']));  //  look up the handle (username)
if($qry->rowCount()!=1){
    KD::addNotice('error','...message...');
    return;
}
$get = $qry->fetch();
if(hash('sha256',$get['salt'].$_POST['secret'])!=$get['hash']){
    KD::addNotice('error','...message...');
    return;
}
KD::addNotice('success','...message...');
session_regenerate_id();
$_SESSION['backend']->login($get['handle']);

Nitpicks

  • variable names: just write the words out. $insert isn't that much longer than $ins, but a lot more readable, same goes for $qry ($query). And $get could be something like $queryResult.
  • You are consistent with your use of space, which is good. Personally, I would prefer spaces around operations such as ==, =>, and . to make the code more readable.
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I've been studying a bit more. And the code both you and several others have mentioned -- pbkdf2 -- basically uses sha256 along with hash_hmac together with an itereation. Why not sha512, or higher bytes? Is it mainly because of speed? \$\endgroup\$
    – ThomasK
    Sep 24, 2014 at 20:53
  • \$\begingroup\$ @ThomasK good question. I'm not sure, but here it is suggested that, yes, it's because of the speed advantage of an attacker. But you can always use sha512 or a list of other algorithms with hash_pbkdf2. \$\endgroup\$
    – tim
    Sep 24, 2014 at 21:05
  • \$\begingroup\$ I've updated my question with some updates that I've done to the code - based on pbkdf2... \$\endgroup\$
    – ThomasK
    Sep 24, 2014 at 23:02
4
\$\begingroup\$

In PHP 5.5+, the best practice is to use password_hash() and password_verify().

You can use it in your code like so. Notice that this obsoletes the salt column in your table.

/**
 *  This is the script that is executed when I add a new user - after the input has passed the validation.
 */
    $ins = $con['site']->prepare('INSERT INTO users (handle, hash) VALUES (:handle, :hash)');
    $ins->execute(array(':handle' => $_POST['handle'], ':hash' => password_hash($_POST['secret'], PASSWORD_BCRYPT)));

    if($ins){
        KD::addNotice('success','...message...');
    } else {
        KD::addNotice('error','...message...');
    }

/**
 *  This is the script that is executed when the user is logging in - also after the input has passed the validation.
 */
    $qry = $con['site']->prepare('SELECT handle, hash FROM users WHERE handle = :handle');
    $qry->execute(array(':handle'=>$_POST['handle']));  //  look up the handle (username)
    if($qry->rowCount()==1){
        $get = $qry->fetch();
        if (password_verify($_POST['secret'], $get['hash']) {
            KD::addNotice('success','...message...');
            session_regenerate_id();
            $_SESSION['backend']->login($get['handle']);
        } else {
            KD::addNotice('error','...message...');
        }
    } else {
        KD::addNotice('error','...message...');
    }

Notice in particular that password_hash() returns a coded string that includes the algorithm used and the salt. This means that you can store it straight into your database and not have to worry about salting and whatnot.

\$\endgroup\$
3
  • \$\begingroup\$ After a quick read through the documentation for those functions, I think this is the most helpful answer. \$\endgroup\$
    – mjolka
    Sep 25, 2014 at 3:56
  • \$\begingroup\$ Unfortunately the host doesn't offer PHP 5.5. They have 5.2, 5.3 and 5.4. (I did, however, just discover a simplified version of the password_hash and password_verify by Anthony Ferrara (from 2012 I believe). Don't know if it valid though). \$\endgroup\$
    – ThomasK
    Sep 25, 2014 at 6:55
  • \$\begingroup\$ @ThomasK password_hash is just the PHP implementation of bcrypt. For further alternatives to this for PHP version < 5.5 see the link How to use bcrypt in my answer. \$\endgroup\$
    – tim
    Sep 25, 2014 at 14:45
3
\$\begingroup\$

I've gone trough a lot of documentations and stuff to get a feel for this hashing jungle, and done some adjustments to my code along the way.

I've created a function - which is just a stripped down modification of the pbkdf2 suggested multiple times on Stack Overflow.

function pbkdf2($algorithm,$salt,$password,$iterations,$hash_bytes){
    /*  I've removed the checks at the beginning, along with the  if (function_exists("hash_pbkdf2"))  */
    $hash_length = strlen(hash($algorithm, "", true));
    $block_count = ceil($hash_bytes/$hash_length);
    $output = '';
    for($i=1; $i<=$block_count; $i++){
        $last = $salt.pack("N", $i);  //  $i encoded as 4 bytes, big endian.
        $last = $xorsum = hash_hmac($algorithm, $last, $password, true);  //  first iteration
    #   perform the other $iterations - 1 iterations
        for($j=1; $j<$iterations; $j++){
            $xorsum ^= ($last = hash_hmac($algorithm, $last, $password, true));
        }
    //
        $output .= $xorsum;
    }
    /*  I also removed the  $raw_format  variable  */
    return base64_encode(substr($output, 0, $hash_bytes));
}

In addition, I'm taking advantage of this one as well when comparing the hashes on login:
well explained here

function slow_equals($a, $b){
    $diff = strlen($a) ^ strlen($b);
    for($i=0; $i<strlen($a) && $i<strlen($b); $i++){
        $diff |= ord($a[$i]) ^ ord($b[$i]);
    }
    return $diff === 0;
}

The adjustments I've made is basically that I've switched

$hash = hash('sha256',$salt.$_POST['secret']);  //  create a 64 character hash based on the salt and the password

out with

$hash = pbkdf2($algorithm, $salt, $_POST['secret'], $iterations, $hash_bytes);  //  create a hash based on the salt and the password

when adding a new user (first script).

Then, inside the second script, where the user is logging in, I'm matching the password/hashes using slow_equals() instead of just is this string equal to the other one? (==) ...

if(slow_equals(pbkdf2($algorithm, $get['salt'], $_POST['secret'], $iterations, $hash_bytes), $get['hash'])){
/*  ...  */
}

My "settings" for the pbkdf2()-function is as follows:

$algorithm = 'sha512';  //  don't really know which algorithm to use
$iterations = 5000;  //  seemed like a good fit
$hash_bytes = 48;  //  produces a 64 character long string

Here are the two scipts with the mentioned adjustments:

/**
 *  This is the script that is executed when I add a new user - after the input has passed the validation.
 */
    $salt = base64_encode(mcrypt_create_iv($hash_bytes,MCRYPT_DEV_URANDOM));  //  generate a random 32 character salt
    $hash = pbkdf2($algorithm, $salt, $_POST['secret'], $iterations, $hash_bytes);  //  create a hash based on the salt and the password

    $ins = $con['site']->prepare('INSERT INTO users (handle, salt, hash) VALUES (:handle, :salt, :hash)');
    $ins->execute(array(':handle'=>$_POST['handle'], ':salt'=>$salt, ':hash'=>$hash));
    if($ins){
        KD::addNotice('success','...message...');
    } else {
        KD::addNotice('error','...message...');
    }

/**
 *  This is the script that is executed when the user is logging in - also after the input has passed the validation.  
 *  Changed to "early returns" as suggested by @tim
 */
    $qry = $con['site']->prepare('SELECT handle, salt, hash FROM _site_users WHERE BINARY handle = :handle');
    $qry->execute(array(':username'=>$_POST['handle']));
    if($qry->rowCount()!=1){
        KD::addNotice('error','...message...');
        return;
    }
    $get = $qry->fetch();
    if(slow_equals(pbkdf2($algorithm, $get['salt'], $_POST['secret'], $iterations, $hash_bytes), $get['hash'])){
        KD::addNotice('success','...message...');
        session_regenerate_id();
        $_SESSION['backend']->login($get['handle']);
    } else {
        KD::addNotice('error','...message...');
    }

Orginal code

\$\endgroup\$
5
  • \$\begingroup\$ PHP actually has a native compare function that is save from timing attacks. And I would use the native PHP hash_pbkdf2 if it exists (you can provide your custom implementation if it is not available, but if it is, you might as well use it). \$\endgroup\$
    – tim
    Sep 24, 2014 at 23:26
  • \$\begingroup\$ The hash_pbkdf2 was not available in my php version on the server. And the host is not willing to update.. That's why I decided to remove that part of the code.. (so I'm guessing "my" pbkdf2 funcition is sort of a custom implementation)... Also, I was not aware of the hash_equals, but ´function_exists`returned false unfortunately.. \$\endgroup\$
    – ThomasK
    Sep 24, 2014 at 23:32
  • 1
    \$\begingroup\$ We cannot review code in an answer. If you'd like further review, post a new follow-up question. \$\endgroup\$
    – Jamal
    Sep 25, 2014 at 2:27
  • \$\begingroup\$ Are you suggesting that I put this answer into a new question in order to get feedback/get my updated code reviewed? with a backlink, or something, to this originale question? \$\endgroup\$
    – ThomasK
    Sep 25, 2014 at 6:57
  • \$\begingroup\$ @ThomasK Yes, that would be appropriate. \$\endgroup\$ Sep 25, 2014 at 8:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.