summaryrefslogtreecommitdiffstats
path: root/src/main
diff options
context:
space:
mode:
authorFlorian Zschocke <f.zschocke+git@gmail.com>2020-04-04 19:25:27 +0200
committerFlorian Zschocke <f.zschocke+git@gmail.com>2020-04-05 12:34:34 +0200
commite47647b00d566d64d311042981e6b1798f683e4a (patch)
tree1a3c74a343cbed999a37d8d9161e0f9943d97272 /src/main
parent8b18ac309bc36c8a16a3d26f088cb168635930d3 (diff)
downloadgitblit-e47647b00d566d64d311042981e6b1798f683e4a.tar.gz
gitblit-e47647b00d566d64d311042981e6b1798f683e4a.zip
🦟 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
Diffstat (limited to 'src/main')
-rw-r--r--src/main/java/com/gitblit/manager/AuthenticationManager.java43
1 files changed, 26 insertions, 17 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;
}