From f48afb4710e6e102b043add55a0649b22ef4a4ab Mon Sep 17 00:00:00 2001 From: Qu Chen Date: Thu, 17 Dec 2020 09:26:33 -0800 Subject: [PATCH] Handle binary safe string for REQUIREPASS and MASTERAUTH directives (#8200) * Handle binary safe string for REQUIREPASS and MASTERAUTH directives. --- src/config.c | 102 +++++++++++++++++++++++++++++++++++++------- src/replication.c | 30 ++++++++----- src/server.h | 2 +- tests/unit/auth.tcl | 41 ++++++++++++++++++ 4 files changed, 148 insertions(+), 27 deletions(-) diff --git a/src/config.c b/src/config.c index c858df3f3b3..4617e867862 100644 --- a/src/config.c +++ b/src/config.c @@ -166,6 +166,15 @@ typedef struct stringConfigData { be stored as a NULL value. */ } stringConfigData; +typedef struct sdsConfigData { + sds *config; /* Pointer to the server config this value is stored in. */ + const char *default_value; /* Default value of the config on rewrite. */ + int (*is_valid_fn)(sds val, char **err); /* Optional function to check validity of new value (generic doc above) */ + int (*update_fn)(sds val, sds prev, char **err); /* Optional function to apply new value at runtime (generic doc above) */ + int convert_empty_to_null; /* Boolean indicating if empty SDS strings should + be stored as a NULL value. */ +} sdsConfigData; + typedef struct enumConfigData { int *config; /* The pointer to the server config this value is stored in */ configEnum *enum_value; /* The underlying enum type this data represents */ @@ -212,6 +221,7 @@ typedef struct numericConfigData { typedef union typeData { boolConfigData yesno; stringConfigData string; + sdsConfigData sds; enumConfigData enumd; numericConfigData numeric; } typeData; @@ -512,7 +522,7 @@ void loadServerConfigFromString(char *config) { } server.repl_state = REPL_STATE_CONNECT; } else if (!strcasecmp(argv[0],"requirepass") && argc == 2) { - if (strlen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) { + if (sdslen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) { err = "Password is longer than CONFIG_AUTHPASS_MAX_LEN"; goto loaderr; } @@ -524,10 +534,10 @@ void loadServerConfigFromString(char *config) { sdsfree(server.requirepass); server.requirepass = NULL; if (sdslen(argv[1])) { - sds aclop = sdscatprintf(sdsempty(),">%s",argv[1]); + sds aclop = sdscatlen(sdsnew(">"), argv[1], sdslen(argv[1])); ACLSetUser(DefaultUser,aclop,sdslen(aclop)); sdsfree(aclop); - server.requirepass = sdsnew(argv[1]); + server.requirepass = sdsdup(argv[1]); } else { ACLSetUser(DefaultUser,"nopass",-1); } @@ -751,10 +761,10 @@ void configSetCommand(client *c) { sdsfree(server.requirepass); server.requirepass = NULL; if (sdslen(o->ptr)) { - sds aclop = sdscatprintf(sdsempty(),">%s",(char*)o->ptr); + sds aclop = sdscatlen(sdsnew(">"), o->ptr, sdslen(o->ptr)); ACLSetUser(DefaultUser,aclop,sdslen(aclop)); sdsfree(aclop); - server.requirepass = sdsnew(o->ptr); + server.requirepass = sdsdup(o->ptr); } else { ACLSetUser(DefaultUser,"nopass",-1); } @@ -1330,6 +1340,28 @@ void rewriteConfigStringOption(struct rewriteConfigState *state, const char *opt rewriteConfigRewriteLine(state,option,line,force); } +/* Rewrite a SDS string option. */ +void rewriteConfigSdsOption(struct rewriteConfigState *state, const char *option, sds value, const sds defvalue) { + int force = 1; + sds line; + + /* If there is no value set, we don't want the SDS option + * to be present in the configuration at all. */ + if (value == NULL) { + rewriteConfigMarkAsProcessed(state, option); + return; + } + + /* Set force to zero if the value is set to its default. */ + if (defvalue && sdscmp(value, defvalue) == 0) force = 0; + + line = sdsnew(option); + line = sdscatlen(line, " ", 1); + line = sdscatrepr(line, value, sdslen(value)); + + rewriteConfigRewriteLine(state, option, line, force); +} + /* Rewrite a numerical (long long range) option. */ void rewriteConfigNumericalOption(struct rewriteConfigState *state, const char *option, long long value, long long defvalue) { int force = value != defvalue; @@ -1802,22 +1834,14 @@ static void boolConfigRewrite(typeData data, const char *name, struct rewriteCon /* String Configs */ static void stringConfigInit(typeData data) { - if (data.string.convert_empty_to_null) { - *data.string.config = data.string.default_value ? zstrdup(data.string.default_value) : NULL; - } else { - *data.string.config = zstrdup(data.string.default_value); - } + *data.string.config = (data.string.convert_empty_to_null && !data.string.default_value) ? NULL : zstrdup(data.string.default_value); } static int stringConfigSet(typeData data, sds value, int update, char **err) { if (data.string.is_valid_fn && !data.string.is_valid_fn(value, err)) return 0; char *prev = *data.string.config; - if (data.string.convert_empty_to_null) { - *data.string.config = value[0] ? zstrdup(value) : NULL; - } else { - *data.string.config = zstrdup(value); - } + *data.string.config = (data.string.convert_empty_to_null && !value[0]) ? NULL : zstrdup(value); if (update && data.string.update_fn && !data.string.update_fn(*data.string.config, prev, err)) { zfree(*data.string.config); *data.string.config = prev; @@ -1835,6 +1859,38 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC rewriteConfigStringOption(state, name,*(data.string.config), data.string.default_value); } +/* SDS Configs */ +static void sdsConfigInit(typeData data) { + *data.sds.config = (data.sds.convert_empty_to_null && !data.sds.default_value) ? NULL: sdsnew(data.sds.default_value); +} + +static int sdsConfigSet(typeData data, sds value, int update, char **err) { + if (data.sds.is_valid_fn && !data.sds.is_valid_fn(value, err)) + return 0; + sds prev = *data.sds.config; + *data.sds.config = (data.sds.convert_empty_to_null && (sdslen(value) == 0)) ? NULL : sdsdup(value); + if (update && data.sds.update_fn && !data.sds.update_fn(*data.sds.config, prev, err)) { + sdsfree(*data.sds.config); + *data.sds.config = prev; + return 0; + } + sdsfree(prev); + return 1; +} + +static void sdsConfigGet(client *c, typeData data) { + if (*data.sds.config) { + addReplyBulkSds(c, sdsdup(*data.sds.config)); + } else { + addReplyBulkCString(c, ""); + } +} + +static void sdsConfigRewrite(typeData data, const char *name, struct rewriteConfigState *state) { + rewriteConfigSdsOption(state, name, *(data.sds.config), data.sds.default_value ? sdsnew(data.sds.default_value) : NULL); +} + + #define ALLOW_EMPTY_STRING 0 #define EMPTY_STRING_IS_NULL 1 @@ -1850,6 +1906,18 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC } \ } +#define createSDSConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, modifiable) \ + embedConfigInterface(sdsConfigInit, sdsConfigSet, sdsConfigGet, sdsConfigRewrite) \ + .data.sds = { \ + .config = &(config_addr), \ + .default_value = (default), \ + .is_valid_fn = (is_valid), \ + .update_fn = (update), \ + .convert_empty_to_null = (empty_to_null), \ + } \ +} + /* Enum configs */ static void enumConfigInit(typeData data) { *data.enumd.config = data.enumd.default_value; @@ -2349,7 +2417,6 @@ standardConfig configs[] = { createStringConfig("pidfile", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.pidfile, NULL, NULL, NULL), createStringConfig("replica-announce-ip", "slave-announce-ip", MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.slave_announce_ip, NULL, NULL, NULL), createStringConfig("masteruser", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masteruser, NULL, NULL, NULL), - createStringConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), createStringConfig("cluster-announce-ip", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_ip, NULL, NULL, NULL), createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL), createStringConfig("dbfilename", NULL, MODIFIABLE_CONFIG, ALLOW_EMPTY_STRING, server.rdb_filename, "dump.rdb", isValidDBfilename, NULL), @@ -2359,6 +2426,9 @@ standardConfig configs[] = { createStringConfig("aof_rewrite_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.aof_rewrite_cpulist, NULL, NULL, NULL), createStringConfig("bgsave_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.bgsave_cpulist, NULL, NULL, NULL), + /* SDS Configs */ + createSDSConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), + /* Enum Configs */ createEnumConfig("supervised", NULL, IMMUTABLE_CONFIG, supervised_mode_enum, server.supervised_mode, SUPERVISED_NONE, NULL, NULL), createEnumConfig("syslog-facility", NULL, IMMUTABLE_CONFIG, syslog_facility_enum, server.syslog_facility, LOG_LOCAL0, NULL, NULL), diff --git a/src/replication.c b/src/replication.c index 64aa41390eb..34ff4408747 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1835,6 +1835,7 @@ void readSyncBulkPayload(connection *conn) { */ #define SYNC_CMD_READ (1<<0) #define SYNC_CMD_WRITE (1<<1) +#define SYNC_CMD_WRITE_SDS (1<<2) #define SYNC_CMD_FULL (SYNC_CMD_READ|SYNC_CMD_WRITE) char *sendSynchronousCommand(int flags, connection *conn, ...) { @@ -1852,8 +1853,13 @@ char *sendSynchronousCommand(int flags, connection *conn, ...) { while(1) { arg = va_arg(ap, char*); if (arg == NULL) break; - - cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); + if (flags & SYNC_CMD_WRITE_SDS) { + cmdargs = sdscatprintf(cmdargs,"$%zu\r\n", sdslen((sds)arg)); + cmdargs = sdscatsds(cmdargs, (sds)arg); + cmdargs = sdscat(cmdargs, "\r\n"); + } else { + cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg); + } argslen++; } @@ -2166,14 +2172,18 @@ void syncWithMaster(connection *conn) { /* AUTH with the master if required. */ if (server.repl_state == REPL_STATE_SEND_AUTH) { - if (server.masteruser && server.masterauth) { - err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH", - server.masteruser,server.masterauth,NULL); - if (err) goto write_error; - server.repl_state = REPL_STATE_RECEIVE_AUTH; - return; - } else if (server.masterauth) { - err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",server.masterauth,NULL); + if (server.masterauth) { + sds auth = sdsnew("AUTH"); + if (server.masteruser) { + sds masteruser = sdsnew(server.masteruser); + err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth, + masteruser, server.masterauth, NULL); + sdsfree(masteruser); + } else { + err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth, + server.masterauth, NULL); + } + sdsfree(auth); if (err) goto write_error; server.repl_state = REPL_STATE_RECEIVE_AUTH; return; diff --git a/src/server.h b/src/server.h index 427d7c2f25f..5c75aed0850 100644 --- a/src/server.h +++ b/src/server.h @@ -1386,7 +1386,7 @@ struct redisServer { int repl_diskless_sync_delay; /* Delay to start a diskless repl BGSAVE. */ /* Replication (slave) */ char *masteruser; /* AUTH with this user and masterauth with master */ - char *masterauth; /* AUTH with this password with master */ + sds masterauth; /* AUTH with this password with master */ char *masterhost; /* Hostname of master */ int masterport; /* Port of master */ int repl_timeout; /* Timeout after N seconds of master idle */ diff --git a/tests/unit/auth.tcl b/tests/unit/auth.tcl index 9080d4bf7e9..b63cf01260d 100644 --- a/tests/unit/auth.tcl +++ b/tests/unit/auth.tcl @@ -25,3 +25,44 @@ start_server {tags {"auth"} overrides {requirepass foobar}} { r incr foo } {101} } + +start_server {tags {"auth_binary_password"}} { + test {AUTH fails when binary password is wrong} { + r config set requirepass "abc\x00def" + catch {r auth abc} err + set _ $err + } {WRONGPASS*} + + test {AUTH succeeds when binary password is correct} { + r config set requirepass "abc\x00def" + r auth "abc\x00def" + } {OK} + + start_server {tags {"masterauth"}} { + set master [srv -1 client] + set master_host [srv -1 host] + set master_port [srv -1 port] + set slave [srv 0 client] + + test {MASTERAUTH test with binary password} { + $master config set requirepass "abc\x00def" + + # Configure the replica with masterauth + set loglines [count_log_lines 0] + $slave slaveof $master_host $master_port + $slave config set masterauth "abc" + + # Verify replica is not able to sync with master + wait_for_log_messages 0 {"*Unable to AUTH to MASTER*"} $loglines 1000 10 + assert_equal {down} [s 0 master_link_status] + + # Test replica with the correct masterauth + $slave config set masterauth "abc\x00def" + wait_for_condition 50 100 { + [s 0 master_link_status] eq {up} + } else { + fail "Can't turn the instance into a replica" + } + } + } +}