Permalink
Browse files

SEC-2056: DaoAuthenticationProvider performs isPasswordValid when use…

…r not found

Previously authenticating a user could take significantly longer than
determining that a user does not exist. This was due to the fact that only
users that were found would use the password encoder and comparing a
password can take a significant amount of time. The difference in the
time required could allow a side channel attack that reveals if a user
exists.

The code has been updated to do comparison against a dummy password
even when the the user was not found.
  • Loading branch information...
1 parent f3b143f commit c076f0f2e190c73a17379d05935c2c81657adee9 @rwinch rwinch committed Sep 21, 2012
@@ -31,19 +31,39 @@
* An {@link AuthenticationProvider} implementation that retrieves user details from a {@link UserDetailsService}.
*
* @author Ben Alex
+ * @author Rob Winch
*/
public class DaoAuthenticationProvider extends AbstractUserDetailsAuthenticationProvider {
+ //~ Static fields/initializers =====================================================================================
+
+ /**
+ * The plaintext password used to perform {@link PasswordEncoder#isPasswordValid(String, String, Object)} on when the user is
+ * not found to avoid SEC-2056.
+ */
+ private static final String USER_NOT_FOUND_PASSWORD = "userNotFoundPassword";
//~ Instance fields ================================================================================================
- private PasswordEncoder passwordEncoder = new PlaintextPasswordEncoder();
+ private PasswordEncoder passwordEncoder;
+
+ /**
+ * The password used to perform {@link PasswordEncoder#isPasswordValid(String, String, Object)} on when the user is
+ * not found to avoid SEC-2056. This is necessary, because some {@link PasswordEncoder} implementations will short circuit if the
+ * password is not in a valid format.
+ */
+ private String userNotFoundEncodedPassword;
private SaltSource saltSource;
private UserDetailsService userDetailsService;
+ public DaoAuthenticationProvider() {
+ setPasswordEncoder(new PlaintextPasswordEncoder());
+ }
+
//~ Methods ========================================================================================================
+ @SuppressWarnings("deprecation")
protected void additionalAuthenticationChecks(UserDetails userDetails,
UsernamePasswordAuthenticationToken authentication) throws AuthenticationException {
Object salt = null;
@@ -80,6 +100,10 @@ protected final UserDetails retrieveUser(String username, UsernamePasswordAuthen
try {
loadedUser = this.getUserDetailsService().loadUserByUsername(username);
} catch (UsernameNotFoundException notFound) {
+ if(authentication.getCredentials() != null) {
+ String presentedPassword = authentication.getCredentials().toString();
+ passwordEncoder.isPasswordValid(userNotFoundEncodedPassword, presentedPassword, null);
+ }
throw notFound;
} catch (Exception repositoryProblem) {
throw new AuthenticationServiceException(repositoryProblem.getMessage(), repositoryProblem);
@@ -106,14 +130,14 @@ public void setPasswordEncoder(Object passwordEncoder) {
Assert.notNull(passwordEncoder, "passwordEncoder cannot be null");
if (passwordEncoder instanceof PasswordEncoder) {
- this.passwordEncoder = (PasswordEncoder) passwordEncoder;
+ setPasswordEncoder((PasswordEncoder) passwordEncoder);
return;
}
if (passwordEncoder instanceof org.springframework.security.crypto.password.PasswordEncoder) {
final org.springframework.security.crypto.password.PasswordEncoder delegate =
(org.springframework.security.crypto.password.PasswordEncoder)passwordEncoder;
- this.passwordEncoder = new PasswordEncoder() {
+ setPasswordEncoder(new PasswordEncoder() {
public String encodePassword(String rawPass, Object salt) {
checkSalt(salt);
return delegate.encode(rawPass);
@@ -127,14 +151,21 @@ public boolean isPasswordValid(String encPass, String rawPass, Object salt) {
private void checkSalt(Object salt) {
Assert.isNull(salt, "Salt value must be null when used with crypto module PasswordEncoder");
}
- };
+ });
return;
}
throw new IllegalArgumentException("passwordEncoder must be a PasswordEncoder instance");
}
+ private void setPasswordEncoder(PasswordEncoder passwordEncoder) {
+ Assert.notNull(passwordEncoder, "passwordEncoder cannot be null");
+
+ this.userNotFoundEncodedPassword = passwordEncoder.encodePassword(USER_NOT_FOUND_PASSWORD, null);
+ this.passwordEncoder = passwordEncoder;
+ }
+
protected PasswordEncoder getPasswordEncoder() {
return passwordEncoder;
}
@@ -15,6 +15,15 @@
package org.springframework.security.authentication.dao;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.isA;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.security.SecureRandom;
+import java.util.ArrayList;
import java.util.List;
import junit.framework.TestCase;
@@ -38,12 +47,15 @@
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.security.core.userdetails.cache.EhCacheBasedUserCache;
import org.springframework.security.core.userdetails.cache.NullUserCache;
+import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
+import org.springframework.security.crypto.password.PasswordEncoder;
/**
* Tests {@link DaoAuthenticationProvider}.
*
* @author Ben Alex
+ * @author Rob Winch
*/
public class DaoAuthenticationProviderTests extends TestCase {
private static final List<GrantedAuthority> ROLES_12 = AuthorityUtils.createAuthorityList("ROLE_ONE", "ROLE_TWO");
@@ -433,6 +445,113 @@ public void testSupports() {
assertTrue(!provider.supports(TestingAuthenticationToken.class));
}
+ // SEC-2056
+ public void testUserNotFoundEncodesPassword() {
+ UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("missing", "koala");
+ PasswordEncoder encoder = mock(PasswordEncoder.class);
+ when(encoder.encode(anyString())).thenReturn("koala");
+ DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
+ provider.setHideUserNotFoundExceptions(false);
+ provider.setPasswordEncoder(encoder);
+ provider.setUserDetailsService(new MockAuthenticationDaoUserrod());
+ try {
+ provider.authenticate(token);
+ fail("Expected Exception");
+ } catch(UsernameNotFoundException success) {}
+
+ // ensure encoder invoked w/ non-null strings since PasswordEncoder impls may fail if encoded password is null
+ verify(encoder).matches(isA(String.class), isA(String.class));
+ }
+
+ public void testUserNotFoundBCryptPasswordEncoder() {
+ UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("missing", "koala");
+ PasswordEncoder encoder = new BCryptPasswordEncoder();
+ DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
+ provider.setHideUserNotFoundExceptions(false);
+ provider.setPasswordEncoder(encoder);
+ MockAuthenticationDaoUserrod userDetailsService = new MockAuthenticationDaoUserrod();
+ userDetailsService.password = encoder.encode((CharSequence) token.getCredentials());
+ provider.setUserDetailsService(userDetailsService);
+ try {
+ provider.authenticate(token);
+ fail("Expected Exception");
+ } catch(UsernameNotFoundException success) {}
+ }
+
+ public void testUserNotFoundDefaultEncoder() {
+ UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("missing", null);
+ DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
+ provider.setHideUserNotFoundExceptions(false);
+ provider.setUserDetailsService(new MockAuthenticationDaoUserrod());
+ try {
+ provider.authenticate(token);
+ fail("Expected Exception");
+ } catch(UsernameNotFoundException success) {}
+ }
+
+ /**
+ * This is an explicit test for SEC-2056. It is intentionally ignored since this test is not
+ * deterministic and {@link #testUserNotFoundEncodesPassword()} ensures that SEC-2056 is fixed.
+ */
+ public void IGNOREtestSec2056() {
+ UsernamePasswordAuthenticationToken foundUser = new UsernamePasswordAuthenticationToken("rod", "koala");
+ UsernamePasswordAuthenticationToken notFoundUser = new UsernamePasswordAuthenticationToken("notFound", "koala");
+ PasswordEncoder encoder = new BCryptPasswordEncoder(10,new SecureRandom());
+ DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
+ provider.setHideUserNotFoundExceptions(false);
+ provider.setPasswordEncoder(encoder);
+ MockAuthenticationDaoUserrod userDetailsService = new MockAuthenticationDaoUserrod();
+ userDetailsService.password = encoder.encode((CharSequence) foundUser.getCredentials());
+ provider.setUserDetailsService(userDetailsService);
+
+ int sampleSize = 100;
+
+ List<Long> userFoundTimes = new ArrayList<Long>(sampleSize);
+ for(int i=0;i<sampleSize;i++) {
+ long start = System.currentTimeMillis();
+ provider.authenticate(foundUser);
+ userFoundTimes.add(System.currentTimeMillis() - start);
+ }
+
+ List<Long> userNotFoundTimes = new ArrayList<Long>(sampleSize);
+ for(int i=0;i<sampleSize;i++) {
+ long start = System.currentTimeMillis();
+ try {
+ provider.authenticate(notFoundUser);
+ fail("Expected Exception");
+ } catch(UsernameNotFoundException success) {}
+ userNotFoundTimes.add(System.currentTimeMillis() - start);
+ }
+
+ double userFoundAvg = avg(userFoundTimes);
+ double userNotFoundAvg = avg(userNotFoundTimes);
+ assertTrue("User not found average " + userNotFoundAvg + " should be within 3ms of user found average " + userFoundAvg,
+ Math.abs(userNotFoundAvg - userFoundAvg) <= 3);
+ }
+
+ private double avg(List<Long> counts) {
+ long sum = 0;
+ for(Long time : counts) {
+ sum += time;
+ }
+ return sum / counts.size();
+ }
+
+ public void testUserNotFoundNullCredentials() {
+ UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken("missing", null);
+ PasswordEncoder encoder = mock(PasswordEncoder.class);
+ DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
+ provider.setHideUserNotFoundExceptions(false);
+ provider.setPasswordEncoder(encoder);
+ provider.setUserDetailsService(new MockAuthenticationDaoUserrod());
+ try {
+ provider.authenticate(token);
+ fail("Expected Exception");
+ } catch(UsernameNotFoundException success) {}
+
+ verify(encoder,times(0)).matches(anyString(), anyString());
+ }
+
//~ Inner Classes ==================================================================================================
private class MockAuthenticationDaoReturnsNull implements UserDetailsService {

0 comments on commit c076f0f

Please sign in to comment.