From 9c4a578bec1de662d692567ea6b490197c5b9f93 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Wed, 30 Dec 2015 15:43:25 +0100 Subject: [PATCH] #358 Remove old methods on PasswordSecurity, unify hash + salt - For encryption methods with a separate salt, the hash is useless without the salt, so hash and salt should always be persisted and retrieved together --- src/main/java/fr/xephi/authme/api/API.java | 9 +- src/main/java/fr/xephi/authme/api/NewAPI.java | 13 +- .../process/login/AsynchronousLogin.java | 6 +- .../unregister/AsynchronousUnregister.java | 5 +- .../authme/security/PasswordSecurity.java | 182 ++++++++---------- .../authme/security/crypts/WORDPRESS.java | 3 +- .../xephi/authme/task/ChangePasswordTask.java | 2 +- 7 files changed, 95 insertions(+), 125 deletions(-) diff --git a/src/main/java/fr/xephi/authme/api/API.java b/src/main/java/fr/xephi/authme/api/API.java index 09e5a423..af96bf96 100644 --- a/src/main/java/fr/xephi/authme/api/API.java +++ b/src/main/java/fr/xephi/authme/api/API.java @@ -121,13 +121,8 @@ public class API { * @return true if the password is correct, false otherwise */ @Deprecated - public static boolean checkPassword(String playerName, - String passwordToCheck) { - if (!isRegistered(playerName)) - return false; - String player = playerName.toLowerCase(); - PlayerAuth auth = instance.getDataSource().getAuth(player); - return passwordSecurity.comparePassword(passwordToCheck, auth.getHash(), playerName); + public static boolean checkPassword(String playerName, String passwordToCheck) { + return isRegistered(playerName) && passwordSecurity.comparePassword(passwordToCheck, playerName); } /** diff --git a/src/main/java/fr/xephi/authme/api/NewAPI.java b/src/main/java/fr/xephi/authme/api/NewAPI.java index 09d47b0c..ee48d5d6 100644 --- a/src/main/java/fr/xephi/authme/api/NewAPI.java +++ b/src/main/java/fr/xephi/authme/api/NewAPI.java @@ -116,9 +116,11 @@ public class NewAPI { } /** - * @param playerName + * Return whether the player is registered. * - * @return true if player is registered + * @param playerName The player name to check + * + * @return true if player is registered, false otherwise */ public boolean isRegistered(String playerName) { String player = playerName.toLowerCase(); @@ -134,12 +136,7 @@ public class NewAPI { * @return true if the password is correct, false otherwise */ public boolean checkPassword(String playerName, String passwordToCheck) { - if (!isRegistered(playerName)) { - return false; - } - String player = playerName.toLowerCase(); - PlayerAuth auth = plugin.getDataSource().getAuth(player); - return plugin.getPasswordSecurity().comparePassword(passwordToCheck, auth.getHash(), playerName); + return isRegistered(playerName) && plugin.getPasswordSecurity().comparePassword(passwordToCheck, playerName); } /** diff --git a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java index 17138cc6..0d99d04c 100644 --- a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java +++ b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java @@ -136,9 +136,9 @@ public class AsynchronousLogin { if (pAuth == null || needsCaptcha()) return; - String hash = pAuth.getHash(); String email = pAuth.getEmail(); - boolean passwordVerified = forceLogin || plugin.getPasswordSecurity().comparePassword(password, hash, realName); + boolean passwordVerified = forceLogin || plugin.getPasswordSecurity() + .comparePassword(password, pAuth.getHash(), pAuth.getSalt(), realName); if (passwordVerified && player.isOnline()) { PlayerAuth auth = PlayerAuth.builder() @@ -147,7 +147,7 @@ public class AsynchronousLogin { .ip(getIP()) .lastLogin(new Date().getTime()) .email(email) - .hash(hash) + .hash(pAuth.getHash()) .salt(pAuth.getSalt()) .build(); database.updateSession(auth); diff --git a/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java b/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java index ed9ad468..1545552e 100644 --- a/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java +++ b/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java @@ -2,6 +2,7 @@ package fr.xephi.authme.process.unregister; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; +import fr.xephi.authme.cache.auth.PlayerAuth; import fr.xephi.authme.cache.auth.PlayerCache; import fr.xephi.authme.cache.backup.JsonCache; import fr.xephi.authme.cache.limbo.LimboCache; @@ -52,7 +53,9 @@ public class AsynchronousUnregister { } public void process() { - if (force || plugin.getPasswordSecurity().comparePassword(password, PlayerCache.getInstance().getAuth(name).getHash(), player.getName())) { + PlayerAuth cachedAuth = PlayerCache.getInstance().getAuth(name); + if (force || plugin.getPasswordSecurity().comparePassword( + password, cachedAuth.getHash(), cachedAuth.getSalt(), player.getName())) { if (!plugin.getDataSource().removeAuth(name)) { m.send(player, MessageKey.ERROR); return; diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index 68a59015..678396b5 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java @@ -1,23 +1,19 @@ package fr.xephi.authme.security; -import fr.xephi.authme.AuthMe; import fr.xephi.authme.cache.auth.PlayerAuth; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.events.PasswordEncryptionEvent; import fr.xephi.authme.security.crypts.EncryptionMethod; import fr.xephi.authme.security.crypts.HashResult; -import fr.xephi.authme.settings.Settings; import org.bukkit.Bukkit; -import java.security.NoSuchAlgorithmException; import java.util.HashMap; /** + * Manager class for password-related operations. */ public class PasswordSecurity { - @Deprecated - public static final HashMap userSalt = new HashMap<>(); private final DataSource dataSource; private final HashAlgorithm algorithm; private final boolean supportOldAlgorithm; @@ -28,49 +24,6 @@ public class PasswordSecurity { this.supportOldAlgorithm = supportOldAlgorithm; } - @Deprecated - public static boolean comparePasswordWithHash(String password, String hash, - String playerName) throws NoSuchAlgorithmException { - HashAlgorithm algorithm = Settings.getPasswordHash; - EncryptionMethod method; - try { - if (algorithm != HashAlgorithm.CUSTOM) { - method = algorithm.getClazz().newInstance(); - } else { - method = null; - } - - PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName); - Bukkit.getPluginManager().callEvent(event); - method = event.getMethod(); - - if (method == null) - throw new NoSuchAlgorithmException("Unknown hash algorithm"); - - String salt = null; - if (method.hasSeparateSalt()) { - PlayerAuth auth = AuthMe.getInstance().getDataSource().getAuth(playerName); - if (auth == null) { - // 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 - return false; - } - salt = auth.getSalt(); - } - - if (method.comparePassword(hash, password, salt, playerName)) - return true; - - if (Settings.supportOldPassword) { - if (compareWithAllEncryptionMethod(password, hash, playerName)) - return true; - } - } catch (InstantiationException | IllegalAccessException e) { - throw new NoSuchAlgorithmException("Problem with this hash algorithm"); - } - return false; - } - public HashResult computeHash(String password, String playerName) { return computeHash(algorithm, password, playerName); } @@ -80,71 +33,92 @@ public class PasswordSecurity { return method.computeHash(password, playerName); } - public boolean comparePassword(String hash, String password, String playerName) { - return comparePassword(algorithm, hash, password, playerName); + public boolean comparePassword(String password, String playerName) { + PlayerAuth auth = dataSource.getAuth(playerName); + if (auth != null) { + return comparePassword(auth.getHash(), auth.getSalt(), password, playerName); + } + return false; } - public boolean comparePassword(HashAlgorithm algorithm, String hash, String password, String playerName) { + public boolean comparePassword(String hash, String salt, String password, String playerName) { EncryptionMethod method = initializeEncryptionMethod(algorithm, playerName); - String salt = null; - if (method.hasSeparateSalt()) { - PlayerAuth auth = dataSource.getAuth(playerName); - if (auth == null) { - // 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 - return false; - } - salt = auth.getSalt(); + // 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 + if (method.hasSeparateSalt() && salt == null) { + return false; } - return method.comparePassword(hash, password, salt, playerName); - // TODO #358: Add logic for Settings.supportOldPassword + return method.comparePassword(hash, password, salt, playerName) + || supportOldAlgorithm && compareWithAllEncryptionMethods(password, hash, salt, playerName); } - private EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) { - EncryptionMethod method; - try { - method = HashAlgorithm.CUSTOM.equals(algorithm) - ? null - : algorithm.getClazz().newInstance(); - } catch (InstantiationException | IllegalAccessException e) { - throw new IllegalStateException("Constructor for '" + algorithm.getClazz() - + "' could not be invoked. (Is there no default constructor?)", e); - } - - PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName); - Bukkit.getPluginManager().callEvent(event); - return event.getMethod(); - } - - @Deprecated - private static boolean compareWithAllEncryptionMethod(String password, - String hash, String playerName) { - String salt; - PlayerAuth auth = AuthMe.getInstance().getDataSource().getAuth(playerName); - if (auth == null) { - salt = null; - } else { - salt = auth.getSalt(); - } - - for (HashAlgorithm algo : HashAlgorithm.values()) { - if (algo != HashAlgorithm.CUSTOM) { - try { - EncryptionMethod method = algo.getClazz().newInstance(); - if (method.comparePassword(hash, password, salt, playerName)) { - PlayerAuth nAuth = AuthMe.getInstance().getDataSource().getAuth(playerName); - if (nAuth != null) { - // nAuth.setHash(getHash(Settings.getPasswordHash, password, playerName)); - nAuth.setSalt(userSalt.containsKey(playerName) ? userSalt.get(playerName) : ""); - AuthMe.getInstance().getDataSource().updatePassword(nAuth); - AuthMe.getInstance().getDataSource().updateSalt(nAuth); - } - return true; - } - } catch (Exception ignored) { + /** + * Compare the given hash with all available encryption methods to support the migration to a new encryption method. + * + * @param password The clear-text password to check + * @param hash The hash to text the password against + * @param salt The salt (or null if none available) + * @param playerName The name of the player + * @return True if the + */ + private boolean compareWithAllEncryptionMethods(String password, String hash, String salt, String playerName) { + for (HashAlgorithm algorithm : HashAlgorithm.values()) { + if (!HashAlgorithm.CUSTOM.equals(algorithm)) { + EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm); + if (method != null && method.comparePassword(hash, password, salt, playerName)) { + hashPasswordForNewAlgorithm(password, playerName); + return true; } } } return false; } + + /** + * Get the encryption method from the given {@link HashAlgorithm} value and emits a + * {@link PasswordEncryptionEvent}. The encryption method from the event is returned, + * which may have been changed by an external listener. + * + * @param algorithm The algorithm to retrieve the encryption method for + * @param playerName The name of the player a password will be hashed for + * @return The encryption method + */ + private static EncryptionMethod initializeEncryptionMethod(HashAlgorithm algorithm, String playerName) { + EncryptionMethod method = initializeEncryptionMethodWithoutEvent(algorithm); + PasswordEncryptionEvent event = new PasswordEncryptionEvent(method, playerName); + Bukkit.getPluginManager().callEvent(event); + return event.getMethod(); + } + + /** + * Initialize the encryption method corresponding to the given hash algorithm. + * + * @param algorithm The algorithm to retrieve the encryption method for + * @return The associated encryption method + */ + private static EncryptionMethod initializeEncryptionMethodWithoutEvent(HashAlgorithm algorithm) { + try { + return HashAlgorithm.CUSTOM.equals(algorithm) + ? null + : algorithm.getClazz().newInstance(); + } catch (InstantiationException | IllegalAccessException e) { + throw new UnsupportedOperationException("Constructor for '" + algorithm.getClazz().getSimpleName() + + "' could not be invoked. (Is there no default constructor?)", e); + } + } + + private void hashPasswordForNewAlgorithm(String password, String playerName) { + PlayerAuth auth = dataSource.getAuth(playerName); + if (auth != null) { + HashResult hashResult = initializeEncryptionMethod(algorithm, playerName) + .computeHash(password, playerName); + + // TODO #358: updatePassword() should just take the HashResult..., or at least hash & salt. Idem for setHash + auth.setSalt(hashResult.getSalt()); + auth.setHash(hashResult.getHash()); + dataSource.updatePassword(auth); + dataSource.updateSalt(auth); + } + } + } 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 7d06aac6..d1e6a9dc 100644 --- a/src/main/java/fr/xephi/authme/security/crypts/WORDPRESS.java +++ b/src/main/java/fr/xephi/authme/security/crypts/WORDPRESS.java @@ -11,7 +11,8 @@ import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.util.Arrays; -@Recommendation(Usage.ACCEPTABLE) +// TODO #391: Wordpress algorithm fails sometimes. Fix it and change the Recommendation to "ACCEPTABLE" if appropriate +@Recommendation(Usage.DO_NOT_USE) @HasSalt(value = SaltType.TEXT, length = 9) // Note ljacqu 20151228: Wordpress is actually a salted algorithm but salt generation is handled internally // and isn't exposed to the outside, so we treat it as an unsalted implementation diff --git a/src/main/java/fr/xephi/authme/task/ChangePasswordTask.java b/src/main/java/fr/xephi/authme/task/ChangePasswordTask.java index 6e332ebd..1b2f75ff 100644 --- a/src/main/java/fr/xephi/authme/task/ChangePasswordTask.java +++ b/src/main/java/fr/xephi/authme/task/ChangePasswordTask.java @@ -34,7 +34,7 @@ public class ChangePasswordTask implements Runnable { final String name = player.getName().toLowerCase(); PlayerAuth auth = PlayerCache.getInstance().getAuth(name); - if (passwordSecurity.comparePassword(oldPassword, auth.getHash(), player.getName())) { + if (passwordSecurity.comparePassword(oldPassword, auth.getHash(), auth.getSalt(), player.getName())) { HashResult hashResult = passwordSecurity.computeHash(newPassword, name); auth.setHash(hashResult.getHash()); auth.setSalt(hashResult.getSalt());