diff options
Diffstat (limited to 'src/main/java/com')
| -rw-r--r-- | src/main/java/com/gitblit/manager/AuthenticationManager.java | 85 | ||||
| -rw-r--r-- | src/main/java/com/gitblit/utils/StringUtils.java | 15 |
2 files changed, 64 insertions, 36 deletions
diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 83ca4b70..68c83dae 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -18,10 +18,7 @@ package com.gitblit.manager; import java.nio.charset.Charset; import java.security.Principal; import java.text.MessageFormat; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.concurrent.TimeUnit; import javax.servlet.http.Cookie; @@ -455,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 @@ -474,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, @@ -520,21 +521,33 @@ public class AuthenticationManager implements IAuthenticationManager { protected UserModel authenticateLocal(UserModel user, char [] password) { UserModel returnedUser = null; - PasswordHash pwdHash = PasswordHash.instanceFor(user.password); - if (pwdHash != null) { - if (pwdHash.matches(user.password, password, user.username)) { + // Create a copy of the password that we can use to rehash to upgrade to a more secure hashing method. + // This is done to be independent from the implementation of the PasswordHash, which might already clear out + // the password it gets passed in. This looks a bit stupid, as we could simply clean up the mess, but this + // falls under "better safe than sorry". + char[] pwdToUpgrade = Arrays.copyOf(password, password.length); + try { + PasswordHash pwdHash = PasswordHash.instanceFor(user.password); + if (pwdHash != null) { + if (pwdHash.matches(user.password, password, user.username)) { + returnedUser = user; + } + } else if (user.password.equals(new String(password))) { + // plain-text password returnedUser = user; } - } else if (user.password.equals(new String(password))) { - // plain-text password - returnedUser = user; - } - - // validate user - returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); - - // try to upgrade the stored password hash to a stronger hash, if necessary - upgradeStoredPassword(returnedUser, password, pwdHash); + + // validate user + returnedUser = validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); + + // try to upgrade the stored password hash to a stronger hash, if necessary + upgradeStoredPassword(returnedUser, pwdToUpgrade, pwdHash); + } + finally { + // Now we make sure that the password is zeroed out in any case. + Arrays.fill(password, Character.MIN_VALUE); + Arrays.fill(pwdToUpgrade, Character.MIN_VALUE); + } return returnedUser; } 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 @@ -57,6 +57,21 @@ public class StringUtils { }
/**
+ * 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.
*
* @param string
|