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/manager/AuthenticationManager.java | 43 +++++++++++++--------- .../gitblit/tests/AuthenticationManagerTest.java | 16 ++++++-- 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)); } -- cgit v1.2.3