Skip to content

Commit 803d417

Browse files
committed
Delete password from memory in AuthenticationManager
Zero out the password to remove it from memory after use. This is only a first step, implementing it for one method: `AuthenticationManager.authenticate(String, char[], String)`.
1 parent e47647b commit 803d417

File tree

4 files changed

+125
-31
lines changed

4 files changed

+125
-31
lines changed

src/main/java/com/gitblit/manager/AuthenticationManager.java

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,6 @@ protected void flagRequest(HttpServletRequest httpRequest, AuthenticationType au
452452
/**
453453
* Authenticate a user based on a username and password.
454454
*
455-
* @see IUserService.authenticate(String, char[])
456455
* @param username
457456
* @param password
458457
* @return a user object or null
@@ -471,34 +470,39 @@ public UserModel authenticate(String username, char[] password, String remoteIP)
471470
}
472471

473472
String usernameDecoded = StringUtils.decodeUsername(username);
474-
String pw = new String(password);
475-
if (StringUtils.isEmpty(pw)) {
473+
if (StringUtils.isEmpty(password)) {
476474
// can not authenticate empty password
477475
return null;
478476
}
479477

480478
UserModel user = userManager.getUserModel(usernameDecoded);
481479

482-
// try local authentication
483-
if (user != null && user.isLocalAccount()) {
484-
UserModel returnedUser = authenticateLocal(user, password);
485-
if (returnedUser != null) {
486-
// user authenticated
487-
return returnedUser;
488-
}
489-
} else {
490-
// try registered external authentication providers
491-
for (AuthenticationProvider provider : authenticationProviders) {
492-
if (provider instanceof UsernamePasswordAuthenticationProvider) {
493-
UserModel returnedUser = provider.authenticate(usernameDecoded, password);
494-
if (returnedUser != null) {
495-
// user authenticated
496-
returnedUser.accountType = provider.getAccountType();
497-
return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
480+
try {
481+
// try local authentication
482+
if (user != null && user.isLocalAccount()) {
483+
UserModel returnedUser = authenticateLocal(user, password);
484+
if (returnedUser != null) {
485+
// user authenticated
486+
return returnedUser;
487+
}
488+
} else {
489+
// try registered external authentication providers
490+
for (AuthenticationProvider provider : authenticationProviders) {
491+
if (provider instanceof UsernamePasswordAuthenticationProvider) {
492+
UserModel returnedUser = provider.authenticate(usernameDecoded, password);
493+
if (returnedUser != null) {
494+
// user authenticated
495+
returnedUser.accountType = provider.getAccountType();
496+
return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
497+
}
498498
}
499499
}
500500
}
501501
}
502+
finally {
503+
// Zero out password array to delete password from memory
504+
Arrays.fill(password, Character.MIN_VALUE);
505+
}
502506

503507
// could not authenticate locally or with a provider
504508
logger.warn(MessageFormat.format("Failed login attempt for {0}, invalid credentials from {1}", username,

src/main/java/com/gitblit/utils/StringUtils.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,21 @@ public static boolean isEmpty(String value) {
5656
return value == null || value.trim().length() == 0;
5757
}
5858

59+
/**
60+
* Returns true if the character array represents an empty String.
61+
* An empty character sequence is defined as a sequence that
62+
* either has no characters at all, or no characters above
63+
* '\u0020' (space).
64+
*
65+
* @param value
66+
* @return true if value is null or represents an empty String
67+
*/
68+
public static boolean isEmpty(char[] value) {
69+
if (value == null || value.length == 0) return true;
70+
for ( char c : value) if (c > '\u0020') return false;
71+
return true;
72+
}
73+
5974
/**
6075
* Replaces carriage returns and line feeds with html line breaks.
6176
*

src/test/java/com/gitblit/tests/AuthenticationManagerTest.java

Lines changed: 73 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,7 @@
1919
import java.io.IOException;
2020
import java.io.UnsupportedEncodingException;
2121
import java.security.Principal;
22-
import java.util.Collection;
23-
import java.util.Collections;
24-
import java.util.Enumeration;
25-
import java.util.HashMap;
26-
import java.util.List;
27-
import java.util.Locale;
28-
import java.util.Map;
22+
import java.util.*;
2923

3024
import javax.servlet.AsyncContext;
3125
import javax.servlet.DispatcherType;
@@ -654,16 +648,84 @@ public boolean deleteRepositoryRole(String role) {
654648
public void testAuthenticate() throws Exception {
655649
IAuthenticationManager auth = newAuthenticationManager();
656650

651+
652+
String password = "pass word";
657653
UserModel user = new UserModel("sunnyjim");
658-
user.password = "password";
654+
user.password = password;
659655
users.updateUserModel(user);
660656

661-
assertNotNull(auth.authenticate(user.username, user.password.toCharArray(), null));
657+
char[] pwd = password.toCharArray();
658+
assertNotNull(auth.authenticate(user.username, pwd, null));
659+
660+
// validate that the passed in password has been zeroed out in memory
661+
char[] zeroes = new char[pwd.length];
662+
Arrays.fill(zeroes, Character.MIN_VALUE);
663+
assertArrayEquals(zeroes, pwd);
664+
}
665+
666+
667+
@Test
668+
public void testAuthenticateDisabledUser() throws Exception {
669+
IAuthenticationManager auth = newAuthenticationManager();
670+
671+
672+
String password = "password";
673+
UserModel user = new UserModel("sunnyjim");
674+
user.password = password;
662675
user.disabled = true;
676+
users.updateUserModel(user);
677+
678+
assertNull(auth.authenticate(user.username, password.toCharArray(), null));
679+
680+
user.disabled = false;
681+
users.updateUserModel(user);
682+
assertNotNull(auth.authenticate(user.username, password.toCharArray(), null));
683+
}
684+
685+
686+
@Test
687+
public void testAuthenticateEmptyPassword() throws Exception {
688+
IAuthenticationManager auth = newAuthenticationManager();
689+
690+
691+
String password = "password";
692+
UserModel user = new UserModel("sunnyjim");
693+
user.password = password;
694+
users.updateUserModel(user);
695+
696+
assertNull(auth.authenticate(user.username, "".toCharArray(), null));
697+
assertNull(auth.authenticate(user.username, " ".toCharArray(), null));
698+
assertNull(auth.authenticate(user.username, new char[]{' ', '\u0010', '\u0015'}, null));
699+
}
700+
701+
702+
663703

704+
@Test
705+
public void testAuthenticateWrongPassword() throws Exception {
706+
IAuthenticationManager auth = newAuthenticationManager();
707+
708+
709+
String password = "password";
710+
UserModel user = new UserModel("sunnyjim");
711+
user.password = password;
664712
users.updateUserModel(user);
665-
assertNull(auth.authenticate(user.username, user.password.toCharArray(), null));
666-
users.deleteUserModel(user);
713+
714+
assertNull(auth.authenticate(user.username, "helloworld".toCharArray(), null));
715+
}
716+
717+
718+
@Test
719+
public void testAuthenticateNoSuchUser() throws Exception {
720+
IAuthenticationManager auth = newAuthenticationManager();
721+
722+
723+
String password = "password";
724+
UserModel user = new UserModel("sunnyjim");
725+
user.password = password;
726+
users.updateUserModel(user);
727+
728+
assertNull(auth.authenticate("rainyjoe", password.toCharArray(), null));
667729
}
668730

669731

src/test/java/com/gitblit/tests/StringUtilsTest.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,25 @@ public class StringUtilsTest extends GitblitUnitTest {
2626

2727
@Test
2828
public void testIsEmpty() throws Exception {
29-
assertTrue(StringUtils.isEmpty(null));
29+
assertTrue(StringUtils.isEmpty((String)null));
3030
assertTrue(StringUtils.isEmpty(""));
3131
assertTrue(StringUtils.isEmpty(" "));
3232
assertFalse(StringUtils.isEmpty("A"));
3333
}
3434

35+
@Test
36+
public void testIsEmptyCharArray() throws Exception {
37+
assertTrue(StringUtils.isEmpty((char[])null));
38+
assertTrue(StringUtils.isEmpty(new char[0]));
39+
assertTrue(StringUtils.isEmpty(new char[]{ ' ' }));
40+
assertTrue(StringUtils.isEmpty(new char[]{ ' '}));
41+
assertTrue(StringUtils.isEmpty(new char[]{ ' ', ' ' }));
42+
assertTrue(StringUtils.isEmpty(new char[]{ ' ', ' ', ' ' }));
43+
assertFalse(StringUtils.isEmpty(new char[]{ '\u0020', 'f' }));
44+
assertFalse(StringUtils.isEmpty(new char[]{ '\u0148', '\u0020' }));
45+
assertFalse(StringUtils.isEmpty(new char[]{ 'A' }));
46+
}
47+
3548
@Test
3649
public void testBreakLinesForHtml() throws Exception {
3750
String input = "this\nis\r\na\rtest\r\n\r\nof\n\nline\r\rbreaking";

0 commit comments

Comments
 (0)