From 391e1b04a2bd091637a76e2fa08ea6a0ede56087 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 14 Jan 2016 21:55:09 +0100 Subject: [PATCH] Fix #440 Hash algo's sometimes skipped for old algorithm support - Fix check that discards potentially trying all encryption methods if password didn't match - Wrap call to encryption method properly to avoid calling methods with hasSeparateSalt() = true and a null salt --- .../authme/security/PasswordSecurity.java | 27 ++++++++++++------- .../fr/xephi/authme/security/crypts/WBB4.java | 9 ++++++- .../authme/security/PasswordSecurityTest.java | 4 ++- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index 552b64dd..1946df6a 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java @@ -41,15 +41,8 @@ public class PasswordSecurity { public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) { EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName); - // User is not in data source, so the result will invariably be wrong because an encryption - // method with hasSeparateSalt() == true NEEDS the salt to evaluate the password - String salt = hashedPassword.getSalt(); - if (method.hasSeparateSalt() && salt == null) { - return false; - } - String playerLowerCase = playerName.toLowerCase(); - return method.comparePassword(password, hashedPassword, playerLowerCase) + return methodMatches(method, password, hashedPassword, playerLowerCase) || supportOldAlgorithm && compareWithAllEncryptionMethods(password, hashedPassword, playerLowerCase); } @@ -69,7 +62,7 @@ public class PasswordSecurity { for (HashAlgorithm algorithm : HashAlgorithm.values()) { if (!HashAlgorithm.CUSTOM.equals(algorithm)) { EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm); - if (method != null && method.comparePassword(password, hashedPassword, playerName)) { + if (methodMatches(method, password, hashedPassword, playerName)) { hashPasswordForNewAlgorithm(password, playerName); return true; } @@ -78,6 +71,22 @@ public class PasswordSecurity { return false; } + /** + * Verify with the given encryption method whether the password matches the hash after checking that + * the method can be called safely with the given data. + * + * @param method The encryption method to use + * @param password The password to check + * @param hashedPassword The hash to check against + * @param playerName The name of the player + * @return True if the password matched, false otherwise + */ + private static boolean methodMatches(EncryptionMethod method, String password, + HashedPassword hashedPassword, String playerName) { + return method != null && (!method.hasSeparateSalt() || hashedPassword.getSalt() != null) + && method.comparePassword(password, hashedPassword, playerName); + } + /** * Get the encryption method from the given {@link HashAlgorithm} value and emit a * {@link PasswordEncryptionEvent}. The encryption method from the event is then returned, 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 9c7d13d3..cbd77fc6 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/WBB4.java +++ b/src/main/java/fr/xephi/authme/security/crypts/WBB4.java @@ -1,7 +1,9 @@ package fr.xephi.authme.security.crypts; +import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.security.crypts.description.Recommendation; import fr.xephi.authme.security.crypts.description.Usage; +import fr.xephi.authme.util.StringUtils; @Recommendation(Usage.DOES_NOT_WORK) public class WBB4 extends HexSaltedMethod { @@ -13,7 +15,12 @@ public class WBB4 extends HexSaltedMethod { @Override public boolean comparePassword(String password, HashedPassword hashedPassword, String playerName) { - return BCRYPT.checkpw(password, hashedPassword.getHash(), 2); + try { + return BCRYPT.checkpw(password, hashedPassword.getHash(), 2); + } catch (IllegalArgumentException e) { + ConsoleLogger.showError("WBB4 compare password returned: " + StringUtils.formatException(e)); + } + return false; } @Override diff --git a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java index d36810ab..cf15eb5f 100644 --- a/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java +++ b/src/test/java/fr/xephi/authme/security/PasswordSecurityTest.java @@ -7,6 +7,7 @@ import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.security.crypts.EncryptionMethod; import fr.xephi.authme.security.crypts.JOOMLA; import fr.xephi.authme.security.crypts.PHPBB; +import fr.xephi.authme.util.WrapperMock; import org.bukkit.event.Event; import org.bukkit.plugin.PluginManager; import org.junit.Before; @@ -42,6 +43,7 @@ public class PasswordSecurityTest { @Before public void setUpMocks() { + WrapperMock.createInstance(); pluginManager = mock(PluginManager.class); dataSource = mock(DataSource.class); method = mock(EncryptionMethod.class); @@ -209,7 +211,7 @@ public class PasswordSecurityTest { HashedPassword hashedPassword = new HashedPassword("~T!est#Hash"); given(method.computeHash(password, username)).willReturn(hashedPassword); given(method.hasSeparateSalt()).willReturn(true); - PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.XAUTH, pluginManager, true); + PasswordSecurity security = new PasswordSecurity(dataSource, HashAlgorithm.XAUTH, pluginManager, false); // when boolean result = security.comparePassword(password, hashedPassword, username);