From 4ad166235efe0fc2662e5e91b3b3429b51ce4862 Mon Sep 17 00:00:00 2001
From: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Date: Fri, 3 Jun 2022 09:30:28 -0700
Subject: [PATCH] Update time independent string compare to use hash length
 (#9759)

* Update time independent string compare to use hash length
---
 src/acl.c    | 41 ++++++-----------------------------------
 src/server.h |  1 -
 2 files changed, 6 insertions(+), 36 deletions(-)

diff --git a/src/acl.c b/src/acl.c
index 0054402b3..3443ee691 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -144,7 +144,7 @@ void ACLFreeLogEntry(void *le);
 int ACLSetSelector(aclSelector *selector, const char *op, size_t oplen);
 
 /* The length of the string representation of a hashed password. */
-#define HASH_PASSWORD_LEN SHA256_BLOCK_SIZE*2
+#define HASH_PASSWORD_LEN (SHA256_BLOCK_SIZE*2)
 
 /* =============================================================================
  * Helper functions for the rest of the ACL implementation
@@ -153,42 +153,13 @@ int ACLSetSelector(aclSelector *selector, const char *op, size_t oplen);
 /* Return zero if strings are the same, non-zero if they are not.
  * The comparison is performed in a way that prevents an attacker to obtain
  * information about the nature of the strings just monitoring the execution
- * time of the function.
- *
- * Note that limiting the comparison length to strings up to 512 bytes we
- * can avoid leaking any information about the password length and any
- * possible branch misprediction related leak.
+ * time of the function. Note: The two strings must be the same length.
  */
-int time_independent_strcmp(char *a, char *b) {
-    char bufa[CONFIG_AUTHPASS_MAX_LEN], bufb[CONFIG_AUTHPASS_MAX_LEN];
-    /* The above two strlen perform len(a) + len(b) operations where either
-     * a or b are fixed (our password) length, and the difference is only
-     * relative to the length of the user provided string, so no information
-     * leak is possible in the following two lines of code. */
-    unsigned int alen = strlen(a);
-    unsigned int blen = strlen(b);
-    unsigned int j;
+int time_independent_strcmp(char *a, char *b, int len) {
     int diff = 0;
-
-    /* We can't compare strings longer than our static buffers.
-     * Note that this will never pass the first test in practical circumstances
-     * so there is no info leak. */
-    if (alen > sizeof(bufa) || blen > sizeof(bufb)) return 1;
-
-    memset(bufa,0,sizeof(bufa));        /* Constant time. */
-    memset(bufb,0,sizeof(bufb));        /* Constant time. */
-    /* Again the time of the following two copies is proportional to
-     * len(a) + len(b) so no info is leaked. */
-    memcpy(bufa,a,alen);
-    memcpy(bufb,b,blen);
-
-    /* Always compare all the chars in the two buffers without
-     * conditional expressions. */
-    for (j = 0; j < sizeof(bufa); j++) {
-        diff |= (bufa[j] ^ bufb[j]);
+    for (int j = 0; j < len; j++) {
+        diff |= (a[j] ^ b[j]);
     }
-    /* Length must be equal as well. */
-    diff |= alen ^ blen;
     return diff; /* If zero strings are the same. */
 }
 
@@ -1414,7 +1385,7 @@ int ACLCheckUserCredentials(robj *username, robj *password) {
     sds hashed = ACLHashPassword(password->ptr,sdslen(password->ptr));
     while((ln = listNext(&li))) {
         sds thispass = listNodeValue(ln);
-        if (!time_independent_strcmp(hashed, thispass)) {
+        if (!time_independent_strcmp(hashed, thispass, HASH_PASSWORD_LEN)) {
             sdsfree(hashed);
             return C_OK;
         }
diff --git a/src/server.h b/src/server.h
index 0fd9de028..04c047cf0 100644
--- a/src/server.h
+++ b/src/server.h
@@ -114,7 +114,6 @@ typedef long long ustime_t; /* microsecond time type. */
 #define LOG_MAX_LEN    1024 /* Default maximum length of syslog messages.*/
 #define AOF_REWRITE_ITEMS_PER_CMD 64
 #define AOF_ANNOTATION_LINE_MAX_LEN 1024
-#define CONFIG_AUTHPASS_MAX_LEN 512
 #define CONFIG_RUN_ID_SIZE 40
 #define RDB_EOF_MARK_SIZE 40
 #define CONFIG_REPL_BACKLOG_MIN_SIZE (1024*16)          /* 16k */