use a time constant comparison function for security

For HMAC and password hash comparisons we need to use a time
constant comparison that always compares the whole value in order
to protect against timing attacks.

Original commit: elastic/x-pack-elasticsearch@f6082c76b9

src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java

@@ -751,12 +751,12 @@ public class BCrypt {
 	 * Check that a plaintext password matches a previously hashed
 	 * one.
      *
-     * Modified from the original to take a SecuredString plaintext
+     * Modified from the original to take a SecuredString plaintext and use a constant time comparison
 	 * @param plaintext	the plaintext password to verify
 	 * @param hashed	the previously-hashed password
 	 * @return	true if the passwords match, false otherwise
 	 */
 	public static boolean checkpw(SecuredString plaintext, String hashed) {
-		return hashed.compareTo(hashpw(plaintext, hashed)) == 0;
+		return SecuredString.constantTimeEquals(hashed, hashpw(plaintext, hashed));
 	}
 }

src/main/java/org/elasticsearch/shield/authc/support/Hasher.java

@@ -14,7 +14,6 @@ import org.elasticsearch.shield.ShieldSettingsException;
 
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
-import java.util.Arrays;
 import java.util.Locale;
 import java.util.Random;
 import java.util.concurrent.ThreadLocalRandom;
@@ -44,22 +43,23 @@ public enum Hasher {
             }
             if (hashStr.startsWith(PLAIN_PREFIX)) {
                 hashStr = hashStr.substring(PLAIN_PREFIX.length());
-                return text.equals(hashStr);
+                return SecuredString.constantTimeEquals(text, hashStr);
             }
             byte[] textBytes = CharArrays.toUtf8Bytes(text.internalChars());
             if (hashStr.startsWith(APR1_PREFIX)) {
-                return hashStr.compareTo(Md5Crypt.apr1Crypt(textBytes, hashStr)) == 0;
+                return SecuredString.constantTimeEquals(hashStr, Md5Crypt.apr1Crypt(textBytes, hashStr));
             }
             if (hashStr.startsWith(SHA1_PREFIX)) {
                 String passwd64 = Base64.encodeBase64String(DigestUtils.sha1(textBytes));
-                return hashStr.substring(SHA1_PREFIX.length()).compareTo(passwd64) == 0;
+                String hashNoPrefix = hashStr.substring(SHA1_PREFIX.length());
+                return SecuredString.constantTimeEquals(passwd64, hashNoPrefix);
             }
             if (hashStr.startsWith(SHA2_PREFIX_5) || hashStr.startsWith(SHA2_PREFIX_6)) {
-                return hashStr.compareTo(Sha2Crypt.sha256Crypt(textBytes, hashStr)) == 0;
+                return SecuredString.constantTimeEquals(hashStr, Sha2Crypt.sha256Crypt(textBytes, hashStr));
             }
             return CRYPT_SUPPORTED ?
-                    hashStr.compareTo(Crypt.crypt(textBytes, hashStr)) == 0 :  // crypt algo
+                    SecuredString.constantTimeEquals(hashStr, Crypt.crypt(textBytes, hashStr)) :  // crypt algo
-                    text.equals(hashStr);                                      // plain text
+                    SecuredString.constantTimeEquals(text, hashStr);                              // plain text
         }
     },
 
@@ -197,7 +197,7 @@ public enum Hasher {
                 return false;
             }
             byte[] textBytes = CharArrays.toUtf8Bytes(text.internalChars());
-            return hashStr.compareTo(Md5Crypt.apr1Crypt(textBytes, hashStr)) == 0;
+            return SecuredString.constantTimeEquals(hashStr, Md5Crypt.apr1Crypt(textBytes, hashStr));
         }
     },
 
@@ -221,7 +221,8 @@ public enum Hasher {
             MessageDigest md = SHA1Provider.sha1();
             md.update(textBytes);
             String passwd64 = Base64.encodeBase64String(md.digest());
-            return hashStr.substring(SHA1_PREFIX.length()).compareTo(passwd64) == 0;
+            String hashNoPrefix = hashStr.substring(SHA1_PREFIX.length());
+            return SecuredString.constantTimeEquals(hashNoPrefix, passwd64);
         }
     },
 
@@ -237,7 +238,7 @@ public enum Hasher {
             String hashStr = new String(hash);
             if (hashStr.startsWith(SHA2_PREFIX_5) || hashStr.startsWith(SHA2_PREFIX_6)) {
                 byte[] textBytes = CharArrays.toUtf8Bytes(text.internalChars());
-                return hashStr.compareTo(Sha2Crypt.sha256Crypt(textBytes, hashStr)) == 0;
+                return SecuredString.constantTimeEquals(hashStr, Sha2Crypt.sha256Crypt(textBytes, hashStr));
             }
             return false;
         }
@@ -262,7 +263,7 @@ public enum Hasher {
             MessageDigest md = MD5Provider.md5();
             md.update(CharArrays.toUtf8Bytes(text.internalChars()));
             String computedHashStr = Base64.encodeBase64String(md.digest());
-            return hashStr.equals(computedHashStr);
+            return SecuredString.constantTimeEquals(hashStr, computedHashStr);
         }
     },
 
@@ -293,7 +294,7 @@ public enum Hasher {
             md.update(CharArrays.toUtf8Bytes(text.internalChars()));
             md.update(new String(saltAndHash, 0, 8).getBytes(Charsets.UTF_8));
             String computedHash = Base64.encodeBase64String(md.digest());
-            return computedHash.equals(new String(saltAndHash, 8, saltAndHash.length - 8));
+            return SecuredString.constantTimeEquals(computedHash, new String(saltAndHash, 8, saltAndHash.length - 8));
         }
     },
 
@@ -305,7 +306,7 @@ public enum Hasher {
 
         @Override
         public boolean verify(SecuredString text, char[] hash) {
-            return Arrays.equals(text.internalChars(), hash);
+            return SecuredString.constantTimeEquals(text.internalChars(), hash);
         }
     };
 

src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java

@@ -155,4 +155,61 @@ public class SecuredString implements CharSequence {
             throw new ElasticsearchException("attempt to use cleared password");
         }
     }
+
+    /**
+     * This does a char by char comparison of the two Strings to provide protection against timing attacks. In other
+     * words it does not exit at the first character that does not match and only exits at the end of the comparison.
+     *
+     * NOTE: length will cause this function to exit early, which is OK as it is not considered feasible to prevent
+     * length attacks
+     *
+     * @param a the first string to be compared
+     * @param b the second string to be compared
+     * @return true if both strings match completely
+     */
+    public static boolean constantTimeEquals(String a, String b) {
+        char[] aChars = a.toCharArray();
+        char[] bChars = b.toCharArray();
+
+        return constantTimeEquals(aChars, bChars);
+    }
+
+    /**
+     * This does a char by char comparison of the two Strings to provide protection against timing attacks. In other
+     * words it does not exit at the first character that does not match and only exits at the end of the comparison.
+     *
+     * NOTE: length will cause this function to exit early, which is OK as it is not considered feasible to prevent
+     * length attacks
+     *
+     * @param securedString the securedstring to compare to string char by char
+     * @param string the string to compare
+     * @return true if both match char for char
+     */
+    public static boolean constantTimeEquals(SecuredString securedString, String string) {
+        return constantTimeEquals(securedString.internalChars(), string.toCharArray());
+    }
+
+    /**
+     * This does a char by char comparison of the two arrays to provide protection against timing attacks. In other
+     * words it does not exit at the first character that does not match and only exits at the end of the comparison.
+     *
+     * NOTE: length will cause this function to exit early, which is OK as it is not considered feasible to prevent
+     * length attacks
+     *
+     * @param a the first char array
+     * @param b the second char array
+     * @return true if both match char for char
+     */
+    public static boolean constantTimeEquals(char[] a, char[] b) {
+        if (a.length != b.length) {
+            return false;
+        }
+
+        int equals = 0;
+        for (int i = 0; i < a.length; i++) {
+            equals |= a[i] ^ b[i];
+        }
+
+        return equals == 0;
+    }
 }

src/main/java/org/elasticsearch/shield/signature/InternalSignatureService.java

@@ -29,6 +29,8 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.regex.Pattern;
 
+import static org.elasticsearch.shield.authc.support.SecuredString.constantTimeEquals;
+
 /**
  *
  */
@@ -105,26 +107,34 @@ public class InternalSignatureService extends AbstractLifecycleComponent<Interna
             return signedText;
         }
 
-        if (!signedText.startsWith("$$")) {
+        if (!signedText.startsWith("$$") || signedText.length() < 2) {
+            throw new SignatureException("tampered signed text");
+        }
+
+        String text;
+        String receivedSignature;
+        try {
+            // $$34$$sigtext
+            int i = signedText.indexOf("$$", 2);
+            int length = Integer.parseInt(signedText.substring(2, i));
+            receivedSignature = signedText.substring(i + 2, i + 2 + length);
+            text = signedText.substring(i + 2 + length);
+        } catch (Throwable t) {
+            logger.error("error occurred while parsing signed text", t);
             throw new SignatureException("tampered signed text");
         }
 
         try {
-            //     $$34$$sigtext
-            int i = signedText.indexOf("$$", 2);
-            int length = Integer.parseInt(signedText.substring(2, i));
-            String sigStr = signedText.substring(i + 2, i + 2 + length);
-            String text = signedText.substring(i + 2 + length);
             String sig = signInternal(text);
-            if (!sig.equals(sigStr)) {
+            if (constantTimeEquals(sig, receivedSignature)) {
-                throw new SignatureException("the signed texts don't match");
+                return text;
             }
-            return text;
-        } catch (SignatureException e) {
-            throw e;
         } catch (Throwable t) {
-            throw new SignatureException("error while verifying the signed text", t);
+            logger.error("error occurred while verifying signed text", t);
+            throw new SignatureException("error while verifying the signed text");
         }
+
+        throw new SignatureException("tampered signed text");
     }
 
     @Override

src/test/java/org/elasticsearch/shield/signature/InternalSignatureServiceTests.java

@@ -22,6 +22,7 @@ import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 /**
  *
@@ -81,6 +82,72 @@ public class InternalSignatureServiceTests extends ElasticsearchTestCase {
         assertThat(text, equalTo(signed));
     }
 
+    @Test
+    public void testTamperedSignature() throws Exception {
+        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService).start();
+        String text = randomAsciiOfLength(10);
+        String signed = service.sign(text);
+        int i = signed.indexOf("$$", 2);
+        int length = Integer.parseInt(signed.substring(2, i));
+        String fakeSignature = randomAsciiOfLength(length);
+        String fakeSignedText = "$$" + length + "$$" + fakeSignature + signed.substring(i + 2 + length);
+
+        try {
+            service.unsignAndVerify(fakeSignedText);
+        } catch (SignatureException e) {
+            assertThat(e.getMessage(), is(equalTo("tampered signed text")));
+            assertThat(e.getCause(), is(nullValue()));
+        }
+    }
+
+    @Test
+    public void testTamperedSignatureOneChar() throws Exception {
+        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService).start();
+        String text = randomAsciiOfLength(10);
+        String signed = service.sign(text);
+        int i = signed.indexOf("$$", 2);
+        int length = Integer.parseInt(signed.substring(2, i));
+        StringBuilder fakeSignature = new StringBuilder(signed.substring(i + 2, i + 2 + length));
+        fakeSignature.setCharAt(randomIntBetween(0, fakeSignature.length() - 1), randomAsciiOfLength(1).charAt(0));
+
+        String fakeSignedText = "$$" + length + "$$" + fakeSignature.toString() + signed.substring(i + 2 + length);
+
+        try {
+            service.unsignAndVerify(fakeSignedText);
+        } catch (SignatureException e) {
+            assertThat(e.getMessage(), is(equalTo("tampered signed text")));
+            assertThat(e.getCause(), is(nullValue()));
+        }
+    }
+
+    @Test
+    public void testTamperedSignatureLength() throws Exception {
+        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService).start();
+        String text = randomAsciiOfLength(10);
+        String signed = service.sign(text);
+        int i = signed.indexOf("$$", 2);
+        int length = Integer.parseInt(signed.substring(2, i));
+        String fakeSignature = randomAsciiOfLength(length);
+
+        // Smaller sig length
+        String fakeSignedText = "$$" + randomIntBetween(0, length - 1) + "$$" + fakeSignature + signed.substring(i + 2 + length);
+
+        try {
+            service.unsignAndVerify(fakeSignedText);
+        } catch (SignatureException e) {
+            assertThat(e.getMessage(), is(equalTo("tampered signed text")));
+        }
+
+        // Larger sig length
+        fakeSignedText = "$$" + randomIntBetween(length + 1, Integer.MAX_VALUE) + "$$" + fakeSignature + signed.substring(i + 2 + length);
+        try {
+            service.unsignAndVerify(fakeSignedText);
+        } catch (SignatureException e) {
+            assertThat(e.getMessage(), is(equalTo("tampered signed text")));
+            assertThat(e.getCause(), is(nullValue()));
+        }
+    }
+
     @Test
     public void testReloadKey() throws Exception {
         final CountDownLatch latch = new CountDownLatch(1);