On Sat, Jul 25, 2015 at 9:54 PM, Lucas Theisen <[email protected]> wrote:
> > On Jul 25, 2015 9:26 AM, "Kiran Ayyagari" <[email protected]> wrote: > > > > > > Lucas, > > > > On Sat, Jul 25, 2015 at 2:11 AM, <[email protected]> wrote: > >> > >> Author: lucastheisen > >> Date: Fri Jul 24 18:11:38 2015 > >> New Revision: 1692562 > >> > >> URL: http://svn.apache.org/r1692562 > >> > >> Log: > >> fixed leak of information when password policy fails authentication > attempt > >> > >> Modified: > >> > directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java > >> > directory/apacheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java > >> > >> Modified: > directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java > >> URL: > http://svn.apache.org/viewvc/directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java?rev=1692562&r1=1692561&r2=1692562&view=diff > >> > >> > ============================================================================== > >> --- > directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java > (original) > >> +++ > directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java > Fri Jul 24 18:11:38 2015 > >> @@ -25,8 +25,10 @@ import java.security.MessageDigest; > >> import java.security.NoSuchAlgorithmException; > >> import java.util.Arrays; > >> > >> + > >> import javax.naming.Context; > >> > >> + > >> import org.apache.commons.collections.map.LRUMap; > >> import > org.apache.directory.api.ldap.model.constants.AuthenticationLevel; > >> import > org.apache.directory.api.ldap.model.constants.LdapSecurityConstants; > >> @@ -45,6 +47,7 @@ import org.apache.directory.server.core. > >> import org.apache.directory.server.core.api.InterceptorEnum; > >> import org.apache.directory.server.core.api.LdapPrincipal; > >> import > org.apache.directory.server.core.api.authn.ppolicy.PasswordPolicyConfiguration; > >> +import > org.apache.directory.server.core.api.authn.ppolicy.PasswordPolicyException; > >> import org.apache.directory.server.core.api.entry.ClonedServerEntry; > >> import > org.apache.directory.server.core.api.interceptor.context.BindOperationContext; > >> import > org.apache.directory.server.core.api.interceptor.context.LookupOperationContext; > >> @@ -222,11 +225,27 @@ public class SimpleAuthenticator extends > >> // Get the stored password, either from cache or from backend > >> byte[][] storedPasswords = principal.getUserPasswords(); > >> > >> + PasswordPolicyException ppe = null; > >> + try > >> + { > >> + checkPwdPolicy( bindContext.getEntry() ); > >> + } > >> + catch ( PasswordPolicyException e ) > >> + { > >> + ppe = e; > >> + } > >> + > > > > the information that checkPwdPolicy() throws is anyway meant to send out > through the ppolicy response > > control, so there is nothing to prevent here from leaking and it is not > clear to me why you wait till comparing > > the credentials to throw the exception. > > > > It's not the information inside of the exception that is leaking. It's > the fact that a PasswordPolicyException is thrown rather than invalid > credentials. This would tell a potential attacker that scans through > possible names when they have found a valid one. They could then use that > information to cause account lockout, or other nefarious things. > there is nothing to hide here, cause the ppolicy response control is here to send out the status of the account, and this control is created based on the details present in this exception. Note that when a bind request comes with ppolicy request control the server must send out a response control with the details like time before expiration, remaining grace logins etc. > In general, I don't think we should provide any information other than > invalid credentials, until the user has successfully authenticated. > > >> // Now, compare the passwords. > >> for ( byte[] storedPassword : storedPasswords ) > >> { > >> if ( PasswordUtil.compareCredentials( credentials, > storedPassword ) ) > >> { > >> + if ( ppe != null ) > >> + { > >> + LOG.debug( "{} Authentication failed: {}", > bindContext.getDn(), ppe.getMessage() ); > >> + throw ppe; > >> + } > >> + > >> if ( IS_DEBUG ) > >> { > >> LOG.debug( "{} Authenticated", bindContext.getDn() > ); > >> @@ -285,7 +304,7 @@ public class SimpleAuthenticator extends > >> throw e; > >> } > >> > >> - checkPwdPolicy( userEntry ); > >> + //checkPwdPolicy( userEntry ); > >> > >> DirectoryService directoryService = getDirectoryService(); > >> String userPasswordAttribute = > SchemaConstants.USER_PASSWORD_AT; > >> > >> Modified: > directory/apacheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java > >> URL: > http://svn.apache.org/viewvc/directory/apacheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java?rev=1692562&r1=1692561&r2=1692562&view=diff > >> > >> > ============================================================================== > >> --- > directory/apacheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java > (original) > >> +++ > directory/apacheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java > Fri Jul 24 18:11:38 2015 > >> @@ -1131,7 +1131,7 @@ public class PasswordPolicyIT extends Ab > >> checkBind( userConnection, userDn, "badPassword", 3, > >> "INVALID_CREDENTIALS: Bind failed: ERR_229 Cannot > authenticate user cn=userLockout,ou=system" ); > >> > >> - checkBind( userConnection, userDn, "badPassword", 1, > >> + checkBind( userConnection, userDn, "12345", 1, > >> "INVALID_CREDENTIALS: Bind failed: account was permanently > locked" ); > >> > >> userConnection.close(); > >> > >> > > > > > > > > -- > > Kiran Ayyagari > > http://keydap.com > -- Kiran Ayyagari http://keydap.com
