From 545b45928220c1a3b02ed9810d60c1af8b168666 Mon Sep 17 00:00:00 2001 From: Andres Tobar Date: Thu, 2 Jul 2026 14:09:20 -0400 Subject: [PATCH 1/2] Fix crash and memory-safety bugs in Redis xbit storage path All in src/redis.c, all standard hiredis-usage errors. Verified against a live Redis 7 with AUTH and against a fault-injection listener that accepts TCP connections and immediately drops them (simulating the production Redis connectivity flaps): * NULL-check the AUTH reply in Redis_Reader_Connect() and Redis_Writer_Connect(). redisCommand() returns NULL when the connection drops mid-command, so a connectivity flap during (re)auth dereferenced NULL and crashed the engine. Reproduced: stock build segfaults (SIGSEGV) on the first dropped connection; patched build survives. The retry is a loop with the existing 2-second backoff (8 attempts in 15s), not recursion, so no stack growth and no connection hammering. A non-NULL reply that is not OK still aborts (genuinely wrong password), as before. * Free the AUTH reply objects (previously leaked on every reconnect), free-and-NULL failed writer contexts (leaked per retry), and NULL the reader context after redisFree() so the retry-loop condition does not read freed memory. * Redis_Reader(): split the command on spaces and use redisCommandArgv() instead of passing the caller's command string as the printf-style format. A '%' in a key or stored payload was interpreted as a printf conversion (undefined behavior/crash). Likewise copy replies with a "%s" format instead of using the reply as the format string. * Redis_Reader(): drop the str[reply->len] = '\0' writes. reply->len is bounded by message_buffer_size * 2 (writer side) while str is message_buffer_size (reader side), so large correlation payloads wrote past the end of the destination buffer. snprintf() already bounds and NUL-terminates. Also guard element[0]->str before use. End-to-end verification with xbit rules (set + isset) against live Redis: the xbit is stored with the setting event's JSON and TTL, the isset GET retrieves it, and the correlated rule fires with the original (setting) event as its correlation data. Context: xbit storage was rolled back fleet-wide from redis to mmap on 2026-06-25..29 because of these crashes, which degraded correlated alerts (self-referential correlation blocks, xbits lost on restart). This patch is intended to make redis xbit storage safe to re-enable. Needs a canary build/deploy on one sensor before fleet rollout. --- src/redis.c | 236 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 171 insertions(+), 65 deletions(-) diff --git a/src/redis.c b/src/redis.c index 0f9f4a9..235f123 100644 --- a/src/redis.c +++ b/src/redis.c @@ -104,59 +104,95 @@ void Redis_Reader_Connect ( void ) { redisReply *reply; + bool authenticated = false; - config->c_reader_redis = NULL; - - while ( config->c_reader_redis == NULL || config->c_reader_redis->err ) + while ( authenticated == false ) { - struct timeval timeout = { 1, 500000 }; // 5.5 seconds - config->c_reader_redis = redisConnectWithTimeout(config->redis_server, config->redis_port, timeout); + config->c_reader_redis = NULL; - if (config->c_reader_redis == NULL || config->c_reader_redis->err) + while ( config->c_reader_redis == NULL || config->c_reader_redis->err ) { - if (config->c_reader_redis) - { - redisFree(config->c_reader_redis); - Sagan_Log(WARN, "[%s, line %d] Redis 'reader' connection error! Sleeping for 2 seconds!", __FILE__, __LINE__); + struct timeval timeout = { 1, 500000 }; // 5.5 seconds + config->c_reader_redis = redisConnectWithTimeout(config->redis_server, config->redis_port, timeout); - } - else + if (config->c_reader_redis == NULL || config->c_reader_redis->err) { - Sagan_Log(WARN, "[%s, line %d] Redis 'reader' connection error - Can't allocate Redis context", __FILE__, __LINE__); + + if (config->c_reader_redis) + { + + /* NULL the context after freeing so the loop + condition doesn't read freed memory. */ + + redisFree(config->c_reader_redis); + config->c_reader_redis = NULL; + Sagan_Log(WARN, "[%s, line %d] Redis 'reader' connection error! Sleeping for 2 seconds!", __FILE__, __LINE__); + + } + else + { + Sagan_Log(WARN, "[%s, line %d] Redis 'reader' connection error - Can't allocate Redis context", __FILE__, __LINE__); + } + sleep(2); } - sleep(2); } - } - /******************/ - /* Log into Redis */ - /******************/ + /******************/ + /* Log into Redis */ + /******************/ - if ( config->redis_password[0] != '\0' ) - { + if ( config->redis_password[0] != '\0' ) + { - reply = redisCommand(config->c_reader_redis, "AUTH %s", config->redis_password); + reply = redisCommand(config->c_reader_redis, "AUTH %s", config->redis_password); - if (!strcmp(reply->str, "OK")) - { + if ( reply == NULL ) + { - if ( debug->debugredis ) + /* redisCommand() returns NULL when the connection drops + mid-AUTH, which is exactly what happens during Redis + connectivity problems. Dereferencing reply->str here was + the crash. Free the context, back off, and retry the + whole connect/auth sequence. */ + + redisFree(config->c_reader_redis); + config->c_reader_redis = NULL; + Sagan_Log(WARN, "[%s, line %d] Redis 'reader' disconnected during AUTH! Sleeping for 2 seconds!", __FILE__, __LINE__); + sleep(2); + continue; + + } + + if ( reply->str != NULL && !strcmp(reply->str, "OK")) { - Sagan_Log( DEBUG, "Authentication success for 'reader' to Redis server at %s:%d (pthread ID: %lu).", config->redis_server, config->redis_port, pthread_self() ); + if ( debug->debugredis ) + { + + Sagan_Log( DEBUG, "Authentication success for 'reader' to Redis server at %s:%d (pthread ID: %lu).", config->redis_server, config->redis_port, pthread_self() ); + + } + + freeReplyObject(reply); } + else + { - } - else - { + /* A real (non-NULL) reply that isn't "OK" means the + password is wrong. That's fatal, as before. */ - Remove_Lock_File(); - Sagan_Log(ERROR, "Authentication failure for 'reader' to to Redis server at %s:%d (pthread ID: %lu). Abort!", config->redis_server, config->redis_port, pthread_self() ); + freeReplyObject(reply); + Remove_Lock_File(); + Sagan_Log(ERROR, "Authentication failure for 'reader' to to Redis server at %s:%d (pthread ID: %lu). Abort!", config->redis_server, config->redis_port, pthread_self() ); + } } + + authenticated = true; + } pthread_mutex_lock(&RedisErrorMutex); @@ -174,63 +210,98 @@ void Redis_Writer_Connect(void) { redisReply *reply; + bool authenticated = false; - c_writer_redis = NULL; - - while ( c_writer_redis == NULL || c_writer_redis->err ) + while ( authenticated == false ) { - struct timeval timeout = { 5, 500000 }; // 5.5 seconds - c_writer_redis = redisConnectWithTimeout(config->redis_server, config->redis_port, timeout); + c_writer_redis = NULL; - if (c_writer_redis == NULL || c_writer_redis->err) + while ( c_writer_redis == NULL || c_writer_redis->err ) { - if (c_writer_redis) + struct timeval timeout = { 5, 500000 }; // 5.5 seconds + c_writer_redis = redisConnectWithTimeout(config->redis_server, config->redis_port, timeout); + + if (c_writer_redis == NULL || c_writer_redis->err) { - Sagan_Log(WARN, "[%s, line %d] Redis 'writer' connection error! Sleeping for 2 seconds.", __FILE__, __LINE__); + if (c_writer_redis) + { - } - else - { + /* Free the failed context (was leaked on every + retry) and NULL it so the loop condition doesn't + read freed memory. */ + + redisFree(c_writer_redis); + c_writer_redis = NULL; + Sagan_Log(WARN, "[%s, line %d] Redis 'writer' connection error! Sleeping for 2 seconds.", __FILE__, __LINE__); + + } + else + { - Sagan_Log(ERROR, "[%s, line %d] Redis 'writer' connection error - Can't allocate Redis context.", __FILE__, __LINE__); + Sagan_Log(ERROR, "[%s, line %d] Redis 'writer' connection error - Can't allocate Redis context.", __FILE__, __LINE__); + } + + sleep(2); } - sleep(2); } - } + /******************/ + /* Log into Redis */ + /******************/ - /******************/ - /* Log into Redis */ - /******************/ + if ( config->redis_password[0] != '\0' ) + { - if ( config->redis_password[0] != '\0' ) - { + reply = redisCommand(c_writer_redis, "AUTH %s", config->redis_password); - reply = redisCommand(c_writer_redis, "AUTH %s", config->redis_password); + if ( reply == NULL ) + { - if (!strcmp(reply->str, "OK")) - { + /* Connection dropped during AUTH. Free the context, back + off, and retry rather than dereferencing a NULL reply + (crash under Redis connectivity loss). */ - if ( debug->debugredis ) + redisFree(c_writer_redis); + c_writer_redis = NULL; + Sagan_Log(WARN, "[%s, line %d] Redis 'writer' disconnected during AUTH! Sleeping for 2 seconds!", __FILE__, __LINE__); + sleep(2); + continue; + + } + + if ( reply->str != NULL && !strcmp(reply->str, "OK")) { - Sagan_Log( DEBUG, "Authentication success for 'writer' to Redis server at %s:%d (pthread ID: %lu).", config->redis_server, config->redis_port, pthread_self() ); + if ( debug->debugredis ) + { + + Sagan_Log( DEBUG, "Authentication success for 'writer' to Redis server at %s:%d (pthread ID: %lu).", config->redis_server, config->redis_port, pthread_self() ); + + } + + freeReplyObject(reply); } + else + { - } - else - { + /* A real (non-NULL) reply that isn't "OK" means the + password is wrong. That's fatal, as before. */ - Remove_Lock_File(); - Sagan_Log(ERROR, "Authentication failure for 'writer' to to Redis server at %s:%d (pthread ID: %lu). Abort!", config->redis_server, config->redis_port, pthread_self() ); + freeReplyObject(reply); + Remove_Lock_File(); + Sagan_Log(ERROR, "Authentication failure for 'writer' to to Redis server at %s:%d (pthread ID: %lu). Abort!", config->redis_server, config->redis_port, pthread_self() ); + } } + + authenticated = true; + } pthread_mutex_lock(&RedisErrorMutex); @@ -365,6 +436,13 @@ void Redis_Reader ( const char *redis_command, char *str, size_t size ) redisReply *reply; + char command_copy[512] = { 0 }; + const char *argv[8] = { NULL }; + size_t argvlen[8] = { 0 }; + int argc = 0; + char *token = NULL; + char *sp = NULL; + if ( connection_read_error == true ) { Sagan_Log(WARN, "[%s, line %d] Redis is an error state. Cannot write.", __FILE__, __LINE__); @@ -373,8 +451,33 @@ void Redis_Reader ( const char *redis_command, char *str, size_t size ) } else { + + /* Split the command on spaces and use redisCommandArgv() so the + command string is never run through printf-style formatting. + The old code passed it as the format string, so a '%' inside + a key (or stored data echoed into a command) was interpreted + as a printf conversion - undefined behavior/crash. */ + + strlcpy(command_copy, redis_command, sizeof(command_copy)); + + token = strtok_r(command_copy, " ", &sp); + + while ( token != NULL && argc < 8 ) + { + argv[argc] = token; + argvlen[argc] = strlen(token); + argc++; + token = strtok_r(NULL, " ", &sp); + } + + if ( argc == 0 ) + { + str[0] = '\0'; + return; + } + pthread_mutex_lock(&RedisReaderMutex); - reply = redisCommand(config->c_reader_redis, redis_command); + reply = redisCommandArgv(config->c_reader_redis, argc, argv, argvlen); pthread_mutex_unlock(&RedisReaderMutex); if ( reply != NULL ) @@ -401,11 +504,15 @@ void Redis_Reader ( const char *redis_command, char *str, size_t size ) Sagan_Log(DEBUG, "[%s, line %d] Redis 'string' Reply: \"%s\"", __FILE__, __LINE__, reply->str); } - snprintf(str, size, reply->str); - str[reply->len] = '\0'; + /* Use "%s" so the reply is copied as data, never interpreted + as a printf format. snprintf() bounds by 'size' and always + NUL-terminates, so do NOT write str[reply->len] (reply->len + can exceed 'size' and that write overran the buffer). */ + + snprintf(str, size, "%s", reply->str); } - else if ( reply->type == REDIS_REPLY_ARRAY && reply->elements > 0 ) + else if ( reply->type == REDIS_REPLY_ARRAY && reply->elements > 0 && reply->element[0]->str != NULL ) { if ( debug->debugredis ) @@ -413,8 +520,7 @@ void Redis_Reader ( const char *redis_command, char *str, size_t size ) Sagan_Log(DEBUG, "[%s, line %d] Redis 'array' Reply: \"%s\"", __FILE__, __LINE__, reply->element[0]->str); } - snprintf(str, size, reply->element[0]->str); - str[reply->len] = '\0'; + snprintf(str, size, "%s", reply->element[0]->str); } From daac8372dbbe75de71b413fb58c09253229fefae Mon Sep 17 00:00:00 2001 From: Andres Tobar Date: Thu, 2 Jul 2026 14:09:20 -0400 Subject: [PATCH 2/2] Include inttypes.h in config-yaml.c for PRIu64 Fixes the build on toolchains where inttypes.h is not pulled in transitively (only surfaces with --disable-lognorm). --- src/config-yaml.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/config-yaml.c b/src/config-yaml.c index 3c1f722..b134db0 100644 --- a/src/config-yaml.c +++ b/src/config-yaml.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include