2 files changed, 38 insertions, 21 deletions

diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java
index 83ca4b70..4f3f4f85 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;
@@ -520,21 +517,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/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));
 	}