diff --git a/src/main/java/fr/xephi/authme/security/HashUtils.java b/src/main/java/fr/xephi/authme/security/HashUtils.java index 3578c80f3..642081c6d 100644 --- a/src/main/java/fr/xephi/authme/security/HashUtils.java +++ b/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. issue #1561. + * + * @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. * diff --git a/src/main/java/fr/xephi/authme/security/crypts/BCrypt2y.java b/src/main/java/fr/xephi/authme/security/crypts/BCrypt2y.java index cf4807abc..a22a68906 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/BCrypt2y.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Joomla.java b/src/main/java/fr/xephi/authme/security/crypts/Joomla.java index 462f5cb28..2ecc1d8d3 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Joomla.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Md5vB.java b/src/main/java/fr/xephi/authme/security/crypts/Md5vB.java index c244ec49d..00656964f 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Md5vB.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/PhpBB.java b/src/main/java/fr/xephi/authme/security/crypts/PhpBB.java index 70ac322d0..2d641706c 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/PhpBB.java +++ b/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. *
@@ -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 } } } diff --git a/src/main/java/fr/xephi/authme/security/crypts/SeparateSaltMethod.java b/src/main/java/fr/xephi/authme/security/crypts/SeparateSaltMethod.java index d0dacda4d..c0ec13dd7 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/SeparateSaltMethod.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Sha256.java b/src/main/java/fr/xephi/authme/security/crypts/Sha256.java index 1b77a2e44..ce6b25492 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Sha256.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Smf.java b/src/main/java/fr/xephi/authme/security/crypts/Smf.java index 24d28fe6c..e24c1b83d 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Smf.java +++ b/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. *
@@ -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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/UnsaltedMethod.java b/src/main/java/fr/xephi/authme/security/crypts/UnsaltedMethod.java index a8f2040e5..33815ec77 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/UnsaltedMethod.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/UsernameSaltMethod.java b/src/main/java/fr/xephi/authme/security/crypts/UsernameSaltMethod.java index 23101e22a..f5930fcf5 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/UsernameSaltMethod.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Wbb4.java b/src/main/java/fr/xephi/authme/security/crypts/Wbb4.java index d1d4953d1..f396c5d84 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Wbb4.java +++ b/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 diff --git a/src/main/java/fr/xephi/authme/security/crypts/Wordpress.java b/src/main/java/fr/xephi/authme/security/crypts/Wordpress.java index 768b92c5d..f70c09496 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/Wordpress.java +++ b/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); } } diff --git a/src/main/java/fr/xephi/authme/security/crypts/XAuth.java b/src/main/java/fr/xephi/authme/security/crypts/XAuth.java index 9f921b6ae..62f2e0d71 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/XAuth.java +++ b/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 diff --git a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java b/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java index fc9fd0d66..e59dae647 100644 --- a/src/test/java/fr/xephi/authme/security/HashAlgorithmIntegrationTest.java +++ b/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 diff --git a/src/test/java/fr/xephi/authme/security/HashUtilsTest.java b/src/test/java/fr/xephi/authme/security/HashUtilsTest.java index 5c1fda220..440e748a2 100644 --- a/src/test/java/fr/xephi/authme/security/HashUtilsTest.java +++ b/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)); + } }