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 #1335tags/r1.9.1^2
import java.nio.charset.Charset; | import java.nio.charset.Charset; | ||||
import java.security.Principal; | import java.security.Principal; | ||||
import java.text.MessageFormat; | 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 java.util.concurrent.TimeUnit; | ||||
import javax.servlet.http.Cookie; | import javax.servlet.http.Cookie; | ||||
protected UserModel authenticateLocal(UserModel user, char [] password) { | protected UserModel authenticateLocal(UserModel user, char [] password) { | ||||
UserModel returnedUser = null; | 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; | 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; | return returnedUser; | ||||
} | } |
public void testAuthenticateUpgradePlaintext() throws Exception { | public void testAuthenticateUpgradePlaintext() throws Exception { | ||||
IAuthenticationManager auth = newAuthenticationManager(); | IAuthenticationManager auth = newAuthenticationManager(); | ||||
String password = "topsecret"; | |||||
UserModel user = new UserModel("sunnyjim"); | UserModel user = new UserModel("sunnyjim"); | ||||
user.password = "password"; | |||||
user.password = password; | |||||
users.updateUserModel(user); | 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 | // validate that plaintext password was automatically updated to hashed one | ||||
assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); | 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)); | |||||
} | } | ||||
public void testAuthenticateUpgradeMD5() throws Exception { | public void testAuthenticateUpgradeMD5() throws Exception { | ||||
IAuthenticationManager auth = newAuthenticationManager(); | IAuthenticationManager auth = newAuthenticationManager(); | ||||
String password = "secretAndHashed"; | |||||
UserModel user = new UserModel("sunnyjim"); | UserModel user = new UserModel("sunnyjim"); | ||||
user.password = "MD5:5F4DCC3B5AA765D61D8327DEB882CF99"; | |||||
user.password = "MD5:BD95A1CFD00868B59B3564112D1E5847"; | |||||
users.updateUserModel(user); | 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 | // validate that MD5 password was automatically updated to hashed one | ||||
assertTrue(user.password.startsWith(PasswordHash.getDefaultType().name() + ":")); | 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)); | |||||
} | } | ||||