diff --git a/src/config.c b/src/config.c index 6f98e5e5a..cf6140084 100644 --- a/src/config.c +++ b/src/config.c @@ -264,6 +264,10 @@ void loadServerConfigFromString(char *config) { { server.aof_rewrite_min_size = memtoll(argv[1],NULL); } else if (!strcasecmp(argv[0],"requirepass") && argc == 2) { + if (strlen(argv[1]) > REDIS_AUTHPASS_MAX_LEN) { + err = "Password is longer than REDIS_AUTHPASS_MAX_LEN"; + goto loaderr; + } server.requirepass = zstrdup(argv[1]); } else if (!strcasecmp(argv[0],"pidfile") && argc == 2) { zfree(server.pidfile); @@ -418,6 +422,7 @@ void configSetCommand(redisClient *c) { zfree(server.rdb_filename); server.rdb_filename = zstrdup(o->ptr); } else if (!strcasecmp(c->argv[2]->ptr,"requirepass")) { + if (sdslen(o->ptr) > REDIS_AUTHPASS_MAX_LEN) goto badfmt; zfree(server.requirepass); server.requirepass = ((char*)o->ptr)[0] ? zstrdup(o->ptr) : NULL; } else if (!strcasecmp(c->argv[2]->ptr,"masterauth")) { diff --git a/src/redis.c b/src/redis.c index b1f83386d..0e80b3e72 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1742,10 +1742,52 @@ int prepareForShutdown(int flags) { /*================================== Commands =============================== */ +/* Return 0 if strings are the same, 1 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. + */ +int time_independent_strcmp(char *a, char *b) { + char bufa[REDIS_AUTHPASS_MAX_LEN], bufb[REDIS_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. */ + int alen = strlen(a); + int blen = strlen(b); + int j; + 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; + return diff; /* If zero strings are the same. */ +} + void authCommand(redisClient *c) { if (!server.requirepass) { addReplyError(c,"Client sent AUTH, but no password is set"); - } else if (!strcmp(c->argv[1]->ptr, server.requirepass)) { + } else if (!time_independent_strcmp(c->argv[1]->ptr, server.requirepass)) { c->authenticated = 1; addReply(c,shared.ok); } else { diff --git a/src/redis.h b/src/redis.h index 4ce5311b9..981c6a5fe 100644 --- a/src/redis.h +++ b/src/redis.h @@ -56,6 +56,7 @@ #define REDIS_SLOWLOG_LOG_SLOWER_THAN 10000 #define REDIS_SLOWLOG_MAX_LEN 128 #define REDIS_MAX_CLIENTS 10000 +#define REDIS_AUTHPASS_MAX_LEN 512 #define REDIS_REPL_TIMEOUT 60 #define REDIS_REPL_PING_SLAVE_PERIOD 10