Improve security of BCrypt-based password encoding

turms-server-common/src/main/java/im/turms/server/common/infra/collection/ArrayUtil.java

@@ -28,9 +28,19 @@ private ArrayUtil() {
     }
 
     public static byte[] concat(byte[] a, byte[] b) {
-        byte[] result = new byte[a.length + b.length];
-        System.arraycopy(a, 0, result, 0, a.length);
-        System.arraycopy(b, 0, result, a.length, b.length);
+        int aLength = a.length;
+        int bLength = b.length;
+        byte[] result = new byte[aLength + bLength];
+        System.arraycopy(a, 0, result, 0, aLength);
+        System.arraycopy(b, 0, result, aLength, bLength);
+        return result;
+    }
+
+    public static byte[] concat(byte[] a, byte b) {
+        int length = a.length;
+        byte[] result = new byte[length + 1];
+        System.arraycopy(a, 0, result, 0, length);
+        result[length] = b;
         return result;
     }
 

turms-server-common/src/main/java/im/turms/server/common/infra/security/password/BCryptPasswordEncoder.java

@@ -31,7 +31,12 @@
 public class BCryptPasswordEncoder implements PasswordEncoder {
 
     public static final int SALT_SIZE_BYTES = 16;
+    /**
+     * This isn't the standard implementation of BCrypt, and is used for Turms only.
+     */
+    public static final byte VERSION_BYTES = 1;
     private static final int COST = 10;
+    private static final byte NULL_TERMINATOR = (byte) 0;
 
     private static final FastThreadLocal<BCrypt> BCRYPT = new FastThreadLocal<>() {
         @Override
@@ -40,26 +45,74 @@ protected BCrypt initialValue() {
         }
     };
 
+    /**
+     * @implNote The implementation doesn't validate the null terminator in the raw password to
+     *           avoid double checks, and it is the responsibility of the caller to ensure that no
+     *           null terminator is in the raw password.
+     */
     @Override
     public byte[] encode(byte[] rawPassword) {
+        int length = rawPassword.length;
+        if (length > BCrypt.MAX_PASSWORD_BYTES) {
+            throw new IllegalArgumentException(
+                    "The password length must be less than "
+                            + BCrypt.MAX_PASSWORD_BYTES);
+        }
+        if (length != BCrypt.MAX_PASSWORD_BYTES) {
+            rawPassword = ArrayUtil.concat(rawPassword, NULL_TERMINATOR);
+        }
         byte[] salt = new byte[SALT_SIZE_BYTES];
         ThreadLocalRandom.current()
                 .nextBytes(salt);
-        byte[] password = BCRYPT.get()
-                .generate(rawPassword, salt, COST);
-        return ArrayUtil.concat(salt, password);
+        byte[] encodedPassword = BCRYPT.get()
+                .deriveRawKey(COST, salt, rawPassword);
+
+        int encodedPasswordLength = encodedPassword.length;
+        int resultLength = VERSION_BYTES + SALT_SIZE_BYTES + encodedPasswordLength;
+        byte[] result = new byte[resultLength];
+        System.arraycopy(salt, 0, result, 0, SALT_SIZE_BYTES);
+        System.arraycopy(encodedPassword, 0, result, SALT_SIZE_BYTES, encodedPasswordLength);
+        // The last byte indicates the version and cost.
+        result[resultLength - 1] = (byte) COST;
+        return result;
     }
 
     @Override
     public boolean matches(byte[] rawPassword, byte[] saltedPasswordWithSalt) {
+        int length = rawPassword.length;
+        if (length > BCrypt.MAX_PASSWORD_BYTES) {
+            return false;
+        }
+        for (byte b : rawPassword) {
+            if (b == NULL_TERMINATOR) {
+                return false;
+            }
+        }
+        // If the length is 40, it means the password of the first version that has no null
+        // terminator.
+        int saltedPasswordWithSaltLength = saltedPasswordWithSalt.length;
+        if (saltedPasswordWithSaltLength == 40) {
+            byte[] saltedPassword = BCRYPT.get()
+                    .deriveRawKey(10, saltedPasswordWithSalt, rawPassword);
+            return Arrays.equals(saltedPassword,
+                    0,
+                    saltedPassword.length,
+                    saltedPasswordWithSalt,
+                    SALT_SIZE_BYTES,
+                    saltedPasswordWithSaltLength);
+        }
+        int cost = saltedPasswordWithSalt[saltedPasswordWithSaltLength - 1] & 0xFF;
+        if (length != BCrypt.MAX_PASSWORD_BYTES) {
+            rawPassword = ArrayUtil.concat(rawPassword, NULL_TERMINATOR);
+        }
         byte[] saltedPassword = BCRYPT.get()
-                .generate(rawPassword, saltedPasswordWithSalt, COST);
+                .deriveRawKey(cost, saltedPasswordWithSalt, rawPassword);
         return Arrays.equals(saltedPassword,
                 0,
                 saltedPassword.length,
                 saltedPasswordWithSalt,
                 SALT_SIZE_BYTES,
-                saltedPasswordWithSalt.length);
+                saltedPasswordWithSaltLength - 1);
     }
 
-}
+}

turms-server-common/src/main/java/org/bouncycastle/crypto/generators/BCrypt.java

@@ -1281,7 +1281,7 @@ private void processTableWithSalt(int[] table, int[] salt32Bit, int iv1, int iv2
      * @param psw  the password
      * @return a 192 bit key
      */
-    private final byte[] deriveRawKey(int cost, byte[] salt, byte[] psw) {
+    public final byte[] deriveRawKey(int cost, byte[] salt, byte[] psw) {
 //        if (salt.length != 16) {
 //            throw new DataLengthException("Invalid salt size: 16 bytes expected.");
 //        }
@@ -1350,37 +1350,6 @@ private final byte[] deriveRawKey(int cost, byte[] salt, byte[] psw) {
      */
     // Blowfish spec limits keys to 448bit/56 bytes to ensure all bits of key affect all ciphertext
     // bits, but technically algorithm handles 72 byte keys and most implementations support this.
-    static final int MAX_PASSWORD_BYTES = 72;
+    public static final int MAX_PASSWORD_BYTES = 72;
 
-    /**
-     * Calculates the <b>bcrypt</b> hash of an input - note for processing general passwords you
-     * want to make sure the password is terminated in a manner similar to what is done by
-     * passwordToByteArray().
-     * <p>
-     * This implements the raw <b>bcrypt</b> function as defined in the bcrypt specification, not
-     * the crypt encoded version implemented in OpenBSD.
-     * </p>
-     *
-     * @param pwInput the password bytes (up to 72 bytes) to use for this invocation.
-     * @param salt    the 128 bit salt to use for this invocation.
-     * @param cost    the bcrypt cost parameter. The cost of the bcrypt function grows as
-     *                <code>2^cost</code>. Legal values are 4..31 inclusive.
-     * @return the output of the raw bcrypt operation: a 192 bit (24 byte) hash.
-     */
-    public byte[] generate(byte[] pwInput, byte[] salt, int cost) {
-        if (pwInput == null || salt == null) {
-            throw new IllegalArgumentException("pwInput and salt are required");
-        }
-//        if (salt.length != SALT_SIZE_BYTES) {
-//            throw new IllegalArgumentException("BCrypt salt must be 128 bits");
-//        }
-        if (pwInput.length > MAX_PASSWORD_BYTES) {
-            throw new IllegalArgumentException("BCrypt password must be <= 72 bytes");
-        }
-        if (cost < MIN_COST || cost > MAX_COST) {
-            throw new IllegalArgumentException("BCrypt cost must be from 4..31");
-        }
-
-        return deriveRawKey(cost, salt, pwInput);
-    }
 }

turms-server-common/src/test/java/unit/im/turms/server/common/infra/security/password/BCryptPasswordEncoderTests.java

@@ -17,6 +17,7 @@
 
 package unit.im.turms.server.common.infra.security.password;
 
+import org.bouncycastle.crypto.generators.BCrypt;
 import org.junit.jupiter.api.Test;
 
 import im.turms.server.common.infra.security.password.BCryptPasswordEncoder;
@@ -28,9 +29,24 @@ class BCryptPasswordEncoderTests extends BasePasswordEncoderTests {
 
     @Test
     void test() {
+        BCryptPasswordEncoder encoder = new BCryptPasswordEncoder();
         int bcryptOutputLength = 24;
-        test(new BCryptPasswordEncoder(),
-                BCryptPasswordEncoder.SALT_SIZE_BYTES + bcryptOutputLength);
+        int expectedEncodedPasswordLength = BCryptPasswordEncoder.VERSION_BYTES
+                + BCryptPasswordEncoder.SALT_SIZE_BYTES + bcryptOutputLength;
+
+        test(encoder, expectedEncodedPasswordLength);
+        String maxPassword = "1".repeat(BCrypt.MAX_PASSWORD_BYTES);
+        assertMatchResult(encoder, maxPassword, maxPassword, true, expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                "1".repeat(BCrypt.MAX_PASSWORD_BYTES - 1),
+                maxPassword,
+                false,
+                expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                maxPassword,
+                "1".repeat(BCrypt.MAX_PASSWORD_BYTES + 1),
+                false,
+                expectedEncodedPasswordLength);
     }
 
 }

turms-server-common/src/test/java/unit/im/turms/server/common/infra/security/password/BasePasswordEncoderTests.java

@@ -29,17 +29,50 @@
 public abstract class BasePasswordEncoderTests {
 
     void test(PasswordEncoder encoder, int expectedEncodedPasswordLength) {
-        byte[] rawPassword = "asdfjh9^(*:L}{<sdfy516测试テスト".getBytes(StandardCharsets.UTF_8);
-        byte[] encodedPassword = encoder.encode(rawPassword);
-        boolean matches = encoder.matches(rawPassword, encodedPassword);
-
-        assertThat(matches).isTrue();
-        assertThat(encodedPassword).hasSize(expectedEncodedPasswordLength);
+        String password = "asdfjh9^(*:L}{<sdfy516测试テスト";
+        assertMatchResult(encoder, password, password, true, expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                password,
+                "asdfjh9^(*:L}\0abc",
+                false,
+                expectedEncodedPasswordLength);
+        assertMatchResult(encoder, "abc", "abcdef", false, expectedEncodedPasswordLength);
+        // Reference: https://github.com/bcgit/bc-java/issues/1496
+        assertMatchResult(encoder, "abc", "abcabc", false, expectedEncodedPasswordLength);
+        assertMatchResult(encoder, "abc", "abc\0abc", false, expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                "abc\0{<sdfy516",
+                "abc\0abc",
+                false,
+                expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                "0123456789012345678901234567890123456789",
+                "01234567890123456789012345678901234567890123456789",
+                false,
+                expectedEncodedPasswordLength);
+        assertMatchResult(encoder,
+                "0123456789012345678901234567890123456789",
+                "0123456789012345678901234567890123456789'\0'0123456789",
+                false,
+                expectedEncodedPasswordLength);
+    }
 
-        rawPassword = "asdfjh9^(*:L}{<sdfy516".getBytes(StandardCharsets.UTF_8);
-        matches = encoder.matches(rawPassword, encodedPassword);
+    public void assertMatchResult(
+            PasswordEncoder encoder,
+            String password1,
+            String password2,
+            boolean expectedMatched,
+            int expectedEncodedPasswordLength) {
+        byte[] encodedPassword;
+        byte[] rawPassword;
+        boolean actualMatched;
+        rawPassword = password1.getBytes(StandardCharsets.UTF_8);
+        encodedPassword = encoder.encode(rawPassword);
+        rawPassword = password2.getBytes(StandardCharsets.UTF_8);
+        actualMatched = encoder.matches(rawPassword, encodedPassword);
 
-        assertThat(matches).isFalse();
+        assertThat(actualMatched).isEqualTo(expectedMatched);
+        assertThat(encodedPassword).hasSize(expectedEncodedPasswordLength);
     }
 
 }