diff options
author | Madelyn Olson <34459052+madolson@users.noreply.github.com> | 2022-06-03 09:30:28 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-06-03 09:30:28 -0700 |
commit | 4ad166235efe0fc2662e5e91b3b3429b51ce4862 (patch) | |
tree | 5f278ab1770673a4783d18e8d8613b623184a181 | |
parent | a18c91d6423777dffd6afca80d32bf1964f449e6 (diff) | |
download | redis-4ad166235efe0fc2662e5e91b3b3429b51ce4862.tar.gz |
Update time independent string compare to use hash length (#9759)
* Update time independent string compare to use hash length
-rw-r--r-- | src/acl.c | 43 | ||||
-rw-r--r-- | src/server.h | 1 |
2 files changed, 7 insertions, 37 deletions
@@ -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]); - } - /* Length must be equal as well. */ - diff |= alen ^ blen; + for (int j = 0; j < len; j++) { + diff |= (a[j] ^ b[j]); + } 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 */ |