From e47647b00d566d64d311042981e6b1798f683e4a Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 4 Apr 2020 19:25:27 +0200 Subject: 🦟 fix: Password hash upgrade kills existing passwords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upgrade of a MD5 stored password hash to a PBKDF password hash destroys the stored password. The has check zeroes out the password that is tested, so that the new hash is built over the zeroed out value. This fix prevents that an also adds a check to the test. Fixes #1335 --- .../com/gitblit/tests/AuthenticationManagerTest.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'src/test/java/com/gitblit') diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java index 45009856..1c6de3b2 100644 --- a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -671,14 +671,18 @@ public class AuthenticationManagerTest extends GitblitUnitTest { public void testAuthenticateUpgradePlaintext() throws Exception { IAuthenticationManager auth = newAuthenticationManager(); + String password = "topsecret"; UserModel user = new UserModel("sunnyjim"); - user.password = "password"; + user.password = password; users.updateUserModel(user); - assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); + assertNotNull(auth.authenticate(user.username, password.toCharArray(), null)); // validate that plaintext password was automatically updated to hashed one assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); + + // validate that the password is still valid and the user can log in + assertNotNull(auth.authenticate(user.username, password.toCharArray(), null)); } @@ -686,14 +690,18 @@ public class AuthenticationManagerTest extends GitblitUnitTest { public void testAuthenticateUpgradeMD5() throws Exception { IAuthenticationManager auth = newAuthenticationManager(); + String password = "secretAndHashed"; UserModel user = new UserModel("sunnyjim"); - user.password = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; + user.password = "MD5:BD95A1CFD00868B59B3564112D1E5847"; users.updateUserModel(user); - assertNotNull(auth.authenticate(user.username, "password".toCharArray(), null)); + 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() + ":")); + + // validate that the password is still valid and the user can log in + assertNotNull(auth.authenticate(user.username, password.toCharArray(), null)); } -- cgit v1.2.3 From 803d4171bf24e82612c526d65de77aa580c8a62f Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 4 Apr 2020 19:43:35 +0200 Subject: Delete password from memory in AuthenticationManager Zero out the password to remove it from memory after use. This is only a first step, implementing it for one method: `AuthenticationManager.authenticate(String, char[], String)`. --- .../com/gitblit/manager/AuthenticationManager.java | 42 ++++++----- src/main/java/com/gitblit/utils/StringUtils.java | 15 ++++ .../gitblit/tests/AuthenticationManagerTest.java | 84 +++++++++++++++++++--- .../java/com/gitblit/tests/StringUtilsTest.java | 15 +++- 4 files changed, 125 insertions(+), 31 deletions(-) (limited to 'src/test/java/com/gitblit') diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 4f3f4f85..68c83dae 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -452,7 +452,6 @@ public class AuthenticationManager implements IAuthenticationManager { /** * Authenticate a user based on a username and password. * - * @see IUserService.authenticate(String, char[]) * @param username * @param password * @return a user object or null @@ -471,34 +470,39 @@ public class AuthenticationManager implements IAuthenticationManager { } String usernameDecoded = StringUtils.decodeUsername(username); - String pw = new String(password); - if (StringUtils.isEmpty(pw)) { + if (StringUtils.isEmpty(password)) { // can not authenticate empty password return null; } UserModel user = userManager.getUserModel(usernameDecoded); - // try local authentication - if (user != null && user.isLocalAccount()) { - UserModel returnedUser = authenticateLocal(user, password); - if (returnedUser != null) { - // user authenticated - return returnedUser; - } - } else { - // try registered external authentication providers - for (AuthenticationProvider provider : authenticationProviders) { - if (provider instanceof UsernamePasswordAuthenticationProvider) { - UserModel returnedUser = provider.authenticate(usernameDecoded, password); - if (returnedUser != null) { - // user authenticated - returnedUser.accountType = provider.getAccountType(); - return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); + try { + // try local authentication + if (user != null && user.isLocalAccount()) { + UserModel returnedUser = authenticateLocal(user, password); + if (returnedUser != null) { + // user authenticated + return returnedUser; + } + } else { + // try registered external authentication providers + for (AuthenticationProvider provider : authenticationProviders) { + if (provider instanceof UsernamePasswordAuthenticationProvider) { + UserModel returnedUser = provider.authenticate(usernameDecoded, password); + if (returnedUser != null) { + // user authenticated + returnedUser.accountType = provider.getAccountType(); + return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); + } } } } } + finally { + // Zero out password array to delete password from memory + Arrays.fill(password, Character.MIN_VALUE); + } // could not authenticate locally or with a provider logger.warn(MessageFormat.format("Failed login attempt for {0}, invalid credentials from {1}", username, diff --git a/src/main/java/com/gitblit/utils/StringUtils.java b/src/main/java/com/gitblit/utils/StringUtils.java index b192c80b..442acbbf 100644 --- a/src/main/java/com/gitblit/utils/StringUtils.java +++ b/src/main/java/com/gitblit/utils/StringUtils.java @@ -56,6 +56,21 @@ public class StringUtils { return value == null || value.trim().length() == 0; } + /** + * Returns true if the character array represents an empty String. + * An empty character sequence is defined as a sequence that + * either has no characters at all, or no characters above + * '\u0020' (space). + * + * @param value + * @return true if value is null or represents an empty String + */ + public static boolean isEmpty(char[] value) { + if (value == null || value.length == 0) return true; + for ( char c : value) if (c > '\u0020') return false; + return true; + } + /** * Replaces carriage returns and line feeds with html line breaks. * diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java index 1c6de3b2..81d68895 100644 --- a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -19,13 +19,7 @@ import java.io.BufferedReader; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.security.Principal; -import java.util.Collection; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashMap; -import java.util.List; -import java.util.Locale; -import java.util.Map; +import java.util.*; import javax.servlet.AsyncContext; import javax.servlet.DispatcherType; @@ -654,16 +648,84 @@ public class AuthenticationManagerTest extends GitblitUnitTest { public void testAuthenticate() throws Exception { IAuthenticationManager auth = newAuthenticationManager(); + + String password = "pass word"; UserModel user = new UserModel("sunnyjim"); - user.password = "password"; + user.password = password; users.updateUserModel(user); - assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null)); + char[] pwd = password.toCharArray(); + assertNotNull(auth.authenticate(user.username, pwd, null)); + + // validate that the passed in password has been zeroed out in memory + char[] zeroes = new char[pwd.length]; + Arrays.fill(zeroes, Character.MIN_VALUE); + assertArrayEquals(zeroes, pwd); + } + + + @Test + public void testAuthenticateDisabledUser() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + + String password = "password"; + UserModel user = new UserModel("sunnyjim"); + user.password = password; user.disabled = true; + users.updateUserModel(user); + + assertNull(auth.authenticate(user.username, password.toCharArray(), null)); + + user.disabled = false; + users.updateUserModel(user); + assertNotNull(auth.authenticate(user.username, password.toCharArray(), null)); + } + + + @Test + public void testAuthenticateEmptyPassword() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + + String password = "password"; + UserModel user = new UserModel("sunnyjim"); + user.password = password; + users.updateUserModel(user); + + assertNull(auth.authenticate(user.username, "".toCharArray(), null)); + assertNull(auth.authenticate(user.username, " ".toCharArray(), null)); + assertNull(auth.authenticate(user.username, new char[]{' ', '\u0010', '\u0015'}, null)); + } + + + + @Test + public void testAuthenticateWrongPassword() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + + String password = "password"; + UserModel user = new UserModel("sunnyjim"); + user.password = password; users.updateUserModel(user); - assertNull(auth.authenticate(user.username, user.password.toCharArray(), null)); - users.deleteUserModel(user); + + assertNull(auth.authenticate(user.username, "helloworld".toCharArray(), null)); + } + + + @Test + public void testAuthenticateNoSuchUser() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + + String password = "password"; + UserModel user = new UserModel("sunnyjim"); + user.password = password; + users.updateUserModel(user); + + assertNull(auth.authenticate("rainyjoe", password.toCharArray(), null)); } diff --git a/src/test/java/com/gitblit/tests/StringUtilsTest.java b/src/test/java/com/gitblit/tests/StringUtilsTest.java index 7176b88c..3dae66f4 100644 --- a/src/test/java/com/gitblit/tests/StringUtilsTest.java +++ b/src/test/java/com/gitblit/tests/StringUtilsTest.java @@ -26,12 +26,25 @@ public class StringUtilsTest extends GitblitUnitTest { @Test public void testIsEmpty() throws Exception { - assertTrue(StringUtils.isEmpty(null)); + assertTrue(StringUtils.isEmpty((String)null)); assertTrue(StringUtils.isEmpty("")); assertTrue(StringUtils.isEmpty(" ")); assertFalse(StringUtils.isEmpty("A")); } + @Test + public void testIsEmptyCharArray() throws Exception { + assertTrue(StringUtils.isEmpty((char[])null)); + assertTrue(StringUtils.isEmpty(new char[0])); + assertTrue(StringUtils.isEmpty(new char[]{ ' ' })); + assertTrue(StringUtils.isEmpty(new char[]{ ' '})); + assertTrue(StringUtils.isEmpty(new char[]{ ' ', ' ' })); + assertTrue(StringUtils.isEmpty(new char[]{ ' ', ' ', ' ' })); + assertFalse(StringUtils.isEmpty(new char[]{ '\u0020', 'f' })); + assertFalse(StringUtils.isEmpty(new char[]{ '\u0148', '\u0020' })); + assertFalse(StringUtils.isEmpty(new char[]{ 'A' })); + } + @Test public void testBreakLinesForHtml() throws Exception { String input = "this\nis\r\na\rtest\r\n\r\nof\n\nline\r\rbreaking"; -- cgit v1.2.3