Return-Path: X-Original-To: apmail-directory-dev-archive@www.apache.org Delivered-To: apmail-directory-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 3C65C178FC for ; Sat, 25 Jul 2015 13:26:03 +0000 (UTC) Received: (qmail 82086 invoked by uid 500); 25 Jul 2015 13:26:03 -0000 Delivered-To: apmail-directory-dev-archive@directory.apache.org Received: (qmail 82019 invoked by uid 500); 25 Jul 2015 13:26:03 -0000 Mailing-List: contact dev-help@directory.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: "Apache Directory Developers List" Delivered-To: mailing list dev@directory.apache.org Received: (qmail 82009 invoked by uid 99); 25 Jul 2015 13:26:02 -0000 Received: from mail-relay.apache.org (HELO mail-relay.apache.org) (140.211.11.15) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 25 Jul 2015 13:26:02 +0000 Received: from mail-ig0-f169.google.com (mail-ig0-f169.google.com [209.85.213.169]) by mail-relay.apache.org (ASF Mail Server at mail-relay.apache.org) with ESMTPSA id 5E1771A032F for ; Sat, 25 Jul 2015 13:26:02 +0000 (UTC) Received: by igk11 with SMTP id 11so22712769igk.1 for ; Sat, 25 Jul 2015 06:26:01 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.107.10.17 with SMTP id u17mr30558500ioi.16.1437830761280; Sat, 25 Jul 2015 06:26:01 -0700 (PDT) Received: by 10.36.31.204 with HTTP; Sat, 25 Jul 2015 06:26:01 -0700 (PDT) In-Reply-To: <20150724181139.12611AC0623@hades.apache.org> References: <20150724181139.12611AC0623@hades.apache.org> Date: Sat, 25 Jul 2015 21:26:01 +0800 Message-ID: Subject: 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 From: Kiran Ayyagari To: Apache Directory Developers List Content-Type: multipart/alternative; boundary=001a113ee77ec5fdb2051bb30fd4 --001a113ee77ec5fdb2051bb30fd4 Content-Type: text/plain; charset=UTF-8 Lucas, On Sat, Jul 25, 2015 at 2:11 AM, 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 --001a113ee77ec5fdb2051bb30fd4 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

Lucas,

On Sat, Jul 25, 2015 at 2:11= AM, <lucastheisen@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:
=C2=A0 =C2=A0 directory/apacheds/trunk/interceptors/authn/src/main/java/org= /apache/directory/server/core/authn/SimpleAuthenticator.java
=C2=A0 =C2=A0 directory/apacheds/trunk/server-integ/src/test/java/org/apach= e/directory/server/ppolicy/PasswordPolicyIT.java

Modified: directory/apacheds/trunk/interceptors/authn/src/main/java/org/apa= che/directory/server/core/authn/SimpleAuthenticator.java
URL: http://svn.apache.org/viewvc/= directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/direct= ory/server/core/authn/SimpleAuthenticator.java?rev=3D1692562&r1=3D16925= 61&r2=3D1692562&view=3Ddiff
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D
--- directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/di= rectory/server/core/authn/SimpleAuthenticator.java (original)
+++ directory/apacheds/trunk/interceptors/authn/src/main/java/org/apache/di= rectory/server/core/authn/SimpleAuthenticator.java Fri Jul 24 18:11:38 2015=
@@ -25,8 +25,10 @@ import java.security.MessageDigest;
=C2=A0import java.security.NoSuchAlgorithmException;
=C2=A0import java.util.Arrays;

+
=C2=A0import javax.naming.Context;

+
=C2=A0import org.apache.commons.collections.map.LRUMap;
=C2=A0import org.apache.directory.api.ldap.model.constants.AuthenticationLe= vel;
=C2=A0import org.apache.directory.api.ldap.model.constants.LdapSecurityCons= tants;
@@ -45,6 +47,7 @@ import org.apache.directory.server.core.
=C2=A0import org.apache.directory.server.core.api.InterceptorEnum;
=C2=A0import org.apache.directory.server.core.api.LdapPrincipal;
=C2=A0import org.apache.directory.server.core.api.authn.ppolicy.PasswordPol= icyConfiguration;
+import org.apache.directory.server.core.api.authn.ppolicy.PasswordPolicyEx= ception;
=C2=A0import org.apache.directory.server.core.api.entry.ClonedServerEntry;<= br> =C2=A0import org.apache.directory.server.core.api.interceptor.context.BindO= perationContext;
=C2=A0import org.apache.directory.server.core.api.interceptor.context.Looku= pOperationContext;
@@ -222,11 +225,27 @@ public class SimpleAuthenticator extends
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// Get the stored password, either from c= ache or from backend
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0byte[][] storedPasswords =3D principal.ge= tUserPasswords();

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 PasswordPolicyException ppe =3D null;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 checkPwdPolicy( bindContext.getE= ntry() );
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 catch ( PasswordPolicyException e )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ppe =3D e;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+
the information that checkPwdPolicy() throws is anyw= ay 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 wh= y you wait till comparing
the credentials to throw the except= ion.

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// Now, compare the passwords.
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for ( byte[] storedPassword : storedPassw= ords )
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ( PasswordUtil.compareCr= edentials( credentials, storedPassword ) )
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( ppe !=3D null= )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOG.= debug( "{} Authentication failed: {}", bindContext.getDn(), ppe.g= etMessage() );
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 thro= w ppe;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ( IS_DEBUG= )
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0LOG.debug( "{} Authenticated", bindContext.getDn() );
@@ -285,7 +304,7 @@ public class SimpleAuthenticator extends
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0throw e;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 checkPwdPolicy( userEntry );
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 //checkPwdPolicy( userEntry );

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DirectoryService directoryService =3D get= DirectoryService();
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0String userPasswordAttribute =3D SchemaCo= nstants.USER_PASSWORD_AT;

Modified: directory/apacheds/trunk/server-integ/src/test/java/org/apache/di= rectory/server/ppolicy/PasswordPolicyIT.java
URL: http://svn.apache.org/viewvc/directory/ap= acheds/trunk/server-integ/src/test/java/org/apache/directory/server/ppolicy= /PasswordPolicyIT.java?rev=3D1692562&r1=3D1692561&r2=3D1692562&= view=3Ddiff
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D
--- directory/apacheds/trunk/server-integ/src/test/java/org/apache/director= y/server/ppolicy/PasswordPolicyIT.java (original)
+++ directory/apacheds/trunk/server-integ/src/test/java/org/apache/director= y/server/ppolicy/PasswordPolicyIT.java Fri Jul 24 18:11:38 2015
@@ -1131,7 +1131,7 @@ public class PasswordPolicyIT extends Ab
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0checkBind( userConnection, userDn, "= badPassword", 3,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"INVALID_CREDENTIALS: = Bind failed: ERR_229 Cannot authenticate user cn=3DuserLockout,ou=3Dsystem&= quot; );

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 checkBind( userConnection, userDn, "badPa= ssword", 1,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 checkBind( userConnection, userDn, "12345= ", 1,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"INVALID_CREDENTIALS: = Bind failed: account was permanently locked" );

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0userConnection.close();





--
Kiran Ayyagari
h= ttp://keydap.com
--001a113ee77ec5fdb2051bb30fd4--