From eea3697fa48952d607619176a3667891d854b8d3 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Wed, 30 Dec 2015 23:24:36 +0100 Subject: [PATCH] #364 Add test for bogus hashes - Verify that a "hash" in the wrong format doesn't throw exception (this is relevant when the supportOldPasswordHash setting is enabled) --- .../fr/xephi/authme/security/crypts/BCRYPT.java | 9 ++++++++- .../xephi/authme/security/crypts/CryptPBKDF2.java | 3 +++ .../authme/security/crypts/CryptPBKDF2Django.java | 3 +++ .../xephi/authme/security/crypts/WHIRLPOOL.java | 1 + .../fr/xephi/authme/security/crypts/XAUTH.java | 7 +++++-- .../crypts/AbstractEncryptionMethodTest.java | 15 +++++++++++++++ 6 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java index 02d68719..b7602e7f 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java +++ b/src/main/java/fr/xephi/authme/security/crypts/BCRYPT.java @@ -13,11 +13,13 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. package fr.xephi.authme.security.crypts; +import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.security.crypts.description.HasSalt; import fr.xephi.authme.security.crypts.description.Usage; import fr.xephi.authme.security.crypts.description.Recommendation; import fr.xephi.authme.security.crypts.description.SaltType; import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.util.StringUtils; import java.io.UnsupportedEncodingException; import java.security.SecureRandom; @@ -527,7 +529,12 @@ public class BCRYPT implements EncryptionMethod { @Override public boolean comparePassword(String password, HashedPassword hash, String name) { - return checkpw(password, hash.getHash()); + try { + return hash.getHash().length() > 3 && checkpw(password, hash.getHash()); + } catch (IllegalArgumentException e) { + ConsoleLogger.showError("Bcrypt checkpw() returned " + StringUtils.formatException(e)); + } + return false; } @Override diff --git a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2.java b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2.java index 4c8fe933..57c13ee6 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2.java +++ b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2.java @@ -22,6 +22,9 @@ public class CryptPBKDF2 extends HexSaltedMethod { @Override public boolean comparePassword(String password, HashedPassword hashedPassword, String unusedName) { String[] line = hashedPassword.getHash().split("\\$"); + if (line.length != 4) { + return false; + } String salt = line[2]; String derivedKey = line[3]; PBKDF2Parameters params = new PBKDF2Parameters("HmacSHA256", "ASCII", salt.getBytes(), 10000, derivedKey.getBytes()); diff --git a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java index 46fbe1ca..eb763f81 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java +++ b/src/main/java/fr/xephi/authme/security/crypts/CryptPBKDF2Django.java @@ -21,6 +21,9 @@ public class CryptPBKDF2Django extends HexSaltedMethod { @Override public boolean comparePassword(String password, HashedPassword hashedPassword, String unusedName) { String[] line = hashedPassword.getHash().split("\\$"); + if (line.length != 4) { + return false; + } String salt = line[2]; byte[] derivedKey = DatatypeConverter.parseBase64Binary(line[3]); PBKDF2Parameters params = new PBKDF2Parameters("HmacSHA256", "ASCII", salt.getBytes(), 15000, derivedKey); diff --git a/src/main/java/fr/xephi/authme/security/crypts/WHIRLPOOL.java b/src/main/java/fr/xephi/authme/security/crypts/WHIRLPOOL.java index 522fb45b..0169d192 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/WHIRLPOOL.java +++ b/src/main/java/fr/xephi/authme/security/crypts/WHIRLPOOL.java @@ -379,6 +379,7 @@ public class WHIRLPOOL extends UnsaltedMethod { } } + @Override public String computeHash(String password) { byte[] digest = new byte[DIGESTBYTES]; NESSIEinit(); 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 5c917327..18612874 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/XAUTH.java +++ b/src/main/java/fr/xephi/authme/security/crypts/XAUTH.java @@ -26,8 +26,11 @@ public class XAUTH extends HexSaltedMethod { public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) { String hash = hashedPassword.getHash(); int saltPos = (password.length() >= hash.length() ? hash.length() - 1 : password.length()); - String saltFromHash = hash.substring(saltPos, saltPos + 12); - return hash.equals(computeHash(password, saltFromHash, null)); + if (saltPos + 12 > hash.length()) { + return false; + } + String salt = hash.substring(saltPos, saltPos + 12); + return hash.equals(computeHash(password, salt, null)); } @Override diff --git a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java index b1bc0c1a..9acc3c25 100644 --- a/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java +++ b/src/test/java/fr/xephi/authme/security/crypts/AbstractEncryptionMethodTest.java @@ -32,6 +32,10 @@ public abstract class AbstractEncryptionMethodTest { private static final List INTERNAL_PASSWORDS = ImmutableList.of("test1234", "Ab_C73", "(!#&$~`_-Aa0", "Ûïé1&?+A"); + private static final String[] BOGUS_HASHES = {"", "test", "$t$test$", "$SHA$Test$$$$", "$$$$$", + "asdfg:hjkl", "::test", "~#$#~~~#$#~", "d41d8cd98f00b204e9800998ecf427e", + "$2y$7a$da641e404b982ed" }; + /** The encryption method to test. */ private EncryptionMethod method; /** Map with the hashes against which the entries in GIVEN_PASSWORDS are tested. */ @@ -123,6 +127,17 @@ public abstract class AbstractEncryptionMethodTest { } } + /** Tests various strings to ensure that encryption methods don't rely on the hash's format too much. */ + @Test + public void testMalformedHashes() { + String salt = method.hasSeparateSalt() ? "testSalt" : null; + for (String bogusHash : BOGUS_HASHES) { + HashedPassword hashedPwd = new HashedPassword(bogusHash, salt); + assertFalse("Passing bogus hash '" + bogusHash + "' does not result in an error", + method.comparePassword("Password", hashedPwd, "player")); + } + } + private boolean doesGivenHashMatch(String password, EncryptionMethod method) { return method.comparePassword(password, hashes.get(password), USERNAME); }