commit 8eb8ca1c54f67cdfb29b04184d2bfc24419e1ef0
author David Pursehouse <dpursehouse@collab.net> Wed Mar 11 23:48:14 2020 +0000
committer Gerrit Code Review <noreply-gerritcodereview@google.com> Wed Mar 11 23:48:14 2020 +0000
tree 52d4e11c45c441973cada27c5dafc5a548dfae05
parent 55d5b0cbdb1b975649cbb6d295d6881a0d811254
parent 3d8dc0f7e66d54e0f6ed245ee713958e389a654b

Merge "Null-terminate passwords by default before hashing them"

java/com/google/gerrit/server/account/HashedPassword.java

diff --git a/java/com/google/gerrit/server/account/HashedPassword.java b/java/com/google/gerrit/server/account/HashedPassword.java
index 64a4495..09115503 100644
--- a/java/com/google/gerrit/server/account/HashedPassword.java
+++ b/java/com/google/gerrit/server/account/HashedPassword.java
@@ -31,6 +31,7 @@
  */
 public class HashedPassword {
   private static final String ALGORITHM_PREFIX = "bcrypt:";
+  private static final String ALGORITHM_PREFIX_0 = "bcrypt0:";
   private static final SecureRandom secureRandom = new SecureRandom();
   private static final BaseEncoding codec = BaseEncoding.base64();
 
@@ -52,7 +53,7 @@
    * @throws DecoderException if input is malformed.
    */
   public static HashedPassword decode(String encoded) throws DecoderException {
-    if (!encoded.startsWith(ALGORITHM_PREFIX)) {
+    if (!encoded.startsWith(ALGORITHM_PREFIX) && !encoded.startsWith(ALGORITHM_PREFIX_0)) {
       throw new DecoderException("unrecognized algorithm");
     }
 
@@ -74,19 +75,24 @@
     if (salt.length != 16) {
       throw new DecoderException("salt should be 16 bytes, got " + salt.length);
     }
-    return new HashedPassword(codec.decode(fields.get(3)), salt, cost);
+    return new HashedPassword(
+        codec.decode(fields.get(3)), salt, cost, encoded.startsWith(ALGORITHM_PREFIX_0));
   }
 
-  private static byte[] hashPassword(String password, byte[] salt, int cost) {
+  private static byte[] hashPassword(
+      String password, byte[] salt, int cost, boolean nullTerminate) {
     byte[] pwBytes = password.getBytes(StandardCharsets.UTF_8);
-
+    if (nullTerminate && !password.endsWith("\0")) {
+      pwBytes = Arrays.append(pwBytes, (byte) 0);
+    }
     return BCrypt.generate(pwBytes, salt, cost);
   }
 
   public static HashedPassword fromPassword(String password) {
     byte[] salt = newSalt();
 
-    return new HashedPassword(hashPassword(password, salt, DEFAULT_COST), salt, DEFAULT_COST);
+    return new HashedPassword(
+        hashPassword(password, salt, DEFAULT_COST, true), salt, DEFAULT_COST, true);
   }
 
   private static byte[] newSalt() {
@@ -98,11 +104,15 @@
   private byte[] salt;
   private byte[] hashed;
   private int cost;
+  // Raw bcrypt repeats the password, so "ABC" works for "ABCABC" too. To prevent this, add
+  // the terminating null char to the password.
+  boolean nullTerminate;
 
-  private HashedPassword(byte[] hashed, byte[] salt, int cost) {
+  private HashedPassword(byte[] hashed, byte[] salt, int cost, boolean nullTerminate) {
     this.salt = salt;
     this.hashed = hashed;
     this.cost = cost;
+    this.nullTerminate = nullTerminate;
 
     checkState(cost >= 4 && cost < 32);
 
@@ -116,11 +126,16 @@
    * @return one-line string encoding the hash and salt.
    */
   public String encode() {
-    return ALGORITHM_PREFIX + cost + ":" + codec.encode(salt) + ":" + codec.encode(hashed);
+    return (nullTerminate ? ALGORITHM_PREFIX_0 : ALGORITHM_PREFIX)
+        + cost
+        + ":"
+        + codec.encode(salt)
+        + ":"
+        + codec.encode(hashed);
   }
 
   public boolean checkPassword(String password) {
     // Constant-time comparison, because we're paranoid.
-    return Arrays.areEqual(hashPassword(password, salt, cost), hashed);
+    return Arrays.areEqual(hashPassword(password, salt, cost, nullTerminate), hashed);
   }
 }

javatests/com/google/gerrit/server/account/HashedPasswordTest.java

diff --git a/javatests/com/google/gerrit/server/account/HashedPasswordTest.java b/javatests/com/google/gerrit/server/account/HashedPasswordTest.java
index 3443720..066502d 100644
--- a/javatests/com/google/gerrit/server/account/HashedPasswordTest.java
+++ b/javatests/com/google/gerrit/server/account/HashedPasswordTest.java
@@ -53,4 +53,22 @@
     assertThat(hashed.checkPassword("false")).isFalse();
     assertThat(hashed.checkPassword(password)).isTrue();
   }
+
+  @Test
+  public void repeatedPasswordFail() throws Exception {
+    String password = "secret";
+    HashedPassword hashed = HashedPassword.fromPassword(password);
+
+    assertThat(hashed.checkPassword(password + password)).isFalse();
+    assertThat(hashed.checkPassword(password)).isTrue();
+  }
+
+  @Test
+  public void cyclicPasswordTest() throws Exception {
+    String encoded = "bcrypt:4:/KgSxlmbopLXb1eDm35DBA==:98n3gu2pKW9D5mCoZ5kNn9v4HcVFPPJy";
+    HashedPassword hashedPassword = HashedPassword.decode(encoded);
+    String password = "abcdef";
+    assertThat(hashedPassword.checkPassword(password)).isTrue();
+    assertThat(hashedPassword.checkPassword(password + password)).isTrue();
+  }
 }