🦟 fix: Password hash upgrade kills existing passwords

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

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));
 	}