You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Kiran Ayyagari <ka...@apache.org> on 2015/07/25 15:26:01 UTC
Re: svn commit: r1692562 - in /directory/apacheds/trunk:
interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java
server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java
Lucas,
On Sat, Jul 25, 2015 at 2:11 AM, <lu...@apache.org> 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.
// 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
Re: svn commit: r1692562 - in /directory/apacheds/trunk:
interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java
server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java
Posted by Kiran Ayyagari <ka...@apache.org>.
On Sat, Jul 25, 2015 at 10:25 PM, Lucas Theisen <lu...@pastdev.com>
wrote:
>
> On Jul 25, 2015 10:01 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
> >
> >
> >
> > On Sat, Jul 25, 2015 at 9:54 PM, Lucas Theisen <lu...@pastdev.com>
> wrote:
> >>
> >>
> >> On Jul 25, 2015 9:26 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
> >> >
> >> >
> >> > Lucas,
> >> >
> >> > On Sat, Jul 25, 2015 at 2:11 AM, <lu...@apache.org> 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.
> >
>
> I understand that the exception would be needed to create a password
> policy control, but I do not think we should be providing that control back
> to an unauthenticated user. If you are not providing valid credentials,
> should we really provide you with information about the account?
>
> > 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.
>
> I follow, but that information only makes sense if the user provided the
> currently valid credentials to start with. If they don't know the
> password, why would they be provided with the time before password they
> clearly don't know expires?
>
a user/client will never know otherwise whether his login attempt failed
cause the password was
expired.
The servers that implement this ppolicy draft should also need to add
sufficient support to avoid
bruteforce attacks, so I believe that is how the original author made the
trade off.
>>
> >> 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
>
--
Kiran Ayyagari
http://keydap.com
Re: svn commit: r1692562 - in /directory/apacheds/trunk:
interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java
server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java
Posted by Lucas Theisen <lu...@pastdev.com>.
On Jul 25, 2015 10:01 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
>
>
>
> On Sat, Jul 25, 2015 at 9:54 PM, Lucas Theisen <lu...@pastdev.com>
wrote:
>>
>>
>> On Jul 25, 2015 9:26 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
>> >
>> >
>> > Lucas,
>> >
>> > On Sat, Jul 25, 2015 at 2:11 AM, <lu...@apache.org> 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.
>
I understand that the exception would be needed to create a password policy
control, but I do not think we should be providing that control back to an
unauthenticated user. If you are not providing valid credentials, should
we really provide you with information about the account?
> 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.
I follow, but that information only makes sense if the user provided the
currently valid credentials to start with. If they don't know the
password, why would they be provided with the time before password they
clearly don't know expires?
>>
>> 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
Re: svn commit: r1692562 - in /directory/apacheds/trunk:
interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java
server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java
Posted by Kiran Ayyagari <ka...@apache.org>.
On Sat, Jul 25, 2015 at 9:54 PM, Lucas Theisen <lu...@pastdev.com>
wrote:
>
> On Jul 25, 2015 9:26 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
> >
> >
> > Lucas,
> >
> > On Sat, Jul 25, 2015 at 2:11 AM, <lu...@apache.org> 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
Re: svn commit: r1692562 - in /directory/apacheds/trunk:
interceptors/authn/src/main/java/org/apache/directory/server/core/authn/SimpleAuthenticator.java
server-integ/src/test/java/org/apache/directory/server/ppolicy/PasswordPolicyIT.java
Posted by Lucas Theisen <lu...@pastdev.com>.
On Jul 25, 2015 9:26 AM, "Kiran Ayyagari" <ka...@apache.org> wrote:
>
>
> Lucas,
>
> On Sat, Jul 25, 2015 at 2:11 AM, <lu...@apache.org> 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.
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