Compare HMACs with MessageDigest to avoid timing attacks (#1054)

* Compare HMACs with MessageDigest to avoid timing attacks * Make static method
square · Apr 14, 2022 · 1a35a2e · 1a35a2e
1 parent 8fc9cd8
commit 1a35a2e
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 4 deletions.
diff --git a/server/src/main/java/keywhiz/service/crypto/RowHmacGenerator.java b/server/src/main/java/keywhiz/service/crypto/RowHmacGenerator.java
@@ -1,6 +1,7 @@
 package keywhiz.service.crypto;
 
 import java.nio.ByteBuffer;
+import java.security.MessageDigest;
 import java.security.SecureRandom;
 import java.util.List;
 import java.util.Objects;
@@ -36,6 +37,18 @@ public String computeRowHmac(String table, List<Object> fields) {
     return cryptographer.computeHmacWithSecretKey(hmacContent.getBytes(UTF_8), hmacKey);
   }
 
+  /**
+   * Checks whether two HMACs are equal.
+   *
+   * Uses MessageDigest to prevent timing attacks.
+   * */
+  public static boolean compareHmacs(String left, String right) {
+    return MessageDigest.isEqual(
+        left.getBytes(UTF_8),
+        right.getBytes(UTF_8)
+    );
+  }
+
   /**
    * The random long generated with random.nextLong only uses 48 bits of randomness,
    * meaning it will not return all possible long values. Instead we generate a long from 8

diff --git a/server/src/main/java/keywhiz/service/daos/AclDAO.java b/server/src/main/java/keywhiz/service/daos/AclDAO.java
@@ -370,7 +370,7 @@ private SanitizedSecret processSanitizedSecretRow(Record row, Client client) {
     String secretHmac = rowHmacGenerator.computeRowHmac(
         SECRETS.getName(), List.of(row.getValue(SECRETS.NAME), row.getValue(SECRETS.ID))
     );
-    if (!secretHmac.equals(row.getValue(SECRETS.ROW_HMAC))) {
+    if (!RowHmacGenerator.compareHmacs(secretHmac, row.getValue(SECRETS.ROW_HMAC))) {
       String errorMessage = String.format(
           "Secret HMAC verification failed for secret: %s", row.getValue(SECRETS.NAME));
       if (rowHmacLog) {
@@ -384,7 +384,7 @@ private SanitizedSecret processSanitizedSecretRow(Record row, Client client) {
     String clientHmac = rowHmacGenerator.computeRowHmac(
         CLIENTS.getName(), List.of(client.getName(), client.getId())
     );
-    if (!clientHmac.equals(row.getValue(CLIENTS.ROW_HMAC))) {
+    if (!RowHmacGenerator.compareHmacs(clientHmac, row.getValue(CLIENTS.ROW_HMAC))) {
       String errorMessage = String.format(
           "Client HMAC verification failed for client: %s", client.getName());
       if (rowHmacLog) {
@@ -397,7 +397,7 @@ private SanitizedSecret processSanitizedSecretRow(Record row, Client client) {
 
     String membershipsHmac = rowHmacGenerator.computeRowHmac(
         MEMBERSHIPS.getName(), List.of(client.getId(), row.getValue(MEMBERSHIPS.GROUPID)));
-    if (!membershipsHmac.equals(row.getValue(MEMBERSHIPS.ROW_HMAC))) {
+    if (!RowHmacGenerator.compareHmacs(membershipsHmac, row.getValue(MEMBERSHIPS.ROW_HMAC))) {
       String errorMessage = String.format(
           "Memberships HMAC verification failed for clientId: %d in groupId: %d",
           client.getId(), row.getValue(MEMBERSHIPS.GROUPID));
@@ -412,7 +412,7 @@ private SanitizedSecret processSanitizedSecretRow(Record row, Client client) {
     String accessgrantsHmac = rowHmacGenerator.computeRowHmac(
         ACCESSGRANTS.getName(),
         List.of(row.getValue(MEMBERSHIPS.GROUPID), row.getValue(SECRETS.ID)));
-    if (!accessgrantsHmac.equals(row.getValue(ACCESSGRANTS.ROW_HMAC))) {
+    if (!RowHmacGenerator.compareHmacs(accessgrantsHmac, row.getValue(ACCESSGRANTS.ROW_HMAC))) {
       String errorMessage = String.format(
           "Access Grants HMAC verification failed for groupId: %d in secretId: %d",
           row.getValue(MEMBERSHIPS.GROUPID), row.getValue(SECRETS.ID));