Permalink
Browse files

[Security] changed the way passwords are compared to avoid timing att…

…acks
  • Loading branch information...
fabpot committed Oct 21, 2010
1 parent cb0f63f commit 0749038e73b0def26abfefa5cc40f30683c7b460
@@ -63,4 +63,29 @@ protected function mergePasswordAndSalt($password, $salt)
return $password.'{'.$salt.'}';
}
/**
* Compares two passwords.
*
* This method implements a constant-time algorithm to compare
* passwords to avoid (remote) timing attacks.
*
* @param string $password1 The first password
* @param string $password2 The second password
*
* @return Boolean true if the two passwords are the same, false otherwise
*/
protected function comparePasswords($password1, $password2)
{
if (strlen($password1) !== strlen($password2)) {

This comment has been minimized.

@rande

rande Oct 21, 2010

This will allow to find the password length with the timing attacks algorithm. Isn't it ?

This comment has been minimized.

@fabpot

fabpot Oct 21, 2010

Owner

yes. The implementation is not mine. This is the same implementation as done by MANY software.

This comment has been minimized.

@naneau

naneau Oct 21, 2010

The length of the hashed version of the password isn't relevant in this case, it tells you nothing about the original password. The hashing function would generate strings of equal length. The only information an attacker could gain from that is how difficult it would be to brute force it.

This comment has been minimized.

@jmather

jmather Oct 23, 2010

Adding to that, it seems the most common hashing algos (md5, sha1) are length consistent, so concern about leaking length is really more a matter of ensuring your algos have fixed length (if you're worried about it) . It is the standard implementation though, Fabian has a good point there.

This comment has been minimized.

@schmittjoh

schmittjoh Oct 23, 2010

As noted below, this whole method is not really relevant except for plaintext password comparison.

When there is hashing and a salt involved, the response time will be no indication whatsoever as to how far the submitted password is correct.

return false;
}
$result = 0;
for ($i = 0; $i < strlen($password1); $i++) {
$result |= ord($password1[$i]) ^ ord($password2[$i]);
}
return 0 === $result;
}
}
@@ -56,6 +56,6 @@ public function encodePassword($raw, $salt)
*/
public function isPasswordValid($encoded, $raw, $salt)
{
return $encoded === $this->encodePassword($raw, $salt);
return $this->comparePasswords($encoded, $this->encodePassword($raw, $salt));
}
}
@@ -41,9 +41,9 @@ public function isPasswordValid($encoded, $raw, $salt)
$pass2 = $this->mergePasswordAndSalt($raw, $salt);
if (!$this->ignorePasswordCase) {
return $encoded === $pass2;
return $this->comparePasswords($encoded, $pass2);
} else {
return strtolower($encoded) === strtolower($pass2);
return $this->comparePasswords(strtolower($encoded), strtolower($pass2));
}
}
}

5 comments on commit 0749038

@pghoratiu

This comment has been minimized.

pghoratiu replied Oct 21, 2010

Timing attacks make sense if you compare plain text passwords. If you compare hashes of passwords it does not make much sense cause a single character change in the password will generate a completely different hash and this information (the comparison time) does not help an attacker.

@fabpot

This comment has been minimized.

@pghoratiu

This comment has been minimized.

pghoratiu replied Oct 21, 2010

I think the article refers to the situation when the attacker has the possibility to inject the comparison term directly and it works in the following situations:
attackerPlainText !== serverPlainText
atackerHash !== serverHash

It does not work with:
hash(atackerPlainText) !== serverHash
cause the attacker has no control over the output of hash(atackerPlainText).

The only situation where this would apply is the session cookie hash but in that case the hash code is searched either in a database or a hash table or the filesystem and it's a PHP issue.

BTW good job with Symfony framework, much appreciated!

@naneau

This comment has been minimized.

naneau replied Oct 21, 2010

The issue described in the post is about HMAC's, i.e. you need to generate a hash that you (or somebody else who knows a secret string) can use to check whether a message is authentic.

The situation is hash(message + secret) == HASH. The attacker, using the timing attack described, can check positions in the resulting hash string separately instead of combined for validity. This reduces the number of options to check by a huge factor. This means that he can much more easily generate HASH. Instead of going through all possible options, he only has to check each position, reducing the brute force time a lot. That means that he can generate a message and a security hash that would seem valid.

@naneau

This comment has been minimized.

naneau replied Oct 21, 2010

The question is, is this component used to generate HMAC's, where the plain text and the hash leave the server together and have to be checked combined on their return? I.e. can an attacker generate their own plain text message and guess the hash much faster than a pure brute force attack. This is not an issue when it's purely about checking a password for a user during login, which pghoratiu seems to be referring to. It's only an issue when the attacker needs to send the resulting hash itself to the server for comparison.

When I see this component I might use it for the generation of HMAC's (it's not at all indicated that I shouldn't do so), so I still think this fix is highly relevant, even if it isn't used as such internally.

Please sign in to comment.