From 4ab81b3465f086f9fbeadc93d6bce326208e85ac Mon Sep 17 00:00:00 2001 From: Martin Spielmann Date: Sat, 7 Jan 2017 13:47:42 +0100 Subject: [PATCH] Update AuthenticationManager to update weakly stored passwords on login --- .../manager/AuthenticationManager.java | 41 ++++++++++++++++--- .../utils/SecurePasswordHashUtils.java | 24 +++++++++-- .../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 { -- 2.39.5