QPID-3158 - Defect in the CRAM-MD5-HEX mechanism - CRAMMD5HexInitialiser fails to pad bytes in range 0A-0F with leading zero. Add testcase to test CRAM-MD5-HEX mechanism. Guard against nulls in SASL SaslServerFactory.getMechanismNames implementations to avoid dependency on mechanism registration order.

Applied patch from  Keith Wall <keith.wall@gmail.com>

git-svn-id: https://svn.apache.org/repos/asf/qpid/branches/0.5.x-dev@1087250 13f79535-47bb-0310-9956-ffa450edef68

7 files changed, 387 insertions, 10 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/amqplain/AmqPlainSaslServerFactory.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/amqplain/AmqPlainSaslServerFactory.java
index 67d20136bf..17d123eb0d 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/amqplain/AmqPlainSaslServerFactory.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/amqplain/AmqPlainSaslServerFactory.java
@@ -45,9 +45,10 @@ public class AmqPlainSaslServerFactory implements SaslServerFactory
 
     public String[] getMechanismNames(Map props)
     {
-        if (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
-            props.containsKey(Sasl.POLICY_NODICTIONARY) ||
-            props.containsKey(Sasl.POLICY_NOACTIVE))
+        if (props != null &&
+            (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
+             props.containsKey(Sasl.POLICY_NODICTIONARY) ||
+             props.containsKey(Sasl.POLICY_NOACTIVE)))
         {
             // returned array must be non null according to interface documentation
             return new String[0];
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/crammd5/CRAMMD5HexInitialiser.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/crammd5/CRAMMD5HexInitialiser.java
index 8020d97364..8e47e45bfb 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/crammd5/CRAMMD5HexInitialiser.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/crammd5/CRAMMD5HexInitialiser.java
@@ -70,7 +70,7 @@ public class CRAMMD5HexInitialiser extends UsernamePasswordInitialiser
             for (char c : password)
             {
                 //toHexString does not prepend 0 so we have to
-                if (((byte) c > -1) && (byte) c < 10)
+                if (((byte) c > -1) && (byte) c < 0x10)
                 {
                     sb.append(0);
                 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/plain/PlainSaslServerFactory.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/plain/PlainSaslServerFactory.java
index f0dd9eeb6d..3144bfbce6 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/plain/PlainSaslServerFactory.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/plain/PlainSaslServerFactory.java
@@ -45,9 +45,10 @@ public class PlainSaslServerFactory implements SaslServerFactory
 
     public String[] getMechanismNames(Map props)
     {
-        if (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
-            props.containsKey(Sasl.POLICY_NODICTIONARY) ||
-            props.containsKey(Sasl.POLICY_NOACTIVE))
+        if (props != null &&
+            (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
+             props.containsKey(Sasl.POLICY_NODICTIONARY) ||
+             props.containsKey(Sasl.POLICY_NOACTIVE)))
         {
             // returned array must be non null according to interface documentation
             return new String[0];
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/sasl/CRAMMD5HexServerTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/sasl/CRAMMD5HexServerTest.java
new file mode 100644
index 0000000000..8b3f9c0622
--- /dev/null
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/sasl/CRAMMD5HexServerTest.java
@@ -0,0 +1,230 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.qpid.server.security.auth.sasl;
+
+import java.io.File;
+import java.io.IOException;
+import java.security.MessageDigest;
+import java.security.Principal;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.security.auth.login.AccountNotFoundException;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import junit.framework.TestCase;
+
+import org.apache.commons.codec.binary.Hex;
+import org.apache.qpid.server.security.auth.database.Base64MD5PasswordFilePrincipalDatabase;
+import org.apache.qpid.server.security.auth.sasl.crammd5.CRAMMD5HexInitialiser;
+import org.apache.qpid.server.security.auth.sasl.crammd5.CRAMMD5HexSaslServer;
+import org.apache.qpid.server.security.auth.sasl.crammd5.CRAMMD5HexServerFactory;
+
+/**
+ * Test for the CRAM-MD5-HEX SASL mechanism.
+ *
+ * This test case focuses on testing {@link CRAMMD5HexSaslServer} but also exercises
+ * collaborators {@link CRAMMD5HexInitialiser} and {@link Base64MD5PasswordFilePrincipalDatabase}
+ */
+public class CRAMMD5HexServerTest extends TestCase
+{
+
+    private SaslServer _saslServer;  // Class under test
+    private CRAMMD5HexServerFactory _saslFactory;
+
+    @Override
+    protected void setUp() throws Exception
+    {
+        super.setUp();
+
+        CRAMMD5HexInitialiser _initializer = new CRAMMD5HexInitialiser();
+
+        //Use properties to create a PrincipalDatabase
+        Base64MD5PasswordFilePrincipalDatabase db = createTestPrincipalDatabase();
+        assertEquals("Unexpected number of test users in the db", 2, db.getUsers().size());
+
+        _initializer.initialise(db);
+
+        _saslFactory = new CRAMMD5HexServerFactory();
+
+        _saslServer = _saslFactory.createSaslServer(CRAMMD5HexSaslServer.MECHANISM,
+                "AMQP",
+                "localhost",
+                _initializer.getProperties(),
+                _initializer.getCallbackHandler());
+        assertNotNull("Unable to create saslServer with mechanism type " + CRAMMD5HexSaslServer.MECHANISM, _saslServer);
+
+    }
+
+    public void testSuccessfulAuth() throws Exception
+    {
+
+        final byte[] serverChallenge = _saslServer.evaluateResponse(new byte[0]);
+
+        // Generate client response
+        final byte[] clientResponse = generateClientResponse("knownuser", "guest", serverChallenge);
+
+
+        byte[] nextServerChallenge = _saslServer.evaluateResponse(clientResponse);
+        assertTrue("Exchange must be flagged as complete after successful authentication", _saslServer.isComplete());
+        assertNull("Next server challenge must be null after successful authentication", nextServerChallenge);
+
+    }
+
+    public void testKnownUserPresentsWrongPassword() throws Exception
+    {
+        byte[] serverChallenge = _saslServer.evaluateResponse(new byte[0]);
+
+
+        final byte[] clientResponse = generateClientResponse("knownuser", "wrong!", serverChallenge);
+        try
+        {
+            _saslServer.evaluateResponse(clientResponse);
+            fail("Exception not thrown");
+        }
+        catch (SaslException se)
+        {
+            // PASS
+        }
+        assertFalse("Exchange must not be flagged as complete after unsuccessful authentication", _saslServer.isComplete());
+    }
+
+    public void testUnknownUser() throws Exception
+    {
+        final byte[] serverChallenge = _saslServer.evaluateResponse(new byte[0]);
+
+
+        final byte[] clientResponse = generateClientResponse("unknownuser", "guest", serverChallenge);
+
+        try
+        {
+            _saslServer.evaluateResponse(clientResponse);
+            fail("Exception not thrown");
+        }
+        catch (SaslException se)
+        {
+            assertExceptionHasUnderlyingAsCause(AccountNotFoundException.class, se);
+            // PASS
+        }
+        assertFalse("Exchange must not be flagged as complete after unsuccessful authentication", _saslServer.isComplete());
+    }
+
+    /**
+     *
+     * Demonstrates QPID-3158.  A defect meant that users with some valid password were failing to 
+     * authenticate when using the .NET 0-8 client (uses this SASL mechanism).  
+     * It so happens that password "guest2" was one of the affected passwords.
+     *
+     * @throws Exception
+     */
+    public void testSuccessfulAuthReproducingQpid3158() throws Exception
+    {
+        byte[] serverChallenge = _saslServer.evaluateResponse(new byte[0]);
+
+        // Generate client response
+        byte[] resp = generateClientResponse("qpid3158user", "guest2", serverChallenge);
+
+        byte[] nextServerChallenge = _saslServer.evaluateResponse(resp);
+        assertTrue("Exchange must be flagged as complete after successful authentication", _saslServer.isComplete());
+        assertNull("Next server challenge must be null after successful authentication", nextServerChallenge);
+    }
+
+    /**
+     * Since we don't have a CRAM-MD5-HEX implementation client implementation in Java, this method
+     * provides the implementation for first principals.
+     *
+     * @param userId user id
+     * @param clearTextPassword clear text password
+     * @param serverChallenge challenge from server
+     *
+     * @return challenge response
+     */
+    private byte[] generateClientResponse(final String userId, final String clearTextPassword, final byte[] serverChallenge) throws Exception
+    {
+        byte[] digestedPasswordBytes = MessageDigest.getInstance("MD5").digest(clearTextPassword.getBytes());
+        char[] hexEncodedDigestedPassword = Hex.encodeHex(digestedPasswordBytes);
+        byte[] hexEncodedDigestedPasswordBytes = new String(hexEncodedDigestedPassword).getBytes();
+
+
+        Mac hmacMd5 = Mac.getInstance("HmacMD5");
+        hmacMd5.init(new SecretKeySpec(hexEncodedDigestedPasswordBytes, "HmacMD5"));
+        final byte[] messageAuthenticationCode = hmacMd5.doFinal(serverChallenge);
+
+        // Build client response
+        String responseAsString = userId + " " + new String(Hex.encodeHex(messageAuthenticationCode));
+        byte[] resp = responseAsString.getBytes();
+        return resp;
+    }
+
+    /**
+     * Creates a test principal database.
+     *
+     * @return
+     * @throws IOException
+     */
+    private Base64MD5PasswordFilePrincipalDatabase createTestPrincipalDatabase() throws IOException
+    {
+        Base64MD5PasswordFilePrincipalDatabase db = new Base64MD5PasswordFilePrincipalDatabase();
+        File file = File.createTempFile("passwd", "db");
+        file.deleteOnExit();
+        db.setPasswordFile(file.getCanonicalPath());
+        db.createPrincipal( createTestPrincipal("knownuser"), "guest".toCharArray());
+        db.createPrincipal( createTestPrincipal("qpid3158user"), "guest2".toCharArray());
+        return db;
+    }
+
+    private Principal createTestPrincipal(final String name)
+    {
+        return new Principal()
+        {
+
+            @Override
+            public String getName()
+            {
+                return name;
+            }
+        };
+    }
+    
+    private void assertExceptionHasUnderlyingAsCause(final Class<? extends Throwable> expectedUnderlying, Throwable e)
+    {
+        assertNotNull(e);
+        int infiniteLoopGuard = 0;  // Guard against loops in the cause chain
+        boolean foundExpectedUnderlying = false;
+        while (e.getCause() != null && infiniteLoopGuard++ < 10)
+        {
+            if (expectedUnderlying.equals(e.getCause().getClass()))
+            {
+                foundExpectedUnderlying = true;
+                break;
+            }
+            e = e.getCause();
+        }
+        
+        if (!foundExpectedUnderlying)
+        {
+            fail("Not found expected underlying exception " + expectedUnderlying + " as underlying cause of " + e.getClass());
+        }
+    }
+
+}
diff --git a/qpid/java/client/src/main/java/org/apache/qpid/client/security/amqplain/AmqPlainSaslClientFactory.java b/qpid/java/client/src/main/java/org/apache/qpid/client/security/amqplain/AmqPlainSaslClientFactory.java
index 30cc786890..26bb9bb1f8 100644
--- a/qpid/java/client/src/main/java/org/apache/qpid/client/security/amqplain/AmqPlainSaslClientFactory.java
+++ b/qpid/java/client/src/main/java/org/apache/qpid/client/security/amqplain/AmqPlainSaslClientFactory.java
@@ -48,9 +48,10 @@ public class AmqPlainSaslClientFactory implements SaslClientFactory
 
     public String[] getMechanismNames(Map props)
     {
-        if (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
-            props.containsKey(Sasl.POLICY_NODICTIONARY) ||
-            props.containsKey(Sasl.POLICY_NOACTIVE))
+        if (props != null &&
+            (props.containsKey(Sasl.POLICY_NOPLAINTEXT) ||
+             props.containsKey(Sasl.POLICY_NODICTIONARY) ||
+             props.containsKey(Sasl.POLICY_NOACTIVE)))
         {
             // returned array must be non null according to interface documentation
             return new String[0];
diff --git a/qpid/java/tools/build.xml b/qpid/java/tools/build.xml
index 7cd1b1172c..c521bc38c4 100644
--- a/qpid/java/tools/build.xml
+++ b/qpid/java/tools/build.xml
@@ -24,4 +24,14 @@
 
   <import file="../module.xml"/>
 
+  <target name="jar" depends="jar.manifest,jar.nomanifest,jar.manifest.qpidpasswordcheck" description="create jars"/>
+
+  <!-- Extra target to build QpidPasswordCheck as workaround QPID-3158 -->
+  <target name="jar.manifest.qpidpasswordcheck">
+      <jar destfile="${build.lib}/${project.name}-passwordcheck-${project.version}.jar" basedir="${module.classes}" includes="**/QpidPasswordCheck.class">
+          <manifest>
+              <attribute name="Main-Class" value="org.apache.qpid.tools.QpidPasswordCheck"/>
+          </manifest>
+      </jar>
+  </target>
 </project>
diff --git a/qpid/java/tools/src/main/java/org/apache/qpid/tools/QpidPasswordCheck.java b/qpid/java/tools/src/main/java/org/apache/qpid/tools/QpidPasswordCheck.java
new file mode 100644
index 0000000000..3f243248e5
--- /dev/null
+++ b/qpid/java/tools/src/main/java/org/apache/qpid/tools/QpidPasswordCheck.java
@@ -0,0 +1,134 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.tools;
+
+import java.io.Console;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
+
+/**
+ * Command line tool to determine if a password will be affected by QPID-3158.
+ */
+public class QpidPasswordCheck
+{
+    
+    public static void main(String[] argv) throws Exception
+    {
+        Console console = System.console();
+        
+        if (console == null) {
+            System.err.println("Console not available.  This utility must be run on the command line.");
+            System.exit(-1);
+        }
+        else 
+        {
+            System.err.println("Defect QPID-3158 in the Qpid java broker (releases 2.6.0.8 and below) means that certain valid client " +
+            		"passwords cause spurious authentication failures when used from the Qpid 0-8 .NET client with a broker " +
+                        "using the Base64MD5 password database type.");
+            System.err.println();
+            System.err.println("This utility accepts a candidate password and reports if the password will cause the defect.");
+            System.err.println();
+            
+            final char[] password = console.readPassword("Enter candidate password: ");
+            final char[] repeatedPassword = console.readPassword("Enter candidate password again: ");
+
+            if (!Arrays.equals(password,repeatedPassword))
+            {
+                System.err.println("Sorry, those passwords do not match.");
+                System.exit(-1);
+            }
+            
+            
+            final byte[] hashedPassword = MessageDigest.getInstance("MD5").digest(new String(password).getBytes());
+            
+
+            final char[] hashedPasswordChars = new char[hashedPassword.length];
+
+            int index = 0;
+            for (byte c : hashedPassword)
+            {
+                hashedPasswordChars[index++] = (char) c;
+            }
+            
+            char[] brokenRepresentation = brokenToHex(hashedPasswordChars);
+            char[] correctRepresentation = fixedToHex(hashedPasswordChars);
+            
+            if (Arrays.equals(brokenRepresentation,correctRepresentation))
+            {
+                System.err.println("Password is suitable for use.");
+                System.exit(0);
+            }
+            else 
+            {
+                System.err.println("Sorry, that password is NOT suitable for use.");
+                System.exit(1);
+            }
+            
+        }    
+    }
+    
+    
+    private static char[] brokenToHex(char[] password)
+    {
+        StringBuilder sb = new StringBuilder();
+        for (char c : password)
+        {
+            //toHexString does not prepend 0 so we have to
+            if (((byte) c > -1) && (byte) c < 10 )
+            {
+                sb.append(0);
+            }
+
+            sb.append(Integer.toHexString(c & 0xFF));
+        }
+
+        //Extract the hex string as char[]
+        char[] hex = new char[sb.length()];
+
+        sb.getChars(0, sb.length(), hex, 0);
+
+        return hex;
+    }
+
+    private static char[] fixedToHex(char[] password)
+    {
+        StringBuilder sb = new StringBuilder();
+        for (char c : password)
+        {
+            //toHexString does not prepend 0 so we have to
+            if (((byte) c > -1) && (byte) c < 0x10 )
+            {
+                sb.append(0);
+            }
+
+            sb.append(Integer.toHexString(c & 0xFF));
+        }
+
+        //Extract the hex string as char[]
+        char[] hex = new char[sb.length()];
+
+        sb.getChars(0, sb.length(), hex, 0);
+
+        return hex;
+    }
+
+}