Handle binary safe string for REQUIREPASS and MASTERAUTH directives (#8200)

* Handle binary safe string for REQUIREPASS and MASTERAUTH directives.

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.convert_empty_to_null && !data.string.default_value) ? NULL : zstrdup(data.string.default_value);
-        *data.string.config = data.string.default_value ? zstrdup(data.string.default_value) : NULL;
-    } else {
-        *data.string.config = 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 = (data.string.convert_empty_to_null && !value[0]) ? NULL : zstrdup(value);
-        *data.string.config = value[0] ? zstrdup(value) : NULL;
-    } else {
-        *data.string.config = 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),

src/replication.c

@@ -1835,6 +1835,7 @@ error:
  */
 #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;
-
+            if (flags & SYNC_CMD_WRITE_SDS) {
-            cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg);
+                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) {
+        if (server.masterauth) {
-            err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",
+            sds auth = sdsnew("AUTH");
-                                         server.masteruser,server.masterauth,NULL);
+            if (server.masteruser) {
-            if (err) goto write_error;
+                sds masteruser = sdsnew(server.masteruser);
-            server.repl_state = REPL_STATE_RECEIVE_AUTH;
+                err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth,
-            return;
+                                             masteruser, server.masterauth, NULL);
-        } else if (server.masterauth) {
+                sdsfree(masteruser);
-            err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",server.masterauth,NULL);
+            } 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;

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 */

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"
+            }
+        }
+    }
+}