From 15782f62ba134006a2f92f65d10f0713e8ad85a0 Mon Sep 17 00:00:00 2001 From: Martin Spielmann Date: Sun, 1 Jan 2017 20:22:06 +0100 Subject: Added possibility to use secure hashes to store passwords Addresses #1166 --- src/main/distrib/data/defaults.properties | 8 +- .../java/com/gitblit/client/EditUserDialog.java | 10 +- .../com/gitblit/manager/AuthenticationManager.java | 10 ++ .../com/gitblit/utils/SecurePasswordHashUtils.java | 174 +++++++++++++++++++++ .../gitblit/wicket/pages/ChangePasswordPage.java | 6 +- src/site/administration.mkd | 2 +- .../gitblit/utils/SecurePasswordHashUtilsTest.java | 63 ++++++++ 7 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java create mode 100644 src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 9bb02482..352a6750 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -854,12 +854,14 @@ realm.userService = ${baseFolder}/users.conf realm.authenticationProviders = # How to store passwords. -# Valid values are plain, md5, or combined-md5. md5 is the hash of password. +# Valid values are plain, md5, combined-md5 or PBKDF2WithHmacSHA256. +# md5 is the hash of password. # combined-md5 is the hash of username.toLowerCase()+password. -# Default is md5. +# PBKDF2WithHmacSHA256 is salt+hash(salt+password) +# Default is PBKDF2WithHmacSHA256. # # SINCE 0.5.0 -realm.passwordStorage = md5 +realm.passwordStorage = PBKDF2WithHmacSHA256 # Minimum valid length for a plain text password. # Default value is 5. Absolute minimum is 4. diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index 4b01ff04..fcbb41ef 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -58,8 +58,10 @@ import com.gitblit.models.RepositoryModel; import com.gitblit.models.ServerSettings; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; +import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.StringUtils; + public class EditUserDialog extends JDialog { private static final long serialVersionUID = 1L; @@ -318,7 +320,8 @@ public class EditUserDialog extends JDialog { return false; } if (!password.toUpperCase().startsWith(StringUtils.MD5_TYPE) - && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE) + && !password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)) { String cpw = new String(confirmPasswordField.getPassword()); if (cpw == null || cpw.length() != password.length()) { error("Please confirm the password!"); @@ -332,7 +335,7 @@ public class EditUserDialog extends JDialog { // change the cookie user.cookie = user.createCookie(); - String type = settings.get(Keys.realm.passwordStorage).getString("md5"); + String type = settings.get(Keys.realm.passwordStorage).getString("PBKDF2WithHmacSHA256"); if (type.equalsIgnoreCase("md5")) { // store MD5 digest of password user.password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); @@ -340,6 +343,9 @@ public class EditUserDialog extends JDialog { // store MD5 digest of username+password user.password = StringUtils.COMBINED_MD5_TYPE + StringUtils.getMD5(user.username + password); + } else if (type.equalsIgnoreCase("PBKDF2WithHmacSHA256")) { + // store PBKDF2WithHmacSHA256 digest of password + user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); } else { // plain-text password user.password = password; diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 0a4d8ed7..7a1fd9f2 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -52,6 +52,7 @@ import com.gitblit.models.UserModel; import com.gitblit.transport.ssh.SshKey; import com.gitblit.utils.Base64; import com.gitblit.utils.HttpUtils; +import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.StringUtils; import com.gitblit.utils.X509Utils.X509Metadata; import com.google.inject.Inject; @@ -518,6 +519,7 @@ public class AuthenticationManager implements IAuthenticationManager { */ protected UserModel authenticateLocal(UserModel user, char [] password) { UserModel returnedUser = null; + //weak password hash if (user.password.startsWith(StringUtils.MD5_TYPE)) { // password digest String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password)); @@ -534,7 +536,15 @@ public class AuthenticationManager implements IAuthenticationManager { } else if (user.password.equals(new String(password))) { // plain-text password returnedUser = user; + } else if (user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)){ + //strong hash + SecurePasswordHashUtils hashUtils = SecurePasswordHashUtils.get(); + boolean isPasswordValid = hashUtils.isPasswordCorrect(password, user.password); + if(isPasswordValid){ + returnedUser = user; + } } + return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); } diff --git a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java new file mode 100644 index 00000000..de2c0084 --- /dev/null +++ b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java @@ -0,0 +1,174 @@ +package com.gitblit.utils; + +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; + +import org.apache.commons.codec.DecoderException; +import org.apache.commons.codec.binary.Hex; + +import java.security.NoSuchAlgorithmException; +import java.security.spec.InvalidKeySpecException; +import java.util.Arrays; + +/** + * The Class SecurePasswordHashUtils provides methods to create and validate secure hashes from user passwords. + * + * It uses the concept proposed by OWASP - Hashing Java: https://www.owasp.org/index.php/Hashing_Java + */ +public class SecurePasswordHashUtils { + + public static final String PBKDF2WITHHMACSHA256_TYPE = "PBKDF2WITHHMACSHA256:"; + + private static final SecureRandom RANDOM = new SecureRandom(); + private static final int ITERATIONS = 10000; + private static final int KEY_LENGTH = 256; + private static final int SALT_LENGTH = 32; + + /** The instance. */ + private static SecurePasswordHashUtils instance; + + /** + * Instantiates a new secure password hash utils. + */ + private SecurePasswordHashUtils() { + } + + /** + * Gets an instance of {@link SecurePasswordHashUtils}. + * + * @return the secure password hash utils + */ + public static SecurePasswordHashUtils get() { + if (instance == null) { + instance = new SecurePasswordHashUtils(); + } + return instance; + } + + /** + * Gets the next salt. + * + * @return the next salt + */ + protected byte[] getNextSalt() { + byte[] salt = new byte[SALT_LENGTH]; + RANDOM.nextBytes(salt); + return salt; + } + + /** + * Hash. + * + * @param password + * the password + * @param salt + * the salt + * @return the byte[] + */ + protected byte[] hash(char[] password, byte[] salt) { + PBEKeySpec spec = new PBEKeySpec(password, salt, ITERATIONS, KEY_LENGTH); + Arrays.fill(password, Character.MIN_VALUE); + try { + SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256"); + return skf.generateSecret(spec).getEncoded(); + } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { + throw new IllegalStateException("Error while hashing password", e); + } finally { + spec.clearPassword(); + } + } + + /** + * Checks if is password correct. + * + * @param passwordToCheck + * the password to check + * @param salt + * the salt + * @param expectedHash + * the expected hash + * @return true, if is password correct + */ + protected boolean isPasswordCorrect(char[] passwordToCheck, byte[] salt, byte[] expectedHash) { + byte[] hashToCheck = hash(passwordToCheck, salt); + Arrays.fill(passwordToCheck, Character.MIN_VALUE); + if (hashToCheck.length != expectedHash.length) { + return false; + } + for (int i = 0; i < hashToCheck.length; i++) { + if (hashToCheck[i] != expectedHash[i]) { + return false; + } + } + return true; + } + + /** + * Creates the new secure hash from a password and formats it properly to be + * stored in a file. + * + * @param password + * the password to be hashed + * @return the sting to be stored in a file (users.conf) + */ + public String createStoredPasswordFromPassword(String password) { + byte[] salt = getNextSalt(); + return String.format("%s%s%s", SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE, StringUtils.toHex(salt), StringUtils.toHex(hash(password.toCharArray(), salt))); + } + + /** + * Gets the salt from stored password. + * + * @param storedPassword + * the stored password + * @return the salt from stored password + */ + protected byte[] getSaltFromStoredPassword(String storedPassword) { + byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); + return Arrays.copyOfRange(pw, 0, SALT_LENGTH); + } + + /** + * Gets the hash from stored password. + * + * @param storedPassword + * the stored password + * @return the hash from stored password + */ + protected byte[] getHashFromStoredPassword(String storedPassword) { + byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); + return Arrays.copyOfRange(pw, SALT_LENGTH, pw.length); + } + + /** + * Strips the prefix ({@link #PBKDF2WITHHMACSHA256_TYPE}) from a stored + * password and returns the decoded hash + * + * @param storedPassword + * the stored password + * @return the stored hash with stripped prefix + */ + protected byte[] getStoredHashWithStrippedPrefix(String storedPassword) { + String saltAndHash = storedPassword.substring(PBKDF2WITHHMACSHA256_TYPE.length()); + try { + return Hex.decodeHex(saltAndHash.toCharArray()); + } catch (DecoderException e) { + throw new IllegalStateException("Error while reading stored credentials", e); + } + } + + /** + * Checks if is password correct. + * + * @param password + * the password + * @param storedPassword + * the stored password + * @return true, if is password correct + */ + public boolean isPasswordCorrect(char[] password, String storedPassword) { + byte[] storedSalt = getSaltFromStoredPassword(storedPassword); + byte[] storedHash = getHashFromStoredPassword(storedPassword); + return isPasswordCorrect(password, storedSalt, storedHash); + } +} diff --git a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java index 259a4bf4..afc4058b 100644 --- a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java +++ b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java @@ -28,6 +28,7 @@ import org.apache.wicket.protocol.http.WebResponse; import com.gitblit.GitBlitException; import com.gitblit.Keys; import com.gitblit.models.UserModel; +import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.StringUtils; import com.gitblit.wicket.GitBlitWebSession; import com.gitblit.wicket.NonTrimmedPasswordTextField; @@ -86,7 +87,7 @@ public class ChangePasswordPage extends RootSubPage { UserModel user = GitBlitWebSession.get().getUser(); // convert to MD5 digest, if appropriate - String type = app().settings().getString(Keys.realm.passwordStorage, "md5"); + String type = app().settings().getString(Keys.realm.passwordStorage, "PBKDF2WithHmacSHA256"); if (type.equalsIgnoreCase("md5")) { // store MD5 digest of password password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); @@ -94,6 +95,9 @@ public class ChangePasswordPage extends RootSubPage { // store MD5 digest of username+password password = StringUtils.COMBINED_MD5_TYPE + StringUtils.getMD5(user.username.toLowerCase() + password); + } else if (type.equalsIgnoreCase("PBKDF2WithHmacSHA256")) { + // store PBKDF2WithHmacSHA256 digest of password + user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); } user.password = password; diff --git a/src/site/administration.mkd b/src/site/administration.mkd index 049a8273..5feedee9 100644 --- a/src/site/administration.mkd +++ b/src/site/administration.mkd @@ -169,7 +169,7 @@ Usernames must be unique and are case-insensitive. Whitespace is illegal. ### Passwords -User passwords are CASE-SENSITIVE and may be *plain*, *md5*, or *combined-md5* formatted (see `gitblit.properties` -> *realm.passwordStorage*). +User passwords are CASE-SENSITIVE and may be *plain*, *md5*, *combined-md5* or *PBKDF2WithHmacSHA256* formatted (see `gitblit.properties` -> *realm.passwordStorage*). ### User Roles There are four actual *roles* in Gitblit: diff --git a/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java b/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java new file mode 100644 index 00000000..f687bda6 --- /dev/null +++ b/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java @@ -0,0 +1,63 @@ +package com.gitblit.utils; + +import static org.junit.Assert.*; + +import org.junit.Before; +import org.junit.Test; + +public class SecurePasswordHashUtilsTest { + + private static final String STORED_PASSWORD = "PBKDF2WITHHMACSHA256:2d7d3ccaa277787f288e9f929247361bfc83607c6a8447bf496267512e360ba0a97b3114937213b23230072517d65a2e00695a1cbc47a732510840817f22c1bc"; + private static final byte[] STORED_SALT_BYTES = new byte[]{45, 125, 60, -54, -94, 119, 120, 127, 40, -114, -97, -110, -110, 71, 54, 27, -4, -125, 96, 124, 106, -124, 71, -65, 73, 98, 103, 81, 46, 54, 11, -96}; + private static final byte[] STORED_HASH_BYTES = new byte[]{-87, 123, 49, 20, -109, 114, 19, -78, 50, 48, 7, 37, 23, -42, 90, 46, 0, 105, 90, 28, -68, 71, -89, 50, 81, 8, 64, -127, 127, 34, -63, -68}; + + private SecurePasswordHashUtils utils; + + @Before + public void init(){ + utils = SecurePasswordHashUtils.get(); + } + + @Test + public void testGetNextSalt() { + assertEquals(32, utils.getNextSalt().length); + } + + @Test + public void testHash() { + byte[] hash = utils.hash("foo".toCharArray(), STORED_SALT_BYTES); + assertArrayEquals(STORED_HASH_BYTES, hash); + } + + @Test + public void testIsPasswordCorrectCharArrayByteArrayByteArray() { + assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); + assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); + } + + @Test + public void testCreateNewStorableHashFromPassword() { + String newPwHash = utils.createStoredPasswordFromPassword("foo"); + assertTrue(newPwHash.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); + } + + @Test + public void testGetSaltFromStoredPassword() { + byte[] saltFromStoredPassword = utils.getSaltFromStoredPassword(STORED_PASSWORD); + assertArrayEquals(STORED_SALT_BYTES, saltFromStoredPassword); + + } + + @Test + public void testGetHashFromStoredPassword() { + byte[] hashFromStoredPassword = utils.getHashFromStoredPassword(STORED_PASSWORD); + assertArrayEquals(STORED_HASH_BYTES, hashFromStoredPassword); + } + + @Test + public void testIsPasswordCorrectCharArrayString() { + assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_PASSWORD)); + assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_PASSWORD)); + } + +} -- cgit v1.2.3 From 4ab81b3465f086f9fbeadc93d6bce326208e85ac Mon Sep 17 00:00:00 2001 From: Martin Spielmann Date: Sat, 7 Jan 2017 13:47:42 +0100 Subject: Update AuthenticationManager to update weakly stored passwords on login --- .../com/gitblit/manager/AuthenticationManager.java | 41 ++++++++++++++++++---- .../com/gitblit/utils/SecurePasswordHashUtils.java | 24 ++++++++++--- .../gitblit/tests/AuthenticationManagerTest.java | 6 ++++ 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 7a1fd9f2..46be2ef2 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -519,7 +519,8 @@ public class AuthenticationManager implements IAuthenticationManager { */ protected UserModel authenticateLocal(UserModel user, char [] password) { UserModel returnedUser = null; - //weak password hash + boolean strongHashUsed = false; + if (user.password.startsWith(StringUtils.MD5_TYPE)) { // password digest String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password)); @@ -533,19 +534,47 @@ public class AuthenticationManager implements IAuthenticationManager { if (user.password.equalsIgnoreCase(md5)) { returnedUser = user; } - } else if (user.password.equals(new String(password))) { - // plain-text password - returnedUser = user; } else if (user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)){ - //strong hash + // strong hash SecurePasswordHashUtils hashUtils = SecurePasswordHashUtils.get(); boolean isPasswordValid = hashUtils.isPasswordCorrect(password, user.password); if(isPasswordValid){ returnedUser = user; + strongHashUsed = true; } + } else if (user.password.equals(new String(password))) { + // plain-text password + returnedUser = user; + } + + // validate user + returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); + + // if no strong hash was used to store the password, try to update it based on the settings + if(!strongHashUsed){ + updateStoredPassword(returnedUser, password); } - return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); + return returnedUser; + } + + /** + * Update stored password to a strong hash if configured. + * + * @param user the user to be updated + * @param password the password + */ + protected void updateStoredPassword(UserModel user, char[] password) { + // check if user has successfully authenticated i.e. is not null + if(user != null){ + // check if strong hash algorithm is configured + String algorithm = settings.getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); + if(algorithm.equals(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)){ + // rehash the provided correct password and + user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + userManager.updateUserModel(user); + } + } } /** diff --git a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java index de2c0084..289084ee 100644 --- a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java +++ b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java @@ -11,13 +11,16 @@ import java.security.spec.InvalidKeySpecException; import java.util.Arrays; /** - * The Class SecurePasswordHashUtils provides methods to create and validate secure hashes from user passwords. + * The Class SecurePasswordHashUtils provides methods to create and validate + * secure hashes from user passwords. * - * It uses the concept proposed by OWASP - Hashing Java: https://www.owasp.org/index.php/Hashing_Java + * It uses the concept proposed by OWASP - Hashing Java: + * https://www.owasp.org/index.php/Hashing_Java */ public class SecurePasswordHashUtils { - public static final String PBKDF2WITHHMACSHA256_TYPE = "PBKDF2WITHHMACSHA256:"; + public static final String PBKDF2WITHHMACSHA256 = "PBKDF2WithHmacSHA256"; + public static final String PBKDF2WITHHMACSHA256_TYPE = PBKDF2WITHHMACSHA256.toUpperCase() + ":"; private static final SecureRandom RANDOM = new SecureRandom(); private static final int ITERATIONS = 10000; @@ -112,8 +115,21 @@ public class SecurePasswordHashUtils { * @return the sting to be stored in a file (users.conf) */ public String createStoredPasswordFromPassword(String password) { + return createStoredPasswordFromPassword(password.toCharArray()); + } + + /** + * Creates the new secure hash from a password and formats it properly to be + * stored in a file. + * + * @param password + * the password to be hashed + * @return the sting to be stored in a file (users.conf) + */ + public String createStoredPasswordFromPassword(char[] password) { byte[] salt = getNextSalt(); - return String.format("%s%s%s", SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE, StringUtils.toHex(salt), StringUtils.toHex(hash(password.toCharArray(), salt))); + return String.format("%s%s%s", SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE, StringUtils.toHex(salt), + StringUtils.toHex(hash(password, salt))); } /** diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java index f8dc8885..31b7512c 100644 --- a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -55,6 +55,7 @@ import com.gitblit.manager.UserManager; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; import com.gitblit.tests.mock.MemorySettings; +import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.XssFilter; import com.gitblit.utils.XssFilter.AllowXssFilter; @@ -658,12 +659,17 @@ public class AuthenticationManagerTest extends GitblitUnitTest { users.updateUserModel(user); assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); + + // validate that plaintext password was automatically updated to hashed one + assertTrue(user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); + user.disabled = true; users.updateUserModel(user); assertNull(auth.authenticate(user.username, user.password.toCharArray(), null)); users.deleteUserModel(user); } + @Test public void testContenairAuthenticate() throws Exception { -- cgit v1.2.3 From a4fef5ae34225f9f3d1fee4c0757466d3a3fb4d7 Mon Sep 17 00:00:00 2001 From: Martin Spielmann Date: Sat, 7 Jan 2017 13:53:36 +0100 Subject: Replaced duplicated strings by using constant --- src/main/java/com/gitblit/client/EditUserDialog.java | 4 ++-- src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index fcbb41ef..141f233b 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -335,7 +335,7 @@ public class EditUserDialog extends JDialog { // change the cookie user.cookie = user.createCookie(); - String type = settings.get(Keys.realm.passwordStorage).getString("PBKDF2WithHmacSHA256"); + String type = settings.get(Keys.realm.passwordStorage).getString(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); if (type.equalsIgnoreCase("md5")) { // store MD5 digest of password user.password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); @@ -343,7 +343,7 @@ public class EditUserDialog extends JDialog { // store MD5 digest of username+password user.password = StringUtils.COMBINED_MD5_TYPE + StringUtils.getMD5(user.username + password); - } else if (type.equalsIgnoreCase("PBKDF2WithHmacSHA256")) { + } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { // store PBKDF2WithHmacSHA256 digest of password user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); } else { diff --git a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java index afc4058b..4c074abc 100644 --- a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java +++ b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java @@ -87,7 +87,7 @@ public class ChangePasswordPage extends RootSubPage { UserModel user = GitBlitWebSession.get().getUser(); // convert to MD5 digest, if appropriate - String type = app().settings().getString(Keys.realm.passwordStorage, "PBKDF2WithHmacSHA256"); + String type = app().settings().getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); if (type.equalsIgnoreCase("md5")) { // store MD5 digest of password password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); @@ -95,7 +95,7 @@ public class ChangePasswordPage extends RootSubPage { // store MD5 digest of username+password password = StringUtils.COMBINED_MD5_TYPE + StringUtils.getMD5(user.username.toLowerCase() + password); - } else if (type.equalsIgnoreCase("PBKDF2WithHmacSHA256")) { + } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { // store PBKDF2WithHmacSHA256 digest of password user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); } -- cgit v1.2.3 From c7f75029fc4da931c16dfe24764656f28c2ca399 Mon Sep 17 00:00:00 2001 From: Martin Spielmann Date: Sat, 7 Jan 2017 13:58:07 +0100 Subject: fix comment --- src/main/java/com/gitblit/manager/AuthenticationManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 46be2ef2..2b54e4b8 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -570,7 +570,7 @@ public class AuthenticationManager implements IAuthenticationManager { // check if strong hash algorithm is configured String algorithm = settings.getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); if(algorithm.equals(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)){ - // rehash the provided correct password and + // rehash the provided correct password and update the user model user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); userManager.updateUserModel(user); } -- cgit v1.2.3 From 4068b152e530305f588bd56e753045936e87ede0 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 5 Nov 2019 13:06:07 +0100 Subject: Add a PasswordHash class as a central place to deal with password hashes. Instead of having to deal with the implementation details of hashing and verifying passwords in multiple places, have a central unit be responsible for it. Otherwise we need to edit three different places when adding a new hashing scheme. With this class adding a new hashing scheme just requires creating a new subclass of `PasswordHash` and registering its type in the enum `PasswordHash.Type`. The rest of the code will use a common interface for all hashing schemes and doesn't need to be changed when a new one is added. --- src/main/java/com/gitblit/utils/PasswordHash.java | 220 +++++++++++ .../java/com/gitblit/utils/PasswordHashTest.java | 420 +++++++++++++++++++++ 2 files changed, 640 insertions(+) create mode 100644 src/main/java/com/gitblit/utils/PasswordHash.java create mode 100644 src/test/java/com/gitblit/utils/PasswordHashTest.java diff --git a/src/main/java/com/gitblit/utils/PasswordHash.java b/src/main/java/com/gitblit/utils/PasswordHash.java new file mode 100644 index 00000000..5ccfab49 --- /dev/null +++ b/src/main/java/com/gitblit/utils/PasswordHash.java @@ -0,0 +1,220 @@ +/* + * Copyright 2017 gitblit.com. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.gitblit.utils; + +/** + * This is the superclass for classes responsible for handling password hashing. + * + * It provides a factory-like interface to create an instance of a class that + * is responsible for the mechanics of a specific password hashing method. + * It also provides the common interface, leaving implementation specifics + * to subclasses of itself, which are the factory products. + * + * @author Florian Zschocke + * @since 1.9.0 + */ +public abstract class PasswordHash { + + + /** + * The types of implemented password hashing schemes. + */ + enum Type { + MD5, + CMD5 + } + + /** + * The hashing scheme type handled by an instance of a subclass + */ + final Type type; + + + /** + * Constructor for subclasses to initialize the final type field. + * @param type + * Type of hashing scheme implemented by this instance. + */ + PasswordHash(Type type) { + this.type = type; + } + + + /** + * Create an instance of a password hashing class for the given hash type. + * + * @param type + * Type of hash to be used. + * @return A class that can calculate the given hash type and verify a user password, + * or null if the given hash type is not a valid one. + */ + public static PasswordHash instanceOf(String type) { + try { + Type hashType = Type.valueOf(type.toUpperCase()); + switch (hashType) { + case MD5: + return new PasswordHashMD5(); + case CMD5: + return new PasswordHashCombinedMD5(); + default: + return null; + } + } + catch (Exception e) { + return null; + } + } + + /** + * Create an instance of a password hashing class of the correct type for a given + * hashed password from the user password table. The stored hashed password needs + * to be prefixed with the hash type identifier. + * + * @param hashedEntry + * Hashed password string from the user table. + * @return + * A class that can calculate the given hash type and verify a user password, + * or null if no instance can be created for the hashed user password. + */ + public static PasswordHash instanceFor(String hashedEntry) { + Type type = getEntryType(hashedEntry); + if (type != null) return instanceOf(type.name()); + return null; + } + + /** + * Test if a given string is a hashed password entry. This method simply checks if the + * given string is prefixed by a known hash type identifier. + * + * @param password + * A stored user password. + * @return True if the given string is detected to be hashed with a known hash type, + * false otherwise. + */ + public static boolean isHashedEntry(String password) { + return null != getEntryType(password); + } + + + + /** + * Convert the given password to a hashed password entry to be stored in the user table. + * The resulting string is prefixed by the hashing scheme type followed by a colon: + * TYPE:theactualhashinhex + * + * @param password + * Password to be hashed. + * @param username + * User name, only used for the Combined-MD5 (user+MD5) hashing type. + * @return + * Hashed password entry to be stored in the user table. + */ + abstract public String toHashedEntry(String password, String username); + + /** + * Test if a given password (and user name) match a hashed password. + * The instance of the password hash class has to be created with + * {code instanceFor}, so that it matches the type of the hashed password + * entry to test against. + * + * + * @param hashedEntry + * The hashed password entry from the user password table. + * @param password + * Clear text password to test against the hashed one. + * @param username + * User name, needed for the MD5+USER hash type. + * @return True, if the password (and username) match the hashed password, + * false, otherwise. + */ + public boolean matches(String hashedEntry, char[] password, String username) { + if (hashedEntry == null || type != PasswordHash.getEntryType(hashedEntry)) return false; + if (password == null) return false; + + String hashed = toHashedEntry(String.valueOf(password), username); + return hashed.equalsIgnoreCase(hashedEntry); + } + + + + + + static Type getEntryType(String hashedEntry) { + if (hashedEntry == null) return null; + int indexOfSeparator = hashedEntry.indexOf(':'); + if (indexOfSeparator <= 0) return null; + String typeId = hashedEntry.substring(0, indexOfSeparator); + + try { + return Type.valueOf(typeId.toUpperCase()); + } + catch (Exception e) { return null;} + } + + + static String getEntryValue(String hashedEntry) { + if (hashedEntry == null) return null; + int indexOfSeparator = hashedEntry.indexOf(':'); + return hashedEntry.substring(indexOfSeparator +1, hashedEntry.length()); + } + + + + + + /************************************** Implementations *************************************************/ + + private static class PasswordHashMD5 extends PasswordHash + { + PasswordHashMD5() { + super(Type.MD5); + } + + @Override + public String toHashedEntry(String password, String username) { + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); + return type.name() + ":" + + StringUtils.getMD5(password); + } + } + + + + + private static class PasswordHashCombinedMD5 extends PasswordHash + { + PasswordHashCombinedMD5() { + super(Type.CMD5); + } + + @Override + public String toHashedEntry(String password, String username) { + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password with Combined-MD5."); + if (username == null) throw new IllegalArgumentException("The username argument may not be null when hashing a password with Combined-MD5."); + if (StringUtils.isEmpty(username)) throw new IllegalArgumentException("The username argument may not be empty when hashing a password with Combined-MD5."); + return type.name() + ":" + + StringUtils.getMD5(username.toLowerCase() + password); + } + + @Override + public boolean matches(String hashedEntry, char[] password, String username) { + if (username == null || StringUtils.isEmpty(username)) return false; + return super.matches(hashedEntry, password, username); + } + + } +} diff --git a/src/test/java/com/gitblit/utils/PasswordHashTest.java b/src/test/java/com/gitblit/utils/PasswordHashTest.java new file mode 100644 index 00000000..12686692 --- /dev/null +++ b/src/test/java/com/gitblit/utils/PasswordHashTest.java @@ -0,0 +1,420 @@ +/* + * Copyright 2017 gitblit.com. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.gitblit.utils; + +import static org.junit.Assert.*; + +import org.junit.Test; + +/** + * @author Florian Zschocke + * + */ +public class PasswordHashTest { + + static final String MD5_PASSWORD_0 = "password"; + static final String MD5_HASHED_ENTRY_0 = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; + static final String MD5_PASSWORD_1 = "This is a test password"; + static final String MD5_HASHED_ENTRY_1 = "md5:8e1901831af502c0f842d4efb9083bcf"; + static final String CMD5_USERNAME_0 = "Jane Doe"; + static final String CMD5_PASSWORD_0 = "password"; + static final String CMD5_HASHED_ENTRY_0 = "CMD5:DB9639A6E5F21457F9DFD7735FAFA68B"; + static final String CMD5_USERNAME_1 = "Joe Black"; + static final String CMD5_PASSWORD_1 = "ThisIsAWeirdScheme.Weird"; + static final String CMD5_HASHED_ENTRY_1 = "cmd5:5c154768287e32fa605656b98894da89"; + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#instanceOf(java.lang.String)} for MD5. + */ + @Test + public void testInstanceOfMD5() { + + PasswordHash pwdh = PasswordHash.instanceOf("md5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.MD5, pwdh.type); + assertTrue("Failed to match " +MD5_HASHED_ENTRY_1, pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), null)); + + pwdh = PasswordHash.instanceOf("MD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.MD5, pwdh.type); + assertTrue("Failed to match " +MD5_HASHED_ENTRY_0, pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), null)); + + pwdh = PasswordHash.instanceOf("mD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.MD5, pwdh.type); + assertTrue("Failed to match " +MD5_HASHED_ENTRY_1, pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), null)); + + + pwdh = PasswordHash.instanceOf("CMD5"); + assertNotNull(pwdh); + assertNotEquals(PasswordHash.Type.MD5, pwdh.type); + assertFalse("Failed to match " +MD5_HASHED_ENTRY_1, pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), null)); + } + + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#instanceOf(java.lang.String)} for combined MD5. + */ + @Test + public void testInstanceOfCombinedMD5() { + + PasswordHash pwdh = PasswordHash.instanceOf("cmd5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + assertTrue("Failed to match " +CMD5_HASHED_ENTRY_1, pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + + pwdh = PasswordHash.instanceOf("cMD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + assertTrue("Failed to match " +CMD5_HASHED_ENTRY_1, pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + + pwdh = PasswordHash.instanceOf("CMD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + assertTrue("Failed to match " +CMD5_HASHED_ENTRY_0, pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + + + pwdh = PasswordHash.instanceOf("MD5"); + assertNotNull(pwdh); + assertNotEquals(PasswordHash.Type.CMD5, pwdh.type); + assertFalse("Failed to match " +CMD5_HASHED_ENTRY_1, pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + } + + + + + + + + /** + * Test that no instance is returned for plaintext or unknown or not + * yet implemented hashing schemes. + */ + @Test + public void testNoInstanceOf() { + PasswordHash pwdh = PasswordHash.instanceOf("plain"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("PLAIN"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("Plain"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("scrypt"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("bCrypt"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("BCRYPT"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf("nixe"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceOf(null); + assertNull(pwdh); + } + + + + /** + * Test that for all known hash types an instance is created for a hashed entry + * that can verify the known password. + * + * Test method for {@link com.gitblit.utils.PasswordHash#instanceFor(java.lang.String)}. + */ + @Test + public void testInstanceFor() { + PasswordHash pwdh = PasswordHash.instanceFor(MD5_HASHED_ENTRY_0); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.MD5, pwdh.type); + assertTrue("Failed to match " +MD5_HASHED_ENTRY_0, pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), null)); + + pwdh = PasswordHash.instanceFor(MD5_HASHED_ENTRY_1); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.MD5, pwdh.type); + assertTrue("Failed to match " +MD5_HASHED_ENTRY_1, pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), null)); + + + pwdh = PasswordHash.instanceFor(CMD5_HASHED_ENTRY_0); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + assertTrue("Failed to match " +CMD5_HASHED_ENTRY_0, pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + + pwdh = PasswordHash.instanceFor(CMD5_HASHED_ENTRY_1); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + assertTrue("Failed to match " +CMD5_HASHED_ENTRY_1, pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + + + } + + /** + * Test that for no instance is returned for plaintext or unknown or + * not yet implemented hashing schemes. + * + * Test method for {@link com.gitblit.utils.PasswordHash#instanceFor(java.lang.String)}. + */ + @Test + public void testInstanceForNaught() { + PasswordHash pwdh = PasswordHash.instanceFor("password"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor("top secret"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor("pass:word"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor("PLAIN:password"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor("SCRYPT:1232rwv12w"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor("BCRYPT:urbvahiaufbvhabaiuevuzggubsbliue"); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor(""); + assertNull(pwdh); + + pwdh = PasswordHash.instanceFor(null); + assertNull(pwdh); + } + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#isHashedEntry(java.lang.String)}. + */ + @Test + public void testIsHashedEntry() { + assertTrue(MD5_HASHED_ENTRY_0, PasswordHash.isHashedEntry(MD5_HASHED_ENTRY_0)); + assertTrue(MD5_HASHED_ENTRY_1, PasswordHash.isHashedEntry(MD5_HASHED_ENTRY_1)); + assertTrue(CMD5_HASHED_ENTRY_0, PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_0)); + assertTrue(CMD5_HASHED_ENTRY_1, PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_1)); + + assertFalse(MD5_PASSWORD_1, PasswordHash.isHashedEntry(MD5_PASSWORD_1)); + assertFalse("topsecret", PasswordHash.isHashedEntry("topsecret")); + assertFalse("top:secret", PasswordHash.isHashedEntry("top:secret")); + assertFalse("secret Password", PasswordHash.isHashedEntry("secret Password")); + assertFalse("Empty string", PasswordHash.isHashedEntry("")); + assertFalse("Null", PasswordHash.isHashedEntry(null)); + } + + /** + * Test that hashed entry detection is not case sensitive on the hash type identifier. + * + * Test method for {@link com.gitblit.utils.PasswordHash#isHashedEntry(java.lang.String)}. + */ + @Test + public void testIsHashedEntryCaseInsenitive() { + assertTrue(MD5_HASHED_ENTRY_1.toLowerCase(), PasswordHash.isHashedEntry(MD5_HASHED_ENTRY_1.toLowerCase())); + assertTrue(CMD5_HASHED_ENTRY_1.toLowerCase(), PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_1.toLowerCase())); + } + + /** + * Test that unknown or not yet implemented hashing schemes are not detected as hashed entries. + * + * Test method for {@link com.gitblit.utils.PasswordHash#isHashedEntry(java.lang.String)}. + */ + @Test + public void testIsHashedEntryUnknown() { + assertFalse("BCRYPT:thisismypassword", PasswordHash.isHashedEntry("BCRYPT:thisismypassword")); + assertFalse("TSTHSH:asdchabufzuzfbhbakrzburzbcuzkuzcbajhbcasjdhbckajsbc", PasswordHash.isHashedEntry("TSTHSH:asdchabufzuzfbhbakrzburzbcuzkuzcbajhbcasjdhbckajsbc")); + } + + + + + /** + * Test creating a hashed entry for scheme MD5. In this scheme there is no salt, so a direct + * comparison to a constant value is possible. + * + * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for MD5. + */ + @Test + public void testToHashedEntryMD5() { + PasswordHash pwdh = PasswordHash.instanceOf("MD5"); + String hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_1, null); + assertTrue(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_1, "charlie"); + assertTrue(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry("badpassword", "charlie"); + assertFalse(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry("badpassword", null); + assertFalse(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + } + + @Test(expected = IllegalArgumentException.class) + public void testToHashedEntryMD5NullPassword() { + PasswordHash pwdh = PasswordHash.instanceOf("MD5"); + pwdh.toHashedEntry(null, null); + } + + + /** + * Test creating a hashed entry for scheme Combined-MD5. In this scheme there is no salt, so a direct + * comparison to a constant value is possible. + * + * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for CMD5. + */ + @Test + public void testToHashedEntryCMD5() { + PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); + String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, CMD5_USERNAME_1); + assertTrue(CMD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, "charlie"); + assertFalse(CMD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry("badpassword", "charlie"); + assertFalse(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + } + + @Test(expected = IllegalArgumentException.class) + public void testToHashedEntryCMD5NullPassword() { + PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); + pwdh.toHashedEntry(null, CMD5_USERNAME_1); + } + + /** + * Test creating a hashed entry for scheme Combined-MD5, when no user is given. + * This should never happen in the application, so we expect an exception to be thrown. + * + * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for broken CMD5. + */ + @Test + public void testToHashedEntryCMD5NoUsername() { + PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); + try { + String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, ""); + fail("CMD5 cannot work with an empty '' username. Got: " + hashedEntry); + } + catch (IllegalArgumentException ignored) { /*success*/ } + + try { + String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, " "); + fail("CMD5 cannot work with an empty ' ' username. Got: " + hashedEntry); + } + catch (IllegalArgumentException ignored) { /*success*/ } + + try { + String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, " "); + fail("CMD5 cannot work with an empty ' ' username. Got: " + hashedEntry); + } + catch (IllegalArgumentException ignored) { /*success*/ } + + try { + String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, null); + fail("CMD5 cannot work with a null username. Got: " + hashedEntry); + } + catch (IllegalArgumentException ignored) { /*success*/ } + } + + + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#matches(String, char[], String)} for MD5. + */ + @Test + public void testMatchesMD5() { + PasswordHash pwdh = PasswordHash.instanceOf("MD5"); + + assertTrue("PWD0, Null user", pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), null)); + assertTrue("PWD0, Empty user", pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), "")); + assertTrue("PWD0, With user", pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), "maxine")); + + assertTrue("PWD1, Null user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), null)); + assertTrue("PWD1, Empty user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), "")); + assertTrue("PWD1, With user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), "maxine")); + + + + assertFalse("Matched wrong password", pwdh.matches(MD5_HASHED_ENTRY_1, "wrongpassword".toCharArray(), null)); + assertFalse("Matched wrong password, with empty user", pwdh.matches(MD5_HASHED_ENTRY_1, "wrongpassword".toCharArray(), " ")); + assertFalse("Matched wrong password, with user", pwdh.matches(MD5_HASHED_ENTRY_1, "wrongpassword".toCharArray(), "someuser")); + + assertFalse("Matched empty password", pwdh.matches(MD5_HASHED_ENTRY_1, "".toCharArray(), null)); + assertFalse("Matched empty password, with empty user", pwdh.matches(MD5_HASHED_ENTRY_1, " ".toCharArray(), " ")); + assertFalse("Matched empty password, with user", pwdh.matches(MD5_HASHED_ENTRY_1, " ".toCharArray(), "someuser")); + + assertFalse("Matched null password", pwdh.matches(MD5_HASHED_ENTRY_1, null, null)); + assertFalse("Matched null password, with empty user", pwdh.matches(MD5_HASHED_ENTRY_1, null, " ")); + assertFalse("Matched null password, with user", pwdh.matches(MD5_HASHED_ENTRY_1, null, "someuser")); + + + assertFalse("Matched wrong hashed entry", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong hashed entry, with empty user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched wrong hashed entry, with user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_0.toCharArray(), "someuser")); + + assertFalse("Matched empty hashed entry", pwdh.matches("", MD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched empty hashed entry, with empty user", pwdh.matches(" ", MD5_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched empty hashed entry, with user", pwdh.matches(" ", MD5_PASSWORD_0.toCharArray(), "someuser")); + + assertFalse("Matched null entry", pwdh.matches(null, MD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched null entry, with empty user", pwdh.matches(null, MD5_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched null entry, with user", pwdh.matches(null, MD5_PASSWORD_0.toCharArray(), "someuser")); + + + assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + } + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#matches(String, char[], String)} for Combined-MD5. + */ + @Test + public void testMatchesCombinedMD5() { + PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); + + assertTrue("PWD0", pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + assertTrue("Empty user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + + + + assertFalse("Matched wrong password", pwdh.matches(CMD5_HASHED_ENTRY_1, "wrongpassword".toCharArray(), CMD5_USERNAME_1)); + assertFalse("Matched wrong password", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_1)); + + assertFalse("Matched wrong user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_0)); + assertFalse("Matched wrong user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), "Samantha Jones")); + + assertFalse("Matched empty user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), "")); + assertFalse("Matched empty user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), " ")); + assertFalse("Matched null user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), null)); + + assertFalse("Matched empty hashed entry", pwdh.matches("", CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + assertFalse("Matched empty hashed entry, with empty user", pwdh.matches(" ", CMD5_PASSWORD_1.toCharArray(), "")); + assertFalse("Matched empty hashed entry, with null user", pwdh.matches(" ", CMD5_PASSWORD_0.toCharArray(), null)); + + assertFalse("Matched null entry, with user", pwdh.matches(null, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + assertFalse("Matched null entry, with empty user", pwdh.matches(null, CMD5_PASSWORD_1.toCharArray(), "")); + assertFalse("Matched null entry, with null user", pwdh.matches(null, CMD5_PASSWORD_1.toCharArray(), null)); + + + assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + } +} -- cgit v1.2.3 From d1ee233d27fae23b1d0a69bbb6b9a363c3a76abe Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 5 Nov 2019 13:43:06 +0100 Subject: Add support for PBKDF2 to PasswordHash Integrate the work of pingunaut to add support for PBKDF2 password hashing. A new class `PasswordHashPbkdf2` is added, which builds on his `SecurePasswordHashUtils` class, but makes it a subclass of `PasswordHash`. This will replace the original class when integrating the new PasswordHash way into GitBlit. --- src/main/java/com/gitblit/utils/PasswordHash.java | 60 ++++- .../java/com/gitblit/utils/PasswordHashPbkdf2.java | 276 +++++++++++++++++++++ .../java/com/gitblit/utils/PasswordHashTest.java | 186 +++++++++++++- 3 files changed, 507 insertions(+), 15 deletions(-) create mode 100644 src/main/java/com/gitblit/utils/PasswordHashPbkdf2.java diff --git a/src/main/java/com/gitblit/utils/PasswordHash.java b/src/main/java/com/gitblit/utils/PasswordHash.java index 5ccfab49..13747805 100644 --- a/src/main/java/com/gitblit/utils/PasswordHash.java +++ b/src/main/java/com/gitblit/utils/PasswordHash.java @@ -16,6 +16,8 @@ package com.gitblit.utils; +import java.util.Arrays; + /** * This is the superclass for classes responsible for handling password hashing. * @@ -35,7 +37,8 @@ public abstract class PasswordHash { */ enum Type { MD5, - CMD5 + CMD5, + PBKDF2 } /** @@ -70,6 +73,8 @@ public abstract class PasswordHash { return new PasswordHashMD5(); case CMD5: return new PasswordHashCombinedMD5(); + case PBKDF2: + return new PasswordHashPbkdf2(); default: return null; } @@ -100,17 +105,31 @@ public abstract class PasswordHash { * Test if a given string is a hashed password entry. This method simply checks if the * given string is prefixed by a known hash type identifier. * - * @param password + * @param storedPassword * A stored user password. * @return True if the given string is detected to be hashed with a known hash type, * false otherwise. */ - public static boolean isHashedEntry(String password) { - return null != getEntryType(password); + public static boolean isHashedEntry(String storedPassword) { + return null != getEntryType(storedPassword); } - - + + + /** + * Convert the given password to a hashed password entry to be stored in the user table. + * The resulting string is prefixed by the hashing scheme type followed by a colon: + * TYPE:theactualhashinhex + * + * @param password + * Password to be hashed. + * @param username + * User name, only used for the Combined-MD5 (user+MD5) hashing type. + * @return + * Hashed password entry to be stored in the user table. + */ + abstract public String toHashedEntry(char[] password, String username); + /** * Convert the given password to a hashed password entry to be stored in the user table. * The resulting string is prefixed by the hashing scheme type followed by a colon: @@ -123,7 +142,10 @@ public abstract class PasswordHash { * @return * Hashed password entry to be stored in the user table. */ - abstract public String toHashedEntry(String password, String username); + public String toHashedEntry(String password, String username) { + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); + return toHashedEntry(password.toCharArray(), username); + } /** * Test if a given password (and user name) match a hashed password. @@ -145,7 +167,8 @@ public abstract class PasswordHash { if (hashedEntry == null || type != PasswordHash.getEntryType(hashedEntry)) return false; if (password == null) return false; - String hashed = toHashedEntry(String.valueOf(password), username); + String hashed = toHashedEntry(password, username); + Arrays.fill(password, Character.MIN_VALUE); return hashed.equalsIgnoreCase(hashedEntry); } @@ -159,6 +182,8 @@ public abstract class PasswordHash { if (indexOfSeparator <= 0) return null; String typeId = hashedEntry.substring(0, indexOfSeparator); + // Compatibility with type id "PBKDF2WITHHMACSHA256", which is also handled by PBKDF2 type. + if (typeId.equalsIgnoreCase("PBKDF2WITHHMACSHA256")) return Type.PBKDF2; try { return Type.valueOf(typeId.toUpperCase()); } @@ -169,7 +194,7 @@ public abstract class PasswordHash { static String getEntryValue(String hashedEntry) { if (hashedEntry == null) return null; int indexOfSeparator = hashedEntry.indexOf(':'); - return hashedEntry.substring(indexOfSeparator +1, hashedEntry.length()); + return hashedEntry.substring(indexOfSeparator +1); } @@ -184,6 +209,14 @@ public abstract class PasswordHash { super(Type.MD5); } + // To keep the handling identical to how it was before, and therefore not risk invalidating stored passwords, + // for MD5 the (String,String) variant of the method is the one implementing the hashing. + @Override + public String toHashedEntry(char[] password, String username) { + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); + return toHashedEntry(new String(password), username); + } + @Override public String toHashedEntry(String password, String username) { if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); @@ -201,9 +234,16 @@ public abstract class PasswordHash { super(Type.CMD5); } + // To keep the handling identical to how it was before, and therefore not risk invalidating stored passwords, + // for Combined-MD5 the (String,String) variant of the method is the one implementing the hashing. + @Override + public String toHashedEntry(char[] password, String username) { + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); + return toHashedEntry(new String(password), username); + } @Override public String toHashedEntry(String password, String username) { - if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password with Combined-MD5."); + if (password == null) throw new IllegalArgumentException("The password argument may not be null when hashing a password."); if (username == null) throw new IllegalArgumentException("The username argument may not be null when hashing a password with Combined-MD5."); if (StringUtils.isEmpty(username)) throw new IllegalArgumentException("The username argument may not be empty when hashing a password with Combined-MD5."); return type.name() + ":" diff --git a/src/main/java/com/gitblit/utils/PasswordHashPbkdf2.java b/src/main/java/com/gitblit/utils/PasswordHashPbkdf2.java new file mode 100644 index 00000000..1bce1229 --- /dev/null +++ b/src/main/java/com/gitblit/utils/PasswordHashPbkdf2.java @@ -0,0 +1,276 @@ +package com.gitblit.utils; + +import org.apache.commons.codec.DecoderException; +import org.apache.commons.codec.binary.Hex; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; +import java.security.NoSuchAlgorithmException; +import java.security.spec.InvalidKeySpecException; +import java.util.Arrays; + +/** + * The class PasswordHashPbkdf2 implements password hashing and validation + * with PBKDF2 + * + * It uses the concept proposed by OWASP - Hashing Java: + * https://www.owasp.org/index.php/Hashing_Java + */ +class PasswordHashPbkdf2 extends PasswordHash +{ + + private static final Logger LOGGER = LoggerFactory.getLogger(PasswordHashPbkdf2.class); + + /** + * The PBKDF has some parameters that define security and workload. + * The Configuration class keeps these parameters. + */ + private static class Configuration + { + private final String algorithm; + private final int iterations; + private final int keyLen; + private final int saltLen; + + private Configuration(String algorithm, int iterations, int keyLen, int saltLen) { + this.algorithm = algorithm; + this.iterations = iterations; + this.keyLen = keyLen; + this.saltLen = saltLen; + } + } + + + private static final SecureRandom RANDOM = new SecureRandom(); + /** + * A list of Configurations is created to list the configurations supported by + * this implementation. The configuration id is stored in the hashed entry, + * identifying the Configuration in this array. + * When adding a new variant with different values for these parameters, add + * it to this array. + * The code uses the last configuration in the array as the most secure, to be used + * when creating new hashes when no configuration is specified. + */ + private static final Configuration[] configurations = { + // Configuration 0, also default when none is specified in the stored hashed entry. + new Configuration("PBKDF2WithHmacSHA256", 10000, 256, 32) + }; + + + PasswordHashPbkdf2() { + super(Type.PBKDF2); + } + + + /* + * We return a hashed entry, where the hash part (salt+hash) itself is prefixed + * again by the configuration id of the configuration that was used for the PBKDF, + * enclosed in '$': + * PBKDF2:$0$thesaltThehash + */ + @Override + public String toHashedEntry(char[] password, String username) { + if (password == null) { + LOGGER.warn("The password argument may not be null when hashing a password."); + throw new IllegalArgumentException("The password argument may not be null when hashing a password."); + } + + int configId = getLatestConfigurationId(); + Configuration config = configurations[configId]; + + byte[] salt = new byte[config.saltLen]; + RANDOM.nextBytes(salt); + byte[] hash = hash(password, salt, config); + + return type.name() + ":" + + "$" + configId + "$" + + StringUtils.toHex(salt) + + StringUtils.toHex(hash); + } + + @Override + public boolean matches(String hashedEntry, char[] password, String username) { + if (hashedEntry == null || type != PasswordHash.getEntryType(hashedEntry)) return false; + if (password == null) return false; + + String hashedPart = getEntryValue(hashedEntry); + int configId = getConfigIdFromStoredPassword(hashedPart); + + return isPasswordCorrect(password, hashedPart, configurations[configId]); + } + + + + + + + + + /** + * Return the id of the most updated configuration of parameters for the PBKDF. + * New password hashes should be generated with this one. + * + * @return An index into the configurations array for the latest configuration. + */ + private int getLatestConfigurationId() { + return configurations.length-1; + } + + + /** + * Get the configuration id from the stored hashed password, that was used when the + * hash was created. The configuration id is the index into the configuration array, + * and is stored in the format $Id$ after the type identifier: TYPE:$Id$.... + * If there is no identifier in the stored entry, id 0 is used, to keep backward + * compatibility. + * If an id is found that is not in the range of the declared configurations, + * 0 is returned. This may fail password validation. As of now there is only one + * configuration and even if there were more, chances are slim that anything else + * was used. So we try at least the first one instead of failing with an exception + * as the probability of success is high enough to save the user from a bad experience + * and to risk some hassle for the admin finding out in the logs why a login failed, + * when it does. + * + * @param hashPart + * The hash part of the stored entry, i.e. the part after the TYPE: + * @return The configuration id, or + * 0 if none was found. + */ + private static int getConfigIdFromStoredPassword(String hashPart) { + String[] parts = hashPart.split("\\$", 3); + // If there are not two parts, there is no '$'-enclosed part and we have no configuration information stored. + // Return default 0. + if (parts.length <= 2) return 0; + + // The first string wil be empty. Even if it isn't we ignore it because it doesn't contain our information. + try { + int configId = Integer.parseInt(parts[1]); + if (configId < 0 || configId >= configurations.length) { + LOGGER.warn("A user table password entry contains a configuration id that is not valid: {}." + + "Assuming PBKDF configuration 0. This may fail to validate the password.", configId); + return 0; + } + return configId; + } + catch (NumberFormatException e) { + LOGGER.warn("A user table password entry contains a configuration id that is not a parsable number ({}${}$...)." + + "Assuming PBKDF configuration 0. This may fail to validate the password.", parts[0], parts[1], e); + return 0; + } + } + + + + + + /** + * Hash. + * + * @param password + * the password + * @param salt + * the salt + * @param config + * Parameter configuration to use for the PBKDF + * @return Hashed result + */ + private static byte[] hash(char[] password, byte[] salt, Configuration config) { + PBEKeySpec spec = new PBEKeySpec(password, salt, config.iterations, config.keyLen); + Arrays.fill(password, Character.MIN_VALUE); + try { + SecretKeyFactory skf = SecretKeyFactory.getInstance(config.algorithm); + return skf.generateSecret(spec).getEncoded(); + } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { + LOGGER.warn("Error while hashing password.", e); + throw new IllegalStateException("Error while hashing password", e); + } finally { + spec.clearPassword(); + } + } + + /** + * Checks if is password correct. + * + * @param passwordToCheck + * the password to check + * @param salt + * the salt + * @param expectedHash + * the expected hash + * @return true, if is password correct + */ + private static boolean isPasswordCorrect(char[] passwordToCheck, byte[] salt, byte[] expectedHash, Configuration config) { + byte[] hashToCheck = hash(passwordToCheck, salt, config); + Arrays.fill(passwordToCheck, Character.MIN_VALUE); + if (hashToCheck.length != expectedHash.length) { + return false; + } + for (int i = 0; i < hashToCheck.length; i++) { + if (hashToCheck[i] != expectedHash[i]) { + return false; + } + } + return true; + } + + + /** + * Gets the salt from stored password. + * + * @param storedPassword + * the stored password + * @return the salt from stored password + */ + private static byte[] getSaltFromStoredPassword(String storedPassword, Configuration config) { + byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); + return Arrays.copyOfRange(pw, 0, config.saltLen); + } + + /** + * Gets the hash from stored password. + * + * @param storedPassword + * the stored password + * @return the hash from stored password + */ + private static byte[] getHashFromStoredPassword(String storedPassword, Configuration config) { + byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); + return Arrays.copyOfRange(pw, config.saltLen, pw.length); + } + + /** + * Strips the configuration id prefix ($Id$) from a stored + * password and returns the decoded hash + * + * @param storedPassword + * the stored password + * @return the stored hash with stripped prefix + */ + private static byte[] getStoredHashWithStrippedPrefix(String storedPassword) { + String[] strings = storedPassword.split("\\$", 3); + String saltAndHash = strings[strings.length -1]; + try { + return Hex.decodeHex(saltAndHash.toCharArray()); + } catch (DecoderException e) { + LOGGER.warn("Failed to decode stored password entry from hex to string.", e); + throw new IllegalStateException("Error while reading stored credentials", e); + } + } + + /** + * Checks if password is correct. + * + * @param password + * the password to validate + * @param storedPassword + * the stored password, i.e. the password entry value, without the leading TYPE: + * @return true, if password is correct, false otherwise + */ + private static boolean isPasswordCorrect(char[] password, String storedPassword, Configuration config) { + byte[] storedSalt = getSaltFromStoredPassword(storedPassword, config); + byte[] storedHash = getHashFromStoredPassword(storedPassword, config); + return isPasswordCorrect(password, storedSalt, storedHash, config); + } +} diff --git a/src/test/java/com/gitblit/utils/PasswordHashTest.java b/src/test/java/com/gitblit/utils/PasswordHashTest.java index 12686692..c5a485dc 100644 --- a/src/test/java/com/gitblit/utils/PasswordHashTest.java +++ b/src/test/java/com/gitblit/utils/PasswordHashTest.java @@ -36,6 +36,15 @@ public class PasswordHashTest { static final String CMD5_USERNAME_1 = "Joe Black"; static final String CMD5_PASSWORD_1 = "ThisIsAWeirdScheme.Weird"; static final String CMD5_HASHED_ENTRY_1 = "cmd5:5c154768287e32fa605656b98894da89"; + static final String PBKDF2_PASSWORD_0 = "password"; + static final String PBKDF2_HASHED_ENTRY_0 = "PBKDF2:70617373776f726450415353574f524470617373776f726450415353574f52440f17d16621b32ae1bb2b1041fcb19e294b35d514d361c08eed385766e38f6f3a"; + static final String PBKDF2_PASSWORD_1 = "A REALLY better scheme than MD5"; + static final String PBKDF2_HASHED_ENTRY_1 = "PBKDF2:$0$46726573682066726f6d207468652053414c54206d696e65206f6620446f6f6de8e50b035679b25ce8b6ab41440938b7b1f97fc0c797fcf59302c2916f6c8fef"; + static final String PBKDF2_PASSWORD_2 = "passwordPASSWORDpassword"; + static final String PBKDF2_HASHED_ENTRY_2 = "pbkdf2:$0$73616c7453414c5473616c7453414c5473616c7453414c5473616c7453414c54560d0f02b565e37695da15141044506d54cb633a5a70b41c574069ea50a1247a"; + static final String PBKDF2_PASSWORD_3 = "foo"; + static final String PBKDF2_HASHED_ENTRY_3 = "PBKDF2WITHHMACSHA256:2d7d3ccaa277787f288e9f929247361bfc83607c6a8447bf496267512e360ba0a97b3114937213b23230072517d65a2e00695a1cbc47a732510840817f22c1bc"; + /** @@ -98,6 +107,32 @@ public class PasswordHashTest { + /** + * Test method for {@link com.gitblit.utils.PasswordHash#instanceOf(java.lang.String)} for PBKDF2. + */ + @Test + public void testInstanceOfPBKDF2() { + PasswordHash pwdh = PasswordHash.instanceOf("PBKDF2"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_0, pwdh.matches(PBKDF2_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), null)); + + pwdh = PasswordHash.instanceOf("pbkdf2"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_1, pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), null)); + + pwdh = PasswordHash.instanceOf("pbKDF2"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_1, pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), null)); + + + pwdh = PasswordHash.instanceOf("md5"); + assertNotNull(pwdh); + assertNotEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertFalse("Failed to match " +PBKDF2_HASHED_ENTRY_1, pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), null)); + } @@ -165,6 +200,20 @@ public class PasswordHashTest { assertTrue("Failed to match " +CMD5_HASHED_ENTRY_1, pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + pwdh = PasswordHash.instanceFor(PBKDF2_HASHED_ENTRY_0); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_0, pwdh.matches(PBKDF2_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), null)); + + pwdh = PasswordHash.instanceFor(PBKDF2_HASHED_ENTRY_1); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_1, pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), null)); + + pwdh = PasswordHash.instanceFor(PBKDF2_HASHED_ENTRY_3); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.PBKDF2, pwdh.type); + assertTrue("Failed to match " +PBKDF2_HASHED_ENTRY_3, pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_3.toCharArray(), null)); } /** @@ -210,6 +259,10 @@ public class PasswordHashTest { assertTrue(MD5_HASHED_ENTRY_1, PasswordHash.isHashedEntry(MD5_HASHED_ENTRY_1)); assertTrue(CMD5_HASHED_ENTRY_0, PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_0)); assertTrue(CMD5_HASHED_ENTRY_1, PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_1)); + assertTrue(PBKDF2_HASHED_ENTRY_0, PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_0)); + assertTrue(PBKDF2_HASHED_ENTRY_1, PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_1)); + assertTrue(PBKDF2_HASHED_ENTRY_2, PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_2)); + assertTrue(PBKDF2_HASHED_ENTRY_3, PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_3)); assertFalse(MD5_PASSWORD_1, PasswordHash.isHashedEntry(MD5_PASSWORD_1)); assertFalse("topsecret", PasswordHash.isHashedEntry("topsecret")); @@ -228,6 +281,8 @@ public class PasswordHashTest { public void testIsHashedEntryCaseInsenitive() { assertTrue(MD5_HASHED_ENTRY_1.toLowerCase(), PasswordHash.isHashedEntry(MD5_HASHED_ENTRY_1.toLowerCase())); assertTrue(CMD5_HASHED_ENTRY_1.toLowerCase(), PasswordHash.isHashedEntry(CMD5_HASHED_ENTRY_1.toLowerCase())); + assertTrue(PBKDF2_HASHED_ENTRY_1.toLowerCase(), PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_1.toLowerCase())); + assertTrue(PBKDF2_HASHED_ENTRY_3.toLowerCase(), PasswordHash.isHashedEntry(PBKDF2_HASHED_ENTRY_3.toLowerCase())); } /** @@ -248,7 +303,7 @@ public class PasswordHashTest { * Test creating a hashed entry for scheme MD5. In this scheme there is no salt, so a direct * comparison to a constant value is possible. * - * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for MD5. + * Test method for {@link PasswordHash#toHashedEntry(String, String)} for MD5. */ @Test public void testToHashedEntryMD5() { @@ -269,7 +324,7 @@ public class PasswordHashTest { @Test(expected = IllegalArgumentException.class) public void testToHashedEntryMD5NullPassword() { PasswordHash pwdh = PasswordHash.instanceOf("MD5"); - pwdh.toHashedEntry(null, null); + pwdh.toHashedEntry((String)null, null); } @@ -277,7 +332,7 @@ public class PasswordHashTest { * Test creating a hashed entry for scheme Combined-MD5. In this scheme there is no salt, so a direct * comparison to a constant value is possible. * - * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for CMD5. + * Test method for {@link PasswordHash#toHashedEntry(String, String)} for CMD5. */ @Test public void testToHashedEntryCMD5() { @@ -295,14 +350,14 @@ public class PasswordHashTest { @Test(expected = IllegalArgumentException.class) public void testToHashedEntryCMD5NullPassword() { PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); - pwdh.toHashedEntry(null, CMD5_USERNAME_1); + pwdh.toHashedEntry((String)null, CMD5_USERNAME_1); } /** * Test creating a hashed entry for scheme Combined-MD5, when no user is given. * This should never happen in the application, so we expect an exception to be thrown. * - * Test method for {@link com.gitblit.utils.PasswordHash#toHashedEntry(String, String)} for broken CMD5. + * Test method for {@link PasswordHash#toHashedEntry(String, String)} for broken CMD5. */ @Test public void testToHashedEntryCMD5NoUsername() { @@ -332,7 +387,40 @@ public class PasswordHashTest { catch (IllegalArgumentException ignored) { /*success*/ } } + /** + * Test creating a hashed entry for scheme PBKDF2. + * Since this scheme uses a salt, we test by running a match. This is a bit backwards, + * but recreating the PBKDF2 here seems a little overkill. + * + * Test method for {@link PasswordHash#toHashedEntry(String, String)} for PBKDF2. + */ + @Test + public void testToHashedEntryPBKDF2() { + PasswordHash pwdh = PasswordHash.instanceOf("PBKDF2"); + String hashedEntry = pwdh.toHashedEntry(PBKDF2_PASSWORD_1, null); + assertTrue("Type identifier is incorrect.", hashedEntry.startsWith(PasswordHash.Type.PBKDF2.name())); + PasswordHash pwdhverify = PasswordHash.instanceFor(hashedEntry); + assertNotNull(pwdhverify); + assertTrue(PBKDF2_PASSWORD_1, pwdhverify.matches(hashedEntry, PBKDF2_PASSWORD_1.toCharArray(), null)); + + hashedEntry = pwdh.toHashedEntry(PBKDF2_PASSWORD_2, ""); + assertTrue("Type identifier is incorrect.", hashedEntry.startsWith(PasswordHash.Type.PBKDF2.name())); + pwdhverify = PasswordHash.instanceFor(hashedEntry); + assertNotNull(pwdhverify); + assertTrue(PBKDF2_PASSWORD_2, pwdhverify.matches(hashedEntry, PBKDF2_PASSWORD_2.toCharArray(), null)); + + hashedEntry = pwdh.toHashedEntry(PBKDF2_PASSWORD_0, "alpha"); + assertTrue("Type identifier is incorrect.", hashedEntry.startsWith(PasswordHash.Type.PBKDF2.name())); + pwdhverify = PasswordHash.instanceFor(hashedEntry); + assertNotNull(pwdhverify); + assertTrue(PBKDF2_PASSWORD_0, pwdhverify.matches(hashedEntry, PBKDF2_PASSWORD_0.toCharArray(), null)); + } + @Test(expected = IllegalArgumentException.class) + public void testToHashedEntryPBKDF2NullPassword() { + PasswordHash pwdh = PasswordHash.instanceOf("PBKDF2"); + pwdh.toHashedEntry((String)null, null); + } /** @@ -379,6 +467,7 @@ public class PasswordHashTest { assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong scheme", pwdh.matches(PBKDF2_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), "")); assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); } @@ -414,7 +503,94 @@ public class PasswordHashTest { assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong scheme", pwdh.matches(PBKDF2_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), "")); assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, MD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); } + + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#matches(String, char[], String)} for PBKDF2. + */ + @Test + public void testMatchesPBKDF2() { + PasswordHash pwdh = PasswordHash.instanceOf("PBKDF2"); + + assertTrue("PWD0, Null user", pwdh.matches(PBKDF2_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), null)); + assertTrue("PWD0, Empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), "")); + assertTrue("PWD0, With user", pwdh.matches(PBKDF2_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), "maxine")); + + assertTrue("PWD1, Null user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), null)); + assertTrue("PWD1, Empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), "")); + assertTrue("PWD1, With user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_1.toCharArray(), "Maxim Gorki")); + + assertTrue("PWD2, Null user", pwdh.matches(PBKDF2_HASHED_ENTRY_2, PBKDF2_PASSWORD_2.toCharArray(), null)); + assertTrue("PWD2, Empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_2, PBKDF2_PASSWORD_2.toCharArray(), "")); + assertTrue("PWD2, With user", pwdh.matches(PBKDF2_HASHED_ENTRY_2, PBKDF2_PASSWORD_2.toCharArray(), "Epson")); + + + + assertFalse("Matched wrong password", pwdh.matches(PBKDF2_HASHED_ENTRY_1, "wrongpassword".toCharArray(), null)); + assertFalse("Matched wrong password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, "wrongpassword".toCharArray(), " ")); + assertFalse("Matched wrong password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, "wrongpassword".toCharArray(), "someuser")); + + assertFalse("Matched empty password", pwdh.matches(PBKDF2_HASHED_ENTRY_2, "".toCharArray(), null)); + assertFalse("Matched empty password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_2, " ".toCharArray(), " ")); + assertFalse("Matched empty password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_2, " ".toCharArray(), "someuser")); + + assertFalse("Matched null password", pwdh.matches(PBKDF2_HASHED_ENTRY_0, null, null)); + assertFalse("Matched null password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_0, null, " ")); + assertFalse("Matched null password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_0, null, "someuser")); + + + assertFalse("Matched wrong hashed entry", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong hashed entry, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched wrong hashed entry, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_1, PBKDF2_PASSWORD_0.toCharArray(), "someuser")); + + assertFalse("Matched empty hashed entry", pwdh.matches("", PBKDF2_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched empty hashed entry, with empty user", pwdh.matches(" ", PBKDF2_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched empty hashed entry, with user", pwdh.matches(" ", PBKDF2_PASSWORD_0.toCharArray(), "someuser")); + + assertFalse("Matched null entry", pwdh.matches(null, PBKDF2_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched null entry, with empty user", pwdh.matches(null, PBKDF2_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched null entry, with user", pwdh.matches(null, PBKDF2_PASSWORD_0.toCharArray(), "someuser")); + + + assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong scheme", pwdh.matches(MD5_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched wrong scheme", pwdh.matches(CMD5_HASHED_ENTRY_0, PBKDF2_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + } + + + /** + * Test method for {@link com.gitblit.utils.PasswordHash#matches(String, char[], String)} + * for old existing entries with the "PBKDF2WITHHMACSHA256" type identifier. + */ + @Test + public void testMatchesPBKDF2Compat() { + PasswordHash pwdh = PasswordHash.instanceOf("PBKDF2"); + + assertTrue("PWD3, Null user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_3.toCharArray(), null)); + assertTrue("PWD3, Empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_3.toCharArray(), "")); + assertTrue("PWD3, With user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_3.toCharArray(), "maxine")); + + + assertFalse("Matched wrong password", pwdh.matches(PBKDF2_HASHED_ENTRY_3, "bar".toCharArray(), null)); + assertFalse("Matched wrong password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, "bar".toCharArray(), " ")); + assertFalse("Matched wrong password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, "bar".toCharArray(), "someuser")); + + assertFalse("Matched empty password", pwdh.matches(PBKDF2_HASHED_ENTRY_3, "".toCharArray(), null)); + assertFalse("Matched empty password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, " ".toCharArray(), " ")); + assertFalse("Matched empty password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, " ".toCharArray(), "someuser")); + + assertFalse("Matched null password", pwdh.matches(PBKDF2_HASHED_ENTRY_3, null, null)); + assertFalse("Matched null password, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, null, " ")); + assertFalse("Matched null password, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, null, "someuser")); + + + assertFalse("Matched wrong hashed entry", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), null)); + assertFalse("Matched wrong hashed entry, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "")); + assertFalse("Matched wrong hashed entry, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "someuser")); + } } -- cgit v1.2.3 From c09335a0305f7f345bf745cbe90c216834689425 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 5 Nov 2019 22:26:11 +0100 Subject: Use the new PasswordHash classes. Integrate the `PasswordHash` class and subclass in the user and password editing and authentication. Replaces the old code and the previous `SecurePasswordHashingUtils` class. --- src/main/distrib/data/defaults.properties | 8 +- .../java/com/gitblit/client/EditUserDialog.java | 30 ++-- .../com/gitblit/manager/AuthenticationManager.java | 57 +++---- src/main/java/com/gitblit/utils/PasswordHash.java | 82 ++++++--- .../com/gitblit/utils/SecurePasswordHashUtils.java | 190 --------------------- src/main/java/com/gitblit/utils/StringUtils.java | 4 - .../gitblit/wicket/pages/ChangePasswordPage.java | 24 +-- .../com/gitblit/wicket/pages/EditUserPage.java | 21 +-- src/site/administration.mkd | 2 +- .../gitblit/tests/AuthenticationManagerTest.java | 38 ++++- .../java/com/gitblit/utils/PasswordHashTest.java | 40 +++++ .../gitblit/utils/SecurePasswordHashUtilsTest.java | 63 ------- 12 files changed, 185 insertions(+), 374 deletions(-) delete mode 100644 src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java delete mode 100644 src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 352a6750..db0c2c69 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -854,14 +854,14 @@ realm.userService = ${baseFolder}/users.conf realm.authenticationProviders = # How to store passwords. -# Valid values are plain, md5, combined-md5 or PBKDF2WithHmacSHA256. +# Valid values are plain, md5, combined-md5 or pbkdf2. # md5 is the hash of password. # combined-md5 is the hash of username.toLowerCase()+password. -# PBKDF2WithHmacSHA256 is salt+hash(salt+password) -# Default is PBKDF2WithHmacSHA256. +# pbkdf2 implements the PBKDF2 algorithm, which is a secure, salted password hashing scheme. +# Default is pbkdf2. Using plain, md5 or combined-md5 is deprecated, as these are insecure schemes by now. # # SINCE 0.5.0 -realm.passwordStorage = PBKDF2WithHmacSHA256 +realm.passwordStorage = pbkdf2 # Minimum valid length for a plain text password. # Default value is 5. Absolute minimum is 4. diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index 141f233b..ecaed13d 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -58,7 +58,7 @@ import com.gitblit.models.RepositoryModel; import com.gitblit.models.ServerSettings; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; -import com.gitblit.utils.SecurePasswordHashUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.utils.StringUtils; @@ -319,9 +319,10 @@ public class EditUserDialog extends JDialog { minLength)); return false; } - if (!password.toUpperCase().startsWith(StringUtils.MD5_TYPE) - && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE) - && !password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)) { + // What we actually test for here, is if the password has been changed. But this also catches if the password + // was not changed, but is stored in plain-text. Which is good because then editing the user will hash the + // password if by now the storage has been changed to a hashed variant. + if (!PasswordHash.isHashedEntry(password)) { String cpw = new String(confirmPasswordField.getPassword()); if (cpw == null || cpw.length() != password.length()) { error("Please confirm the password!"); @@ -335,22 +336,17 @@ public class EditUserDialog extends JDialog { // change the cookie user.cookie = user.createCookie(); - String type = settings.get(Keys.realm.passwordStorage).getString(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - user.password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - user.password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username + password); - } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { - // store PBKDF2WithHmacSHA256 digest of password - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + String type = settings.get(Keys.realm.passwordStorage).getString(PasswordHash.getDefaultType().name()); + PasswordHash pwdHash = PasswordHash.instanceOf(type); + if (pwdHash != null) { + user.password = pwdHash.toHashedEntry(password, user.username); } else { - // plain-text password + // plain-text password. + // TODO: This is also used when the "realm.passwordStorage" configuration is not a valid type. + // This is a rather bad default, and should probably caught and changed to a secure default. user.password = password; } - } else if (rename && password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + } else if (rename && password.toUpperCase().startsWith(PasswordHash.Type.CMD5.name())) { error("Gitblit is configured for combined-md5 password hashing. You must enter a new password on account rename."); return false; } else { diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 2b54e4b8..83ca4b70 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -52,7 +52,7 @@ import com.gitblit.models.UserModel; import com.gitblit.transport.ssh.SshKey; import com.gitblit.utils.Base64; import com.gitblit.utils.HttpUtils; -import com.gitblit.utils.SecurePasswordHashUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.utils.StringUtils; import com.gitblit.utils.X509Utils.X509Metadata; import com.google.inject.Inject; @@ -519,29 +519,12 @@ public class AuthenticationManager implements IAuthenticationManager { */ protected UserModel authenticateLocal(UserModel user, char [] password) { UserModel returnedUser = null; - boolean strongHashUsed = false; - if (user.password.startsWith(StringUtils.MD5_TYPE)) { - // password digest - String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password)); - if (user.password.equalsIgnoreCase(md5)) { + PasswordHash pwdHash = PasswordHash.instanceFor(user.password); + if (pwdHash != null) { + if (pwdHash.matches(user.password, password, user.username)) { returnedUser = user; } - } else if (user.password.startsWith(StringUtils.COMBINED_MD5_TYPE)) { - // username+password digest - String md5 = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username.toLowerCase() + new String(password)); - if (user.password.equalsIgnoreCase(md5)) { - returnedUser = user; - } - } else if (user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)){ - // strong hash - SecurePasswordHashUtils hashUtils = SecurePasswordHashUtils.get(); - boolean isPasswordValid = hashUtils.isPasswordCorrect(password, user.password); - if(isPasswordValid){ - returnedUser = user; - strongHashUsed = true; - } } else if (user.password.equals(new String(password))) { // plain-text password returnedUser = user; @@ -550,33 +533,37 @@ public class AuthenticationManager implements IAuthenticationManager { // validate user returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); - // if no strong hash was used to store the password, try to update it based on the settings - if(!strongHashUsed){ - updateStoredPassword(returnedUser, password); - } - + // try to upgrade the stored password hash to a stronger hash, if necessary + upgradeStoredPassword(returnedUser, password, pwdHash); + return returnedUser; } /** - * Update stored password to a strong hash if configured. + * Upgrade stored password to a strong hash if configured. * * @param user the user to be updated * @param password the password + * @param pwdHash + * Instance of PasswordHash for the stored password entry. If null, no current hashing is assumed. */ - protected void updateStoredPassword(UserModel user, char[] password) { + private void upgradeStoredPassword(UserModel user, char[] password, PasswordHash pwdHash) { // check if user has successfully authenticated i.e. is not null - if(user != null){ - // check if strong hash algorithm is configured - String algorithm = settings.getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if(algorithm.equals(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)){ - // rehash the provided correct password and update the user model - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + if (user == null) return; + + // check if strong hash algorithm is configured + String algorithm = settings.getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + if (pwdHash == null || pwdHash.needsUpgradeTo(algorithm)) { + // rehash the provided correct password and update the user model + pwdHash = PasswordHash.instanceOf(algorithm); + if (pwdHash != null) { // necessary since the algorithm name could be something not supported. + user.password = pwdHash.toHashedEntry(password, user.username); userManager.updateUserModel(user); - } + } } } + /** * Returns the Gitlbit cookie in the request. * diff --git a/src/main/java/com/gitblit/utils/PasswordHash.java b/src/main/java/com/gitblit/utils/PasswordHash.java index 13747805..ee60dfb6 100644 --- a/src/main/java/com/gitblit/utils/PasswordHash.java +++ b/src/main/java/com/gitblit/utils/PasswordHash.java @@ -31,14 +31,27 @@ import java.util.Arrays; */ public abstract class PasswordHash { - /** * The types of implemented password hashing schemes. */ - enum Type { + public enum Type { MD5, CMD5, - PBKDF2 + PBKDF2; + + static Type fromName(String name) { + if (name == null) return null; + for (Type type : Type.values()) { + if (type.name().equalsIgnoreCase(name)) return type; + } + // Compatibility with type id "PBKDF2WITHHMACSHA256", which is also handled by PBKDF2 type. + if (name.equalsIgnoreCase("PBKDF2WITHHMACSHA256")) return Type.PBKDF2; + + // Recognise the name used for CMD5 in the settings file. + if (name.equalsIgnoreCase("combined-md5")) return Type.CMD5; + + return null; + } } /** @@ -57,6 +70,14 @@ public abstract class PasswordHash { } + /** + * When no hash type is specified, this determines the default that should be used. + */ + public static Type getDefaultType() { + return Type.PBKDF2; + } + + /** * Create an instance of a password hashing class for the given hash type. * @@ -66,21 +87,17 @@ public abstract class PasswordHash { * or null if the given hash type is not a valid one. */ public static PasswordHash instanceOf(String type) { - try { - Type hashType = Type.valueOf(type.toUpperCase()); - switch (hashType) { - case MD5: - return new PasswordHashMD5(); - case CMD5: - return new PasswordHashCombinedMD5(); - case PBKDF2: - return new PasswordHashPbkdf2(); - default: - return null; - } - } - catch (Exception e) { - return null; + Type hashType = Type.fromName(type); + if (hashType == null) return null; + switch (hashType) { + case MD5: + return new PasswordHashMD5(); + case CMD5: + return new PasswordHashCombinedMD5(); + case PBKDF2: + return new PasswordHashPbkdf2(); + default: + return null; } } @@ -115,6 +132,27 @@ public abstract class PasswordHash { } + /** + * Some hash methods are considered more secure than others. This method determines for a certain type + * of password hash if it is inferior than a given other type and stored passwords should be + * upgraded to the given hashing method. + * + * @param algorithm + * Password hashing type to be checked if it is superior than the one of this instance. + * @return True, if the given type in parameter {@code algorithm} is better and stored passwords should be upgraded to it, + * false, otehrwise. + */ + public boolean needsUpgradeTo(String algorithm) { + Type hashType = Type.fromName(algorithm); + if (hashType == null) return false; + if (this.type == hashType) return false; + + // Right now we keep it really simple. With the existing types, only PBKDF2 is considered secure, + // everything else is inferior. This will need to be updated once more secure hashing algorithms + // are implemented, or the workload/parameters of the PBKDF2 are changed. + return hashType == Type.PBKDF2; + } + /** * Convert the given password to a hashed password entry to be stored in the user table. @@ -181,13 +219,7 @@ public abstract class PasswordHash { int indexOfSeparator = hashedEntry.indexOf(':'); if (indexOfSeparator <= 0) return null; String typeId = hashedEntry.substring(0, indexOfSeparator); - - // Compatibility with type id "PBKDF2WITHHMACSHA256", which is also handled by PBKDF2 type. - if (typeId.equalsIgnoreCase("PBKDF2WITHHMACSHA256")) return Type.PBKDF2; - try { - return Type.valueOf(typeId.toUpperCase()); - } - catch (Exception e) { return null;} + return Type.fromName(typeId); } diff --git a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java b/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java deleted file mode 100644 index 289084ee..00000000 --- a/src/main/java/com/gitblit/utils/SecurePasswordHashUtils.java +++ /dev/null @@ -1,190 +0,0 @@ -package com.gitblit.utils; - -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; - -import org.apache.commons.codec.DecoderException; -import org.apache.commons.codec.binary.Hex; - -import java.security.NoSuchAlgorithmException; -import java.security.spec.InvalidKeySpecException; -import java.util.Arrays; - -/** - * The Class SecurePasswordHashUtils provides methods to create and validate - * secure hashes from user passwords. - * - * It uses the concept proposed by OWASP - Hashing Java: - * https://www.owasp.org/index.php/Hashing_Java - */ -public class SecurePasswordHashUtils { - - public static final String PBKDF2WITHHMACSHA256 = "PBKDF2WithHmacSHA256"; - public static final String PBKDF2WITHHMACSHA256_TYPE = PBKDF2WITHHMACSHA256.toUpperCase() + ":"; - - private static final SecureRandom RANDOM = new SecureRandom(); - private static final int ITERATIONS = 10000; - private static final int KEY_LENGTH = 256; - private static final int SALT_LENGTH = 32; - - /** The instance. */ - private static SecurePasswordHashUtils instance; - - /** - * Instantiates a new secure password hash utils. - */ - private SecurePasswordHashUtils() { - } - - /** - * Gets an instance of {@link SecurePasswordHashUtils}. - * - * @return the secure password hash utils - */ - public static SecurePasswordHashUtils get() { - if (instance == null) { - instance = new SecurePasswordHashUtils(); - } - return instance; - } - - /** - * Gets the next salt. - * - * @return the next salt - */ - protected byte[] getNextSalt() { - byte[] salt = new byte[SALT_LENGTH]; - RANDOM.nextBytes(salt); - return salt; - } - - /** - * Hash. - * - * @param password - * the password - * @param salt - * the salt - * @return the byte[] - */ - protected byte[] hash(char[] password, byte[] salt) { - PBEKeySpec spec = new PBEKeySpec(password, salt, ITERATIONS, KEY_LENGTH); - Arrays.fill(password, Character.MIN_VALUE); - try { - SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256"); - return skf.generateSecret(spec).getEncoded(); - } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new IllegalStateException("Error while hashing password", e); - } finally { - spec.clearPassword(); - } - } - - /** - * Checks if is password correct. - * - * @param passwordToCheck - * the password to check - * @param salt - * the salt - * @param expectedHash - * the expected hash - * @return true, if is password correct - */ - protected boolean isPasswordCorrect(char[] passwordToCheck, byte[] salt, byte[] expectedHash) { - byte[] hashToCheck = hash(passwordToCheck, salt); - Arrays.fill(passwordToCheck, Character.MIN_VALUE); - if (hashToCheck.length != expectedHash.length) { - return false; - } - for (int i = 0; i < hashToCheck.length; i++) { - if (hashToCheck[i] != expectedHash[i]) { - return false; - } - } - return true; - } - - /** - * Creates the new secure hash from a password and formats it properly to be - * stored in a file. - * - * @param password - * the password to be hashed - * @return the sting to be stored in a file (users.conf) - */ - public String createStoredPasswordFromPassword(String password) { - return createStoredPasswordFromPassword(password.toCharArray()); - } - - /** - * Creates the new secure hash from a password and formats it properly to be - * stored in a file. - * - * @param password - * the password to be hashed - * @return the sting to be stored in a file (users.conf) - */ - public String createStoredPasswordFromPassword(char[] password) { - byte[] salt = getNextSalt(); - return String.format("%s%s%s", SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE, StringUtils.toHex(salt), - StringUtils.toHex(hash(password, salt))); - } - - /** - * Gets the salt from stored password. - * - * @param storedPassword - * the stored password - * @return the salt from stored password - */ - protected byte[] getSaltFromStoredPassword(String storedPassword) { - byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); - return Arrays.copyOfRange(pw, 0, SALT_LENGTH); - } - - /** - * Gets the hash from stored password. - * - * @param storedPassword - * the stored password - * @return the hash from stored password - */ - protected byte[] getHashFromStoredPassword(String storedPassword) { - byte[] pw = getStoredHashWithStrippedPrefix(storedPassword); - return Arrays.copyOfRange(pw, SALT_LENGTH, pw.length); - } - - /** - * Strips the prefix ({@link #PBKDF2WITHHMACSHA256_TYPE}) from a stored - * password and returns the decoded hash - * - * @param storedPassword - * the stored password - * @return the stored hash with stripped prefix - */ - protected byte[] getStoredHashWithStrippedPrefix(String storedPassword) { - String saltAndHash = storedPassword.substring(PBKDF2WITHHMACSHA256_TYPE.length()); - try { - return Hex.decodeHex(saltAndHash.toCharArray()); - } catch (DecoderException e) { - throw new IllegalStateException("Error while reading stored credentials", e); - } - } - - /** - * Checks if is password correct. - * - * @param password - * the password - * @param storedPassword - * the stored password - * @return true, if is password correct - */ - public boolean isPasswordCorrect(char[] password, String storedPassword) { - byte[] storedSalt = getSaltFromStoredPassword(storedPassword); - byte[] storedHash = getHashFromStoredPassword(storedPassword); - return isPasswordCorrect(password, storedSalt, storedHash); - } -} diff --git a/src/main/java/com/gitblit/utils/StringUtils.java b/src/main/java/com/gitblit/utils/StringUtils.java index 643c52c3..b192c80b 100644 --- a/src/main/java/com/gitblit/utils/StringUtils.java +++ b/src/main/java/com/gitblit/utils/StringUtils.java @@ -46,10 +46,6 @@ import java.util.regex.PatternSyntaxException; */ public class StringUtils { - public static final String MD5_TYPE = "MD5:"; - - public static final String COMBINED_MD5_TYPE = "CMD5:"; - /** * Returns true if the string is null or empty. * diff --git a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java index 4c074abc..53c3e14d 100644 --- a/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java +++ b/src/main/java/com/gitblit/wicket/pages/ChangePasswordPage.java @@ -28,15 +28,14 @@ import org.apache.wicket.protocol.http.WebResponse; import com.gitblit.GitBlitException; import com.gitblit.Keys; import com.gitblit.models.UserModel; -import com.gitblit.utils.SecurePasswordHashUtils; -import com.gitblit.utils.StringUtils; +import com.gitblit.utils.PasswordHash; import com.gitblit.wicket.GitBlitWebSession; import com.gitblit.wicket.NonTrimmedPasswordTextField; public class ChangePasswordPage extends RootSubPage { - IModel password = new Model(""); - IModel confirmPassword = new Model(""); + private IModel password = new Model(""); + private IModel confirmPassword = new Model(""); public ChangePasswordPage() { super(); @@ -86,18 +85,11 @@ public class ChangePasswordPage extends RootSubPage { UserModel user = GitBlitWebSession.get().getUser(); - // convert to MD5 digest, if appropriate - String type = app().settings().getString(Keys.realm.passwordStorage, SecurePasswordHashUtils.PBKDF2WITHHMACSHA256); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - password = StringUtils.MD5_TYPE + StringUtils.getMD5(password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(user.username.toLowerCase() + password); - } else if (type.equalsIgnoreCase(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256)) { - // store PBKDF2WithHmacSHA256 digest of password - user.password = SecurePasswordHashUtils.get().createStoredPasswordFromPassword(password); + // convert to digest, if appropriate + String type = app().settings().getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + PasswordHash pwdHash = PasswordHash.instanceOf(type); + if (pwdHash != null) { + password = pwdHash.toHashedEntry(password, user.username); } user.password = password; diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java index 72dee6b6..94dd35ab 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java +++ b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import com.gitblit.utils.PasswordHash; import org.apache.wicket.PageParameters; import org.apache.wicket.behavior.SimpleAttributeModifier; import org.apache.wicket.extensions.markup.html.form.palette.Palette; @@ -141,15 +142,14 @@ public class EditUserPage extends RootSubPage { return; } String password = userModel.password; - if (!password.toUpperCase().startsWith(StringUtils.MD5_TYPE) - && !password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + if (!PasswordHash.isHashedEntry(password)) { // This is a plain text password. // Check length. int minLength = app().settings().getInteger(Keys.realm.minPasswordLength, 5); if (minLength < 4) { minLength = 4; } - if (password.trim().length() < minLength) { + if (password.trim().length() < minLength) { // TODO: Why do we trim here, but not in EditUserDialog and ChangePasswordPage? error(MessageFormat.format(getString("gb.passwordTooShort"), minLength)); return; @@ -159,18 +159,13 @@ public class EditUserPage extends RootSubPage { userModel.cookie = userModel.createCookie(); // Optionally store the password MD5 digest. - String type = app().settings().getString(Keys.realm.passwordStorage, "md5"); - if (type.equalsIgnoreCase("md5")) { - // store MD5 digest of password - userModel.password = StringUtils.MD5_TYPE - + StringUtils.getMD5(userModel.password); - } else if (type.equalsIgnoreCase("combined-md5")) { - // store MD5 digest of username+password - userModel.password = StringUtils.COMBINED_MD5_TYPE - + StringUtils.getMD5(username + userModel.password); + String type = app().settings().getString(Keys.realm.passwordStorage, PasswordHash.getDefaultType().name()); + PasswordHash pwdh = PasswordHash.instanceOf(type); + if (pwdh != null) { // Hash the password + userModel.password = pwdh.toHashedEntry(password, username); } } else if (rename - && password.toUpperCase().startsWith(StringUtils.COMBINED_MD5_TYPE)) { + && password.toUpperCase().startsWith(PasswordHash.Type.CMD5.name())) { error(getString("gb.combinedMd5Rename")); return; } diff --git a/src/site/administration.mkd b/src/site/administration.mkd index 5feedee9..fed71e8a 100644 --- a/src/site/administration.mkd +++ b/src/site/administration.mkd @@ -169,7 +169,7 @@ Usernames must be unique and are case-insensitive. Whitespace is illegal. ### Passwords -User passwords are CASE-SENSITIVE and may be *plain*, *md5*, *combined-md5* or *PBKDF2WithHmacSHA256* formatted (see `gitblit.properties` -> *realm.passwordStorage*). +User passwords are CASE-SENSITIVE and may be *plain*, *md5*, *combined-md5* or *pbkdf2* formatted (see `gitblit.properties` -> *realm.passwordStorage*). ### User Roles There are four actual *roles* in Gitblit: diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java index 31b7512c..45009856 100644 --- a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -43,6 +43,7 @@ import javax.servlet.http.HttpSessionContext; import javax.servlet.http.HttpUpgradeHandler; import javax.servlet.http.Part; +import com.gitblit.utils.PasswordHash; import org.junit.Test; import com.gitblit.IUserService; @@ -55,7 +56,6 @@ import com.gitblit.manager.UserManager; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; import com.gitblit.tests.mock.MemorySettings; -import com.gitblit.utils.SecurePasswordHashUtils; import com.gitblit.utils.XssFilter; import com.gitblit.utils.XssFilter.AllowXssFilter; @@ -659,17 +659,43 @@ public class AuthenticationManagerTest extends GitblitUnitTest { users.updateUserModel(user); assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); - - // validate that plaintext password was automatically updated to hashed one - assertTrue(user.password.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); - user.disabled = true; users.updateUserModel(user); assertNull(auth.authenticate(user.username, user.password.toCharArray(), null)); users.deleteUserModel(user); } - + + + @Test + public void testAuthenticateUpgradePlaintext() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + UserModel user = new UserModel("sunnyjim"); + user.password = "password"; + users.updateUserModel(user); + + assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); + + // validate that plaintext password was automatically updated to hashed one + assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); + } + + + @Test + public void testAuthenticateUpgradeMD5() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + UserModel user = new UserModel("sunnyjim"); + user.password = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; + users.updateUserModel(user); + + assertNotNull(auth.authenticate(user.username, "password".toCharArray(), null)); + + // validate that MD5 password was automatically updated to hashed one + assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); + } + @Test public void testContenairAuthenticate() throws Exception { diff --git a/src/test/java/com/gitblit/utils/PasswordHashTest.java b/src/test/java/com/gitblit/utils/PasswordHashTest.java index c5a485dc..40c472aa 100644 --- a/src/test/java/com/gitblit/utils/PasswordHashTest.java +++ b/src/test/java/com/gitblit/utils/PasswordHashTest.java @@ -99,6 +99,15 @@ public class PasswordHashTest { assertTrue("Failed to match " +CMD5_HASHED_ENTRY_0, pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); + pwdh = PasswordHash.instanceOf("combined-md5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + + pwdh = PasswordHash.instanceOf("COMBINED-MD5"); + assertNotNull(pwdh); + assertEquals(PasswordHash.Type.CMD5, pwdh.type); + + pwdh = PasswordHash.instanceOf("MD5"); assertNotNull(pwdh); assertNotEquals(PasswordHash.Type.CMD5, pwdh.type); @@ -593,4 +602,35 @@ public class PasswordHashTest { assertFalse("Matched wrong hashed entry, with empty user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "")); assertFalse("Matched wrong hashed entry, with user", pwdh.matches(PBKDF2_HASHED_ENTRY_3, PBKDF2_PASSWORD_0.toCharArray(), "someuser")); } + + @Test + public void getEntryType() { + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("MD5:blah")); + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("md5:blah")); + assertEquals(PasswordHash.Type.MD5, PasswordHash.getEntryType("mD5:blah")); + + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("CMD5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("cmd5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("Cmd5:blah")); + + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("combined-md5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("COMBINED-MD5:blah")); + assertEquals(PasswordHash.Type.CMD5, PasswordHash.getEntryType("combined-MD5:blah")); + + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("pbkdf2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("Pbkdf2:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("pbKDF2:blah")); + + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2WithHmacSHA256:blah")); + assertEquals(PasswordHash.Type.PBKDF2, PasswordHash.getEntryType("PBKDF2WITHHMACSHA256:blah")); + } + + @Test + public void getEntryValue() { + assertEquals("value", PasswordHash.getEntryValue("MD5:value")); + assertEquals("plain text", PasswordHash.getEntryValue("plain text")); + assertEquals("what this", PasswordHash.getEntryValue(":what this")); + assertEquals("", PasswordHash.getEntryValue(":")); + } } diff --git a/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java b/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java deleted file mode 100644 index f687bda6..00000000 --- a/src/test/java/com/gitblit/utils/SecurePasswordHashUtilsTest.java +++ /dev/null @@ -1,63 +0,0 @@ -package com.gitblit.utils; - -import static org.junit.Assert.*; - -import org.junit.Before; -import org.junit.Test; - -public class SecurePasswordHashUtilsTest { - - private static final String STORED_PASSWORD = "PBKDF2WITHHMACSHA256:2d7d3ccaa277787f288e9f929247361bfc83607c6a8447bf496267512e360ba0a97b3114937213b23230072517d65a2e00695a1cbc47a732510840817f22c1bc"; - private static final byte[] STORED_SALT_BYTES = new byte[]{45, 125, 60, -54, -94, 119, 120, 127, 40, -114, -97, -110, -110, 71, 54, 27, -4, -125, 96, 124, 106, -124, 71, -65, 73, 98, 103, 81, 46, 54, 11, -96}; - private static final byte[] STORED_HASH_BYTES = new byte[]{-87, 123, 49, 20, -109, 114, 19, -78, 50, 48, 7, 37, 23, -42, 90, 46, 0, 105, 90, 28, -68, 71, -89, 50, 81, 8, 64, -127, 127, 34, -63, -68}; - - private SecurePasswordHashUtils utils; - - @Before - public void init(){ - utils = SecurePasswordHashUtils.get(); - } - - @Test - public void testGetNextSalt() { - assertEquals(32, utils.getNextSalt().length); - } - - @Test - public void testHash() { - byte[] hash = utils.hash("foo".toCharArray(), STORED_SALT_BYTES); - assertArrayEquals(STORED_HASH_BYTES, hash); - } - - @Test - public void testIsPasswordCorrectCharArrayByteArrayByteArray() { - assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); - assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_SALT_BYTES, STORED_HASH_BYTES)); - } - - @Test - public void testCreateNewStorableHashFromPassword() { - String newPwHash = utils.createStoredPasswordFromPassword("foo"); - assertTrue(newPwHash.startsWith(SecurePasswordHashUtils.PBKDF2WITHHMACSHA256_TYPE)); - } - - @Test - public void testGetSaltFromStoredPassword() { - byte[] saltFromStoredPassword = utils.getSaltFromStoredPassword(STORED_PASSWORD); - assertArrayEquals(STORED_SALT_BYTES, saltFromStoredPassword); - - } - - @Test - public void testGetHashFromStoredPassword() { - byte[] hashFromStoredPassword = utils.getHashFromStoredPassword(STORED_PASSWORD); - assertArrayEquals(STORED_HASH_BYTES, hashFromStoredPassword); - } - - @Test - public void testIsPasswordCorrectCharArrayString() { - assertTrue(utils.isPasswordCorrect("foo".toCharArray(), STORED_PASSWORD)); - assertFalse(utils.isPasswordCorrect("bar".toCharArray(), STORED_PASSWORD)); - } - -} -- cgit v1.2.3 From b85267c81bac8168186ec78dace3ef2ec6b8cf24 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Wed, 6 Nov 2019 12:35:31 +0100 Subject: Add more PasswordHash tests with strings beyond iso-8859-1. --- .../java/com/gitblit/utils/PasswordHashTest.java | 32 +++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/gitblit/utils/PasswordHashTest.java b/src/test/java/com/gitblit/utils/PasswordHashTest.java index 40c472aa..2fbf6580 100644 --- a/src/test/java/com/gitblit/utils/PasswordHashTest.java +++ b/src/test/java/com/gitblit/utils/PasswordHashTest.java @@ -30,12 +30,24 @@ public class PasswordHashTest { static final String MD5_HASHED_ENTRY_0 = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; static final String MD5_PASSWORD_1 = "This is a test password"; static final String MD5_HASHED_ENTRY_1 = "md5:8e1901831af502c0f842d4efb9083bcf"; + static final String MD5_PASSWORD_2 = "版本库管理方案"; + static final String MD5_HASHED_ENTRY_2 = "MD5:980017891ff67cf8a20f23aa810e7b5a"; + static final String MD5_PASSWORD_3 = "PÿrâṃiĐ"; + static final String MD5_HASHED_ENTRY_3 = "MD5:60359b7e22941164708ae2040040521f"; + static final String CMD5_USERNAME_0 = "Jane Doe"; static final String CMD5_PASSWORD_0 = "password"; static final String CMD5_HASHED_ENTRY_0 = "CMD5:DB9639A6E5F21457F9DFD7735FAFA68B"; static final String CMD5_USERNAME_1 = "Joe Black"; static final String CMD5_PASSWORD_1 = "ThisIsAWeirdScheme.Weird"; static final String CMD5_HASHED_ENTRY_1 = "cmd5:5c154768287e32fa605656b98894da89"; + static final String CMD5_USERNAME_2 = "快速便"; + static final String CMD5_PASSWORD_2 = "版本库管理方案"; + static final String CMD5_HASHED_ENTRY_2 = "CMD5:f38575ee8af23ba6d923c0d98ee767fc"; + static final String CMD5_USERNAME_3 = "İńa"; + static final String CMD5_PASSWORD_3 = "PÿrâṃiĐ"; + static final String CMD5_HASHED_ENTRY_3 = "CMD5:f1cdc2348c907677529e0e1b011f6793"; + static final String PBKDF2_PASSWORD_0 = "password"; static final String PBKDF2_HASHED_ENTRY_0 = "PBKDF2:70617373776f726450415353574f524470617373776f726450415353574f52440f17d16621b32ae1bb2b1041fcb19e294b35d514d361c08eed385766e38f6f3a"; static final String PBKDF2_PASSWORD_1 = "A REALLY better scheme than MD5"; @@ -320,9 +332,16 @@ public class PasswordHashTest { String hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_1, null); assertTrue(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_2, null); + assertTrue(MD5_HASHED_ENTRY_2.equalsIgnoreCase(hashedEntry)); + hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_1, "charlie"); assertTrue(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + hashedEntry = pwdh.toHashedEntry(MD5_PASSWORD_3, CMD5_USERNAME_3); + assertTrue(MD5_HASHED_ENTRY_3.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry("badpassword", "charlie"); assertFalse(MD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); @@ -349,6 +368,13 @@ public class PasswordHashTest { String hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, CMD5_USERNAME_1); assertTrue(CMD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); + hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_2, CMD5_USERNAME_2); + assertTrue(CMD5_HASHED_ENTRY_2.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_3, CMD5_USERNAME_3); + assertTrue(CMD5_HASHED_ENTRY_3.equalsIgnoreCase(hashedEntry)); + + hashedEntry = pwdh.toHashedEntry(CMD5_PASSWORD_1, "charlie"); assertFalse(CMD5_HASHED_ENTRY_1.equalsIgnoreCase(hashedEntry)); @@ -447,6 +473,8 @@ public class PasswordHashTest { assertTrue("PWD1, Empty user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), "")); assertTrue("PWD1, With user", pwdh.matches(MD5_HASHED_ENTRY_1, MD5_PASSWORD_1.toCharArray(), "maxine")); + assertTrue("PWD2", pwdh.matches(MD5_HASHED_ENTRY_2, MD5_PASSWORD_2.toCharArray(), null)); + assertTrue("PWD3", pwdh.matches(MD5_HASHED_ENTRY_3, MD5_PASSWORD_3.toCharArray(), null)); assertFalse("Matched wrong password", pwdh.matches(MD5_HASHED_ENTRY_1, "wrongpassword".toCharArray(), null)); @@ -488,7 +516,9 @@ public class PasswordHashTest { PasswordHash pwdh = PasswordHash.instanceOf("CMD5"); assertTrue("PWD0", pwdh.matches(CMD5_HASHED_ENTRY_0, CMD5_PASSWORD_0.toCharArray(), CMD5_USERNAME_0)); - assertTrue("Empty user", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + assertTrue("PWD1", pwdh.matches(CMD5_HASHED_ENTRY_1, CMD5_PASSWORD_1.toCharArray(), CMD5_USERNAME_1)); + assertTrue("PWD2", pwdh.matches(CMD5_HASHED_ENTRY_2, CMD5_PASSWORD_2.toCharArray(), CMD5_USERNAME_2)); + assertTrue("PWD3", pwdh.matches(CMD5_HASHED_ENTRY_3, CMD5_PASSWORD_3.toCharArray(), CMD5_USERNAME_3)); -- cgit v1.2.3