diff --git a/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java b/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java index aa918a5b14..6753c6a3dd 100644 --- a/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java +++ b/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java @@ -31,19 +31,39 @@ import org.springframework.util.Assert; * 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 @@ public class DaoAuthenticationProvider extends AbstractUserDetailsAuthentication 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 class DaoAuthenticationProvider extends AbstractUserDetailsAuthentication 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,7 +151,7 @@ public class DaoAuthenticationProvider extends AbstractUserDetailsAuthentication private void checkSalt(Object salt) { Assert.isNull(salt, "Salt value must be null when used with crypto module PasswordEncoder"); } - }; + }); return; } @@ -135,6 +159,13 @@ public class DaoAuthenticationProvider extends AbstractUserDetailsAuthentication 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; } diff --git a/core/src/test/java/org/springframework/security/authentication/dao/DaoAuthenticationProviderTests.java b/core/src/test/java/org/springframework/security/authentication/dao/DaoAuthenticationProviderTests.java index a1181501ad..ee95703d77 100644 --- a/core/src/test/java/org/springframework/security/authentication/dao/DaoAuthenticationProviderTests.java +++ b/core/src/test/java/org/springframework/security/authentication/dao/DaoAuthenticationProviderTests.java @@ -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.UserDetailsService; 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 ROLES_12 = AuthorityUtils.createAuthorityList("ROLE_ONE", "ROLE_TWO"); @@ -433,6 +445,113 @@ public class DaoAuthenticationProviderTests extends TestCase { 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 userFoundTimes = new ArrayList(sampleSize); + for(int i=0;i userNotFoundTimes = new ArrayList(sampleSize); + for(int i=0;i 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 {