#1561 Fix timing attacks by comparing hashes in constant time (#1563)

* #1561 Fix timing attacks by comparing hashes in constant time

* #1561 Fix timing attacks in phpBB fallback hashes
- As noted by @games647

src/main/java/fr/xephi/authme/security/HashUtils.java

@@ -1,6 +1,7 @@
 package fr.xephi.authme.security;
 
 import java.math.BigInteger;
+import java.nio.charset.StandardCharsets;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 
@@ -78,6 +79,20 @@ public final class HashUtils {
         return hash.length() > 3 && hash.substring(0, 2).equals("$2");
     }
 
+    /**
+     * Checks whether the two strings are equal to each other in a time-constant manner.
+     * This helps to avoid timing side channel attacks,
+     * cf. <a href="https://github.com/AuthMe/AuthMeReloaded/issues/1561">issue #1561</a>.
+     *
+     * @param string1 first string
+     * @param string2 second string
+     * @return true if the strings are equal to each other, false otherwise
+     */
+    public static boolean isEqual(String string1, String string2) {
+        return MessageDigest.isEqual(
+            string1.getBytes(StandardCharsets.UTF_8), string2.getBytes(StandardCharsets.UTF_8));
+    }
+
     /**
      * Hash the message with the given algorithm and return the hash in its hexadecimal notation.
      *

src/main/java/fr/xephi/authme/security/crypts/BCrypt2y.java

@@ -3,6 +3,8 @@ package fr.xephi.authme.security.crypts;
 import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 @Recommendation(Usage.RECOMMENDED)
 public class BCrypt2y extends HexSaltedMethod {
 
@@ -23,7 +25,7 @@ public class BCrypt2y extends HexSaltedMethod {
         // The salt is the first 29 characters of the hash
 
         String salt = hash.substring(0, 29);
-        return hash.equals(computeHash(password, salt, null));
+        return isEqual(hash, computeHash(password, salt, null));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Joomla.java

@@ -4,6 +4,8 @@ import fr.xephi.authme.security.HashUtils;
 import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 @Recommendation(Usage.ACCEPTABLE)
 public class Joomla extends HexSaltedMethod {
 
@@ -16,7 +18,7 @@ public class Joomla extends HexSaltedMethod {
     public boolean comparePassword(String password, HashedPassword hashedPassword, String unusedName) {
         String hash = hashedPassword.getHash();
         String[] hashParts = hash.split(":");
-        return hashParts.length == 2 && hash.equals(computeHash(password, hashParts[1], null));
+        return hashParts.length == 2 && isEqual(hash, computeHash(password, hashParts[1], null));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Md5vB.java

@@ -1,5 +1,6 @@
 package fr.xephi.authme.security.crypts;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
 import static fr.xephi.authme.security.HashUtils.md5;
 
 public class Md5vB extends HexSaltedMethod {
@@ -13,7 +14,7 @@ public class Md5vB extends HexSaltedMethod {
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
         String hash = hashedPassword.getHash();
         String[] line = hash.split("\\$");
-        return line.length == 4 && hash.equals(computeHash(password, line[2], name));
+        return line.length == 4 && isEqual(hash, computeHash(password, line[2], name));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/PhpBB.java

@@ -10,6 +10,8 @@ import fr.xephi.authme.security.crypts.description.Usage;
 import java.io.UnsupportedEncodingException;
 import java.security.MessageDigest;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 /**
  * Encryption method compatible with phpBB3.
  * <p>
@@ -43,7 +45,7 @@ public class PhpBB implements EncryptionMethod {
         } else if (hash.length() == 34) {
             return PhpassSaltedMd5.phpbb_check_hash(password, hash);
         } else {
-            return PhpassSaltedMd5.md5(password).equals(hash);
+            return isEqual(hash, PhpassSaltedMd5.md5(password));
         }
     }
 
@@ -153,7 +155,7 @@ public class PhpBB implements EncryptionMethod {
         }
 
         private static boolean phpbb_check_hash(String password, String hash) {
-            return _hash_crypt_private(password, hash).equals(hash);
+            return isEqual(hash, _hash_crypt_private(password, hash)); // #1561: fix timing issue
         }
     }
 }

src/main/java/fr/xephi/authme/security/crypts/SeparateSaltMethod.java

@@ -1,5 +1,7 @@
 package fr.xephi.authme.security.crypts;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 /**
  * Common supertype for encryption methods which store their salt separately from the hash.
  */
@@ -19,7 +21,7 @@ public abstract class SeparateSaltMethod implements EncryptionMethod {
 
     @Override
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
-        return hashedPassword.getHash().equals(computeHash(password, hashedPassword.getSalt(), null));
+        return isEqual(hashedPassword.getHash(), computeHash(password, hashedPassword.getSalt(), null));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Sha256.java

@@ -3,6 +3,7 @@ package fr.xephi.authme.security.crypts;
 import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
 import static fr.xephi.authme.security.HashUtils.sha256;
 
 @Recommendation(Usage.RECOMMENDED)
@@ -14,10 +15,10 @@ public class Sha256 extends HexSaltedMethod {
     }
 
     @Override
-    public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) {
+    public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
         String hash = hashedPassword.getHash();
         String[] line = hash.split("\\$");
-        return line.length == 4 && hash.equals(computeHash(password, line[2], ""));
+        return line.length == 4 && isEqual(hash, computeHash(password, line[2], name));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Smf.java

@@ -7,6 +7,8 @@ import fr.xephi.authme.security.crypts.description.SaltType;
 import fr.xephi.authme.security.crypts.description.Usage;
 import fr.xephi.authme.util.RandomStringUtils;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 /**
  * Hashing algorithm for SMF forums.
  * <p>
@@ -32,7 +34,7 @@ public class Smf implements EncryptionMethod {
 
     @Override
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
-        return computeHash(password, null, name).equals(hashedPassword.getHash());
+        return isEqual(hashedPassword.getHash(), computeHash(password, null, name));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/UnsaltedMethod.java

@@ -5,6 +5,8 @@ import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.SaltType;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 /**
  * Common type for encryption methods which do not use any salt whatsoever.
  */
@@ -26,7 +28,7 @@ public abstract class UnsaltedMethod implements EncryptionMethod {
 
     @Override
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
-        return hashedPassword.getHash().equals(computeHash(password));
+        return isEqual(hashedPassword.getHash(), computeHash(password));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/UsernameSaltMethod.java

@@ -5,6 +5,8 @@ import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.SaltType;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 /**
  * Common supertype of encryption methods that use a player's username
  * (or something based on it) as embedded salt.
@@ -23,7 +25,7 @@ public abstract class UsernameSaltMethod implements EncryptionMethod {
 
     @Override
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
-        return hashedPassword.getHash().equals(computeHash(password, name).getHash());
+        return isEqual(hashedPassword.getHash(), computeHash(password, name).getHash());
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Wbb4.java

@@ -3,6 +3,7 @@ package fr.xephi.authme.security.crypts;
 import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
 import static fr.xephi.authme.security.crypts.BCryptService.hashpw;
 
 @Recommendation(Usage.RECOMMENDED)
@@ -14,12 +15,12 @@ public class Wbb4 extends HexSaltedMethod {
     }
 
     @Override
-    public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) {
+    public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
         if (hashedPassword.getHash().length() != 60) {
             return false;
         }
         String salt = hashedPassword.getHash().substring(0, 29);
-        return computeHash(password, salt, null).equals(hashedPassword.getHash());
+        return isEqual(hashedPassword.getHash(), computeHash(password, salt, name));
     }
 
     @Override

src/main/java/fr/xephi/authme/security/crypts/Wordpress.java

@@ -12,6 +12,8 @@ import java.security.MessageDigest;
 import java.security.SecureRandom;
 import java.util.Arrays;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 @Recommendation(Usage.ACCEPTABLE)
 @HasSalt(value = SaltType.TEXT, length = 9)
 // Note ljacqu 20151228: Wordpress is actually a salted algorithm but salt generation is handled internally
@@ -115,7 +117,7 @@ public class Wordpress extends UnsaltedMethod {
     public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
         String hash = hashedPassword.getHash();
         String comparedHash = crypt(password, hash);
-        return comparedHash.equals(hash);
+        return isEqual(hash, comparedHash);
     }
 
 }

src/main/java/fr/xephi/authme/security/crypts/XAuth.java

@@ -3,6 +3,8 @@ package fr.xephi.authme.security.crypts;
 import fr.xephi.authme.security.crypts.description.Recommendation;
 import fr.xephi.authme.security.crypts.description.Usage;
 
+import static fr.xephi.authme.security.HashUtils.isEqual;
+
 @Recommendation(Usage.RECOMMENDED)
 public class XAuth extends HexSaltedMethod {
 
@@ -23,14 +25,14 @@ public class XAuth extends HexSaltedMethod {
     }
 
     @Override
-    public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) {
+    public boolean comparePassword(String password, HashedPassword hashedPassword, String name) {
         String hash = hashedPassword.getHash();
         int saltPos = password.length() >= hash.length() ? hash.length() - 1 : password.length();
         if (saltPos + 12 > hash.length()) {
             return false;
         }
         String salt = hash.substring(saltPos, saltPos + 12);
-        return hash.equals(computeHash(password, salt, null));
+        return isEqual(hash, computeHash(password, salt, name));
     }
 
     @Override

src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java

@@ -2,6 +2,7 @@ package fr.xephi.authme.security;
 
 import ch.jalu.injector.Injector;
 import ch.jalu.injector.InjectorBuilder;
+import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.security.crypts.Argon2;
 import fr.xephi.authme.security.crypts.EncryptionMethod;
 import fr.xephi.authme.security.crypts.HashedPassword;
@@ -40,6 +41,7 @@ public class HashAlgorithmIntegrationTest {
         given(settings.getProperty(SecuritySettings.PBKDF2_NUMBER_OF_ROUNDS)).willReturn(10_000);
         injector = new InjectorBuilder().addDefaultHandlers("fr.xephi.authme").create();
         injector.register(Settings.class, settings);
+        TestHelper.setupLogger();
     }
 
     @Test

src/test/java/fr/xephi/authme/security/HashUtilsTest.java

@@ -123,4 +123,13 @@ public class HashUtilsTest {
         assertThat(HashUtils.isValidBcryptHash("#2ae5fc78"), equalTo(false));
     }
 
+    @Test
+    public void shouldCompareStrings() {
+        // given / when / then
+        assertThat(HashUtils.isEqual("test", "test"), equalTo(true));
+        assertThat(HashUtils.isEqual("test", "Test"), equalTo(false));
+        assertThat(HashUtils.isEqual("1234", "1234."), equalTo(false));
+        assertThat(HashUtils.isEqual("ພາສາຫວຽດນາມ", "ພາສາຫວຽດນາມ"), equalTo(true));
+        assertThat(HashUtils.isEqual("test", "tëst"), equalTo(false));
+    }
 }