4
\$\begingroup\$

This is a follow-up question to Iteration of password hashing in PHP

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.

PBKDF2 seemed like a good fit, so that's wath I went for.
Also, it's important to know that I don't have access to PHP 5.5 with my current host. Only 5.2, 5.3 and 5.4 is available. And thy won't let me update it either...

I've created a function - which is just a stripped down modification of the pbkdf2 suggested multiple times on Stack Overflow. (please see links at the bottom).

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 (original question here) 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...');
    }

Any opinions regarding this updated code? Also, have I in any way made the pbkdf2 vulnerable with the modifications I've done?

Links:

\$\endgroup\$
3
\$\begingroup\$

Early returns in this case means that in theory, an attacker could execute a timing attack to enumerate usernames (since, if the name is present, it has to calculate a hash, while it does not have to hash something when the name is not present). In this case, you should use a prepared combination of salt, password and hash to simulate the hashing and comparing of the password. That way, the hashing will always happen and there is less discrepancy in length between authenticating a user and a non-user.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That makes sense, and I didn't think of that. Allthough the username is not that secret, people are using the same username/nickname, on multiple site, in combination with thier favourite password... But what you are suggesting is to compare both the handle and the password at once? (... WHERE BINARY handle = :handle AND salt = :salt). I fail to see how since the salt is always randomly generated... \$\endgroup\$ – ThomasK Sep 25 '14 at 11:44
  • 2
    \$\begingroup\$ No, what I'm saying is: use the same code. When you check for rowcount, and it's not 1, you use a dummy password + salt, run that through your hashing algorithm you're using by default and compare that to the correct hardcoded answer for that hashing combination. Example: if the hash of password 'cat' and salt 'dog' is 'mouse', you run hash('cat', 'dog') and compare that to 'mouse'. That way, your authentication always does a hashing procedure. There is still a small timing difference due to session_regenerate_id() and login($get['handle']);, but that's harder to get rid of. \$\endgroup\$ – Nzall Sep 25 '14 at 11:55
  • \$\begingroup\$ I see. I've just read that even a fraction of a millisecond is detectable, and that an pattern can be learnd from that. So, with that in mind, it wouldn't really matter if the time difference was just a couple of microseconds anyway? Also, the answer by @KIKO Software would probably eliminate the need for simulating a success login? But I might be missing something crucial.. \$\endgroup\$ – ThomasK Sep 26 '14 at 22:03
3
\$\begingroup\$

Timing attacks

I noticed you use slow_equals() to prevent timing attacks. This is a relatively complicated function. Why not use the normal == and combine it with:

usleep(rand(0,10000));

This is much easier to understand, and leaves your server free to do other things. You can (almost) ignore the fact that the rand() function is not pefect, it's good enough for this job. You can occasionally seed it, if you're worried a hacker might compensate for the 'random' sequence. I would like to meet that hacker! Nevertheless this is better:

function sleepRandom()
{
  if (rand(0,10) == 1) srand((double)microtime()*1000000); 
  usleep(rand(0,10000));
}

Once every 10 page calls will be quite sufficient.

You should realise that this simple solution might even be better than the solution you use now. slow_equals() only hides one thing, this solution hides all timing issues in your code.

Don't be fooled by people saying you should not think about your code, and argue you should only use theirs. If everybody did that, it would be a field day for the hackers.

Please choose the delay at least 5 times the normal time PHP takes to execute the script. If you make it too small the execution time still largely depends on what the script does. You can use two microtime(TRUE)'s, at the start and the end of the script to measure how long it takes.

$start    = microtime(TRUE);
<.... all your code ....>
$finish   = microtime(TRUE);
$exectime = $finish-$start;

Do not use the browser to measure execution time, since it cannot. A normal script, in a framework, should execute in 10 - 20 milliseconds. This does, of course, depend heavily on the speed of your server, but if your login code takes more than 100 milliseconds to execute it is seriously inefficient.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The slow_equals, to my understanding, does not just delay the execution time. It does so by turning each character in the string into ASCII, and compare them that way. But to be honest. I'm not that sure about how much that increases the security. If the only reason for doing so is do delay the execution time - I suppose the solution you provided is just as good. \$\endgroup\$ – ThomasK Sep 26 '14 at 17:54
  • \$\begingroup\$ slow_equals() is slower, no doubt, but what it does is make each hash comparison last an equal amount of time. This way no hacker cannot detect when a hash did not match. The problem is that this also leaves your code open for other types of timing attacks. By having, instead, a short delay of random duration any timing attack is thwarted. It is therefore the better solution of the two. \$\endgroup\$ – KIKO Software Sep 26 '14 at 19:12
  • \$\begingroup\$ could you give an example of other types of attacks? And. I can't seem to get how to use the function you suggested in my code. But I'm guessing it is within the comparison of the two hashes. \$\endgroup\$ – ThomasK Sep 26 '14 at 20:13
  • \$\begingroup\$ Just add the short random delay anywhere to the PHP script accepting the login and use the uncomplicated == to compare the hashes. Examples are really not useful. The problem is with timing attacks I, or you, or anybody else, has thought about, but a hacker has. \$\endgroup\$ – KIKO Software Sep 26 '14 at 20:35
  • 1
    \$\begingroup\$ "Why not use the normal == and combine it with: usleep(rand(0,10000));" No. This is not sufficient. \$\endgroup\$ – Scott Arciszewski Nov 4 '14 at 22:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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