SEC-694: Add check to LdapShaPasswordEncoder to detect use with non-SHA passwords

http://jira.springframework.org/browse/SEC-694

core/src/main/java/org/springframework/security/providers/ldap/authenticator/LdapShaPasswordEncoder.java

@@ -15,8 +15,6 @@
 
 package org.springframework.security.providers.ldap.authenticator;
 
-import org.springframework.security.ldap.LdapDataAccessException;
-
 import org.springframework.security.providers.encoding.PasswordEncoder;
 import org.springframework.security.providers.encoding.ShaPasswordEncoder;
 
@@ -85,7 +83,7 @@ public class LdapShaPasswordEncoder implements PasswordEncoder {
         try {
             sha = MessageDigest.getInstance("SHA");
         } catch (java.security.NoSuchAlgorithmException e) {
-            throw new LdapDataAccessException("No SHA implementation available!", e);
+            throw new IllegalStateException("No SHA implementation available!", e);
         }
 
         sha.update(rawPass.getBytes());
@@ -129,23 +127,44 @@ public class LdapShaPasswordEncoder implements PasswordEncoder {
      *
      * @return true if they match (independent of the case of the prefix).
      */
-    public boolean isPasswordValid(String encPass, String rawPass, Object salt) {
+    public boolean isPasswordValid(final String encPass, final String rawPass, Object salt) {
-        String encPassWithoutPrefix;
+        String prefix = extractPrefix(encPass);
         
-        if (!encPass.startsWith("{")) {
+        if (prefix == null) {
             return encPass.equals(rawPass);
         }
 
-        if (encPass.startsWith(SSHA_PREFIX) || encPass.startsWith(SSHA_PREFIX_LC)) {
+        if (prefix.equals(SSHA_PREFIX) || prefix.equals(SSHA_PREFIX_LC)) {
-            encPassWithoutPrefix = encPass.substring(6);
             salt = extractSalt(encPass);
+        } else if (!prefix.equals(SHA_PREFIX) && !prefix.equals(SHA_PREFIX_LC)) {
+            throw new IllegalArgumentException("Unsupported password prefix '" + prefix + "'");
         } else {
-            encPassWithoutPrefix = encPass.substring(5);
+        	// Standard SHA
-            salt = null;
+        	salt = null;
         }
 
-        // Compare the encoded passwords without the prefix
+        int startOfHash = prefix.length() + 1;
-        return encodePassword(rawPass, salt).endsWith(encPassWithoutPrefix);
+        
+        String encodedRawPass = encodePassword(rawPass, salt).substring(startOfHash);
+        
+        return encodedRawPass.equals(encPass.substring(startOfHash));
+    }
+    
+    /**
+     * Returns the hash prefix or null if there isn't one. 
+     */
+    private String extractPrefix(String encPass) {
+        if (!encPass.startsWith("{")) {
+        	return null;
+        }
+
+		int secondBrace = encPass.lastIndexOf('}');
+		
+		if (secondBrace < 0) {
+			throw new IllegalArgumentException("Couldn't find closing brace for SHA prefix");
+		}
+		
+		return encPass.substring(0, secondBrace + 1);
     }
 
     public void setForceLowerCasePrefix(boolean forceLowerCasePrefix) {

core/src/main/java/org/springframework/security/providers/ldap/authenticator/PasswordComparisonAuthenticator.java

@@ -35,17 +35,11 @@ import java.util.Iterator;
 
 /**
  * An {@link org.springframework.security.providers.ldap.LdapAuthenticator LdapAuthenticator} which compares the login
- * password with the value stored in the directory using an LDAP "compare" operation.
+ * password with the value stored in the directory using a remote LDAP "compare" operation.
  *
  * <p>
- * This can be achieved either by retrieving the password attribute for the user and comparing it locally,
- * or by peforming an LDAP "compare" operation. If the password attribute (default "userPassword") is found in the
- * retrieved attributes it will be compared locally. If not, the remote comparison will be attempted.
- * </p>
- * <p>
  * If passwords are stored in digest form in the repository, then a suitable {@link PasswordEncoder}
  * implementation must be supplied. By default, passwords are encoded using the {@link LdapShaPasswordEncoder}.
- * </p>
  *
  * @author Luke Taylor
  * @version $Id$

core/src/test/java/org/springframework/security/providers/ldap/authenticator/LdapShaPasswordEncoderTests.java

@@ -15,7 +15,10 @@
 
 package org.springframework.security.providers.ldap.authenticator;
 
-import junit.framework.TestCase;
+import static org.junit.Assert.*;
+
+import org.junit.Before;
+import org.junit.Test;
 
 
 /**
@@ -24,37 +27,39 @@ import junit.framework.TestCase;
  * @author Luke Taylor
  * @version $Id$
  */
-public class LdapShaPasswordEncoderTests extends TestCase {
+public class LdapShaPasswordEncoderTests {
     //~ Instance fields ================================================================================================
 
     LdapShaPasswordEncoder sha;
 
     //~ Methods ========================================================================================================
 
-    protected void setUp() throws Exception {
+    @Before
-        super.setUp();
+    public void setUp() throws Exception {
         sha = new LdapShaPasswordEncoder();
     }
 
-    public void testInvalidPasswordFails() {
+    @Test
+    public void invalidPasswordFails() {
         assertFalse(sha.isPasswordValid("{SHA}ddSFGmjXYPbZC+NXR2kCzBRjqiE=", "wrongpassword", null));
     }
 
-    public void testInvalidSaltedPasswordFails() {
+    @Test    
+    public void invalidSaltedPasswordFails() {
         assertFalse(sha.isPasswordValid("{SSHA}25ro4PKC8jhQZ26jVsozhX/xaP0suHgX", "wrongpassword", null));
         assertFalse(sha.isPasswordValid("{SSHA}PQy2j+6n5ytA+YlAKkM8Fh4p6u2JxfVd", "wrongpassword", null));
     }
 
-    public void testNonByteArraySaltThrowsException() {
+    @Test(expected=IllegalArgumentException.class)
-        try {
+    public void nonByteArraySaltThrowsException() {
-            sha.encodePassword("password", "AStringNotAByteArray");
+        sha.encodePassword("password", "AStringNotAByteArray");
-        } catch (IllegalArgumentException expected) {}
     }
 
     /**
      * Test values generated by 'slappasswd -h {SHA} -s boabspasswurd'
      */
-    public void testValidPasswordSucceeds() {
+    @Test
+    public void validPasswordSucceeds() {
         sha.setForceLowerCasePrefix(false);
         assertTrue(sha.isPasswordValid("{SHA}ddSFGmjXYPbZC+NXR2kCzBRjqiE=", "boabspasswurd", null));
         assertTrue(sha.isPasswordValid("{sha}ddSFGmjXYPbZC+NXR2kCzBRjqiE=", "boabspasswurd", null));
@@ -66,7 +71,8 @@ public class LdapShaPasswordEncoderTests extends TestCase {
     /**
      * Test values generated by 'slappasswd -s boabspasswurd'
      */
-    public void testValidSaltedPasswordSucceeds() {
+    @Test
+    public void validSaltedPasswordSucceeds() {
         sha.setForceLowerCasePrefix(false);
         assertTrue(sha.isPasswordValid("{SSHA}25ro4PKC8jhQZ26jVsozhX/xaP0suHgX", "boabspasswurd", null));
         assertTrue(sha.isPasswordValid("{ssha}PQy2j+6n5ytA+YlAKkM8Fh4p6u2JxfVd", "boabspasswurd", null));
@@ -75,7 +81,8 @@ public class LdapShaPasswordEncoderTests extends TestCase {
         assertTrue(sha.isPasswordValid("{ssha}PQy2j+6n5ytA+YlAKkM8Fh4p6u2JxfVd", "boabspasswurd", null));
     }
 
-    public void testCorrectPrefixCaseIsUsed() {
+    @Test
+    public void correctPrefixCaseIsUsed() {
         sha.setForceLowerCasePrefix(false);
         assertEquals("{SHA}ddSFGmjXYPbZC+NXR2kCzBRjqiE=", sha.encodePassword("boabspasswurd", null));
         assertTrue(sha.encodePassword("somepassword", "salt".getBytes()).startsWith("{SSHA}"));
@@ -85,4 +92,15 @@ public class LdapShaPasswordEncoderTests extends TestCase {
         assertTrue(sha.encodePassword("somepassword", "salt".getBytes()).startsWith("{ssha}"));
 
     }
+
+    @Test(expected=IllegalArgumentException.class)
+    public void invalidPrefixIsRejected() {
+    	sha.isPasswordValid("{MD9}xxxxxxxxxx" , "somepassword", null);
+    }
+    
+    @Test(expected=IllegalArgumentException.class)
+    public void malformedPrefixIsRejected() {
+    	// No right brace
+    	sha.isPasswordValid("{SSHA25ro4PKC8jhQZ26jVsozhX/xaP0suHgX" , "somepassword", null);
+    }
 }