From 2db0d898f8d9daef34a6b9678a905a51cc43c298 Mon Sep 17 00:00:00 2001 From: bugwz Date: Tue, 5 Apr 2022 13:51:51 +0800 Subject: [PATCH] Cluster node name sanity check (#10391) * Limit cluster node id length for CLUSTER commands loading * Cluster node name sanity check for length and values Co-authored-by: Madelyn Olson --- src/cluster.c | 78 +++++++++++++++++++++++++++++++++------------------ src/cluster.h | 3 +- src/module.c | 7 +++-- 3 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 6b5fb8d1a..701871b36 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -56,7 +56,6 @@ void clusterSendFailoverAuthIfNeeded(clusterNode *node, clusterMsg *request); void clusterUpdateState(void); int clusterNodeGetSlotBit(clusterNode *n, int slot); sds clusterGenNodesDescription(int filter, int use_pport); -clusterNode *clusterLookupNode(const char *name); list *clusterGetNodesServingMySlots(clusterNode *node); int clusterNodeAddSlave(clusterNode *master, clusterNode *slave); int clusterAddSlot(clusterNode *n, int slot); @@ -212,7 +211,11 @@ int clusterLoadConfig(char *filename) { } /* Create this node if it does not exist */ - n = clusterLookupNode(argv[0]); + if (verifyClusterNodeId(argv[0], sdslen(argv[0])) == C_ERR) { + sdsfreesplitres(argv, argc); + goto fmterr; + } + n = clusterLookupNode(argv[0], sdslen(argv[0])); if (!n) { n = createClusterNode(argv[0],0); clusterAddNode(n); @@ -288,7 +291,11 @@ int clusterLoadConfig(char *filename) { /* Get master if any. Set the master and populate master's * slave list. */ if (argv[3][0] != '-') { - master = clusterLookupNode(argv[3]); + if (verifyClusterNodeId(argv[3], sdslen(argv[3])) == C_ERR) { + sdsfreesplitres(argv, argc); + goto fmterr; + } + master = clusterLookupNode(argv[3], sdslen(argv[3])); if (!master) { master = createClusterNode(argv[3],0); clusterAddNode(master); @@ -324,7 +331,14 @@ int clusterLoadConfig(char *filename) { goto fmterr; } p += 3; - cn = clusterLookupNode(p); + + char *pr = strchr(p, ']'); + size_t node_len = pr - p; + if (pr == NULL || verifyClusterNodeId(p, node_len) == C_ERR) { + sdsfreesplitres(argv, argc); + goto fmterr; + } + cn = clusterLookupNode(p, CLUSTER_NAMELEN); if (!cn) { cn = createClusterNode(p,0); clusterAddNode(cn); @@ -1182,12 +1196,23 @@ void clusterDelNode(clusterNode *delnode) { freeClusterNode(delnode); } -/* Node lookup by name */ -clusterNode *clusterLookupNode(const char *name) { - sds s = sdsnewlen(name, CLUSTER_NAMELEN); - dictEntry *de; +/* Cluster node sanity check. Returns C_OK if the node id + * is valid an C_ERR otherwise. */ +int verifyClusterNodeId(const char *name, int length) { + if (length != CLUSTER_NAMELEN) return C_ERR; + for (int i = 0; i < length; i++) { + if (name[i] >= 'a' && name[i] <= 'z') continue; + if (name[i] >= '0' && name[i] <= '9') continue; + return C_ERR; + } + return C_OK; +} - de = dictFind(server.cluster->nodes,s); +/* Node lookup by name */ +clusterNode *clusterLookupNode(const char *name, int length) { + if (verifyClusterNodeId(name, length) != C_OK) return NULL; + sds s = sdsnewlen(name, length); + dictEntry *de = dictFind(server.cluster->nodes, s); sdsfree(s); if (de == NULL) return NULL; return dictGetVal(de); @@ -1603,7 +1628,7 @@ int clusterStartHandshake(char *ip, int port, int cport) { void clusterProcessGossipSection(clusterMsg *hdr, clusterLink *link) { uint16_t count = ntohs(hdr->count); clusterMsgDataGossip *g = (clusterMsgDataGossip*) hdr->data.ping.gossip; - clusterNode *sender = link->node ? link->node : clusterLookupNode(hdr->sender); + clusterNode *sender = link->node ? link->node : clusterLookupNode(hdr->sender, CLUSTER_NAMELEN); while(count--) { uint16_t flags = ntohs(g->flags); @@ -1622,7 +1647,7 @@ void clusterProcessGossipSection(clusterMsg *hdr, clusterLink *link) { } /* Update our state accordingly to the gossip sections */ - node = clusterLookupNode(g->nodename); + node = clusterLookupNode(g->nodename, CLUSTER_NAMELEN); if (node) { /* We already know this node. Handle failure reports, only when the sender is a master. */ @@ -1985,7 +2010,7 @@ int writeHostnamePingExt(clusterMsgPingExt **cursor) { /* We previously validated the extensions, so this function just needs to * handle the extensions. */ void clusterProcessPingExtensions(clusterMsg *hdr, clusterLink *link) { - clusterNode *sender = link->node ? link->node : clusterLookupNode(hdr->sender); + clusterNode *sender = link->node ? link->node : clusterLookupNode(hdr->sender, CLUSTER_NAMELEN); char *ext_hostname = NULL; uint16_t extensions = ntohs(hdr->extensions); /* Loop through all the extensions and process them */ @@ -2018,7 +2043,7 @@ static clusterNode *getNodeFromLinkAndMsg(clusterLink *link, clusterMsg *hdr) { sender = link->node; } else { /* Otherwise, fetch sender based on the message */ - sender = clusterLookupNode(hdr->sender); + sender = clusterLookupNode(hdr->sender, CLUSTER_NAMELEN); /* We know the sender node but haven't associate it with the link. This must * be an inbound link because only for inbound links we didn't know which node * to associate when they were created. */ @@ -2329,7 +2354,7 @@ int clusterProcessPacket(clusterLink *link) { clusterSetNodeAsMaster(sender); } else { /* Node is a slave. */ - clusterNode *master = clusterLookupNode(hdr->slaveof); + clusterNode *master = clusterLookupNode(hdr->slaveof, CLUSTER_NAMELEN); if (nodeIsMaster(sender)) { /* Master turned into a slave! Reconfigure the node. */ @@ -2444,7 +2469,7 @@ int clusterProcessPacket(clusterLink *link) { clusterNode *failing; if (sender) { - failing = clusterLookupNode(hdr->data.fail.about.nodename); + failing = clusterLookupNode(hdr->data.fail.about.nodename, CLUSTER_NAMELEN); if (failing && !(failing->flags & (CLUSTER_NODE_FAIL|CLUSTER_NODE_MYSELF))) { @@ -2532,7 +2557,7 @@ int clusterProcessPacket(clusterLink *link) { ntohu64(hdr->data.update.nodecfg.configEpoch); if (!sender) return 1; /* We don't know the sender. */ - n = clusterLookupNode(hdr->data.update.nodecfg.nodename); + n = clusterLookupNode(hdr->data.update.nodecfg.nodename, CLUSTER_NAMELEN); if (!n) return 1; /* We don't know the reported node. */ if (n->configEpoch >= reportedConfigEpoch) return 1; /* Nothing new. */ @@ -3163,7 +3188,7 @@ int clusterSendModuleMessageToTarget(const char *target, uint64_t module_id, uin clusterNode *node = NULL; if (target != NULL) { - node = clusterLookupNode(target); + node = clusterLookupNode(target, strlen(target)); if (node == NULL || node->link == NULL) return C_ERR; } @@ -5354,7 +5379,8 @@ NULL addReplyErrorFormat(c,"I'm not the owner of hash slot %u",slot); return; } - if ((n = clusterLookupNode(c->argv[4]->ptr)) == NULL) { + n = clusterLookupNode(c->argv[4]->ptr, sdslen(c->argv[4]->ptr)); + if (n == NULL) { addReplyErrorFormat(c,"I don't know about node %s", (char*)c->argv[4]->ptr); return; @@ -5370,7 +5396,8 @@ NULL "I'm already the owner of hash slot %u",slot); return; } - if ((n = clusterLookupNode(c->argv[4]->ptr)) == NULL) { + n = clusterLookupNode(c->argv[4]->ptr, sdslen(c->argv[4]->ptr)); + if (n == NULL) { addReplyErrorFormat(c,"I don't know about node %s", (char*)c->argv[4]->ptr); return; @@ -5386,8 +5413,7 @@ NULL server.cluster->migrating_slots_to[slot] = NULL; } else if (!strcasecmp(c->argv[3]->ptr,"node") && c->argc == 5) { /* CLUSTER SETSLOT NODE */ - clusterNode *n = clusterLookupNode(c->argv[4]->ptr); - + n = clusterLookupNode(c->argv[4]->ptr, sdslen(c->argv[4]->ptr)); if (!n) { addReplyErrorFormat(c,"Unknown node %s", (char*)c->argv[4]->ptr); @@ -5599,8 +5625,7 @@ NULL } } else if (!strcasecmp(c->argv[1]->ptr,"forget") && c->argc == 3) { /* CLUSTER FORGET */ - clusterNode *n = clusterLookupNode(c->argv[2]->ptr); - + clusterNode *n = clusterLookupNode(c->argv[2]->ptr, sdslen(c->argv[2]->ptr)); if (!n) { addReplyErrorFormat(c,"Unknown node %s", (char*)c->argv[2]->ptr); return; @@ -5618,9 +5643,8 @@ NULL addReply(c,shared.ok); } else if (!strcasecmp(c->argv[1]->ptr,"replicate") && c->argc == 3) { /* CLUSTER REPLICATE */ - clusterNode *n = clusterLookupNode(c->argv[2]->ptr); - /* Lookup the specified node in our table. */ + clusterNode *n = clusterLookupNode(c->argv[2]->ptr, sdslen(c->argv[2]->ptr)); if (!n) { addReplyErrorFormat(c,"Unknown node %s", (char*)c->argv[2]->ptr); return; @@ -5656,7 +5680,7 @@ NULL } else if ((!strcasecmp(c->argv[1]->ptr,"slaves") || !strcasecmp(c->argv[1]->ptr,"replicas")) && c->argc == 3) { /* CLUSTER SLAVES */ - clusterNode *n = clusterLookupNode(c->argv[2]->ptr); + clusterNode *n = clusterLookupNode(c->argv[2]->ptr, sdslen(c->argv[2]->ptr)); int j; /* Lookup the specified node in our table. */ @@ -5683,7 +5707,7 @@ NULL c->argc == 3) { /* CLUSTER COUNT-FAILURE-REPORTS */ - clusterNode *n = clusterLookupNode(c->argv[2]->ptr); + clusterNode *n = clusterLookupNode(c->argv[2]->ptr, sdslen(c->argv[2]->ptr)); if (!n) { addReplyErrorFormat(c,"Unknown node %s", (char*)c->argv[2]->ptr); diff --git a/src/cluster.h b/src/cluster.h index d233c947c..27e9e7770 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -377,7 +377,8 @@ void clusterInit(void); void clusterCron(void); void clusterBeforeSleep(void); clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); -clusterNode *clusterLookupNode(const char *name); +int verifyClusterNodeId(const char *name, int length); +clusterNode *clusterLookupNode(const char *name, int length); int clusterRedirectBlockedClientIfNeeded(client *c); void clusterRedirectClient(client *c, clusterNode *n, int hashslot, int error_code); void migrateCloseTimedoutSockets(void); diff --git a/src/module.c b/src/module.c index cc34c5bc8..3fc6a5499 100644 --- a/src/module.c +++ b/src/module.c @@ -7951,8 +7951,9 @@ size_t RM_GetClusterSize(void) { } /* Populate the specified info for the node having as ID the specified 'id', - * then returns REDISMODULE_OK. Otherwise if the node ID does not exist from - * the POV of this local node, REDISMODULE_ERR is returned. + * then returns REDISMODULE_OK. Otherwise if the format of node ID is invalid + * or the node ID does not exist from the POV of this local node, REDISMODULE_ERR + * is returned. * * The arguments `ip`, `master_id`, `port` and `flags` can be NULL in case we don't * need to populate back certain info. If an `ip` and `master_id` (only populated @@ -7972,7 +7973,7 @@ size_t RM_GetClusterSize(void) { int RM_GetClusterNodeInfo(RedisModuleCtx *ctx, const char *id, char *ip, char *master_id, int *port, int *flags) { UNUSED(ctx); - clusterNode *node = clusterLookupNode(id); + clusterNode *node = clusterLookupNode(id, strlen(id)); if (node == NULL || node->flags & (CLUSTER_NODE_NOADDR|CLUSTER_NODE_HANDSHAKE)) {