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(-) 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