From 6d3d0114ef6ea5f18d1669eadaf676433ebb52a2 Mon Sep 17 00:00:00 2001 From: "Scott R. Shinn" Date: Wed, 26 Aug 2026 10:54:14 -0400 Subject: [PATCH 1/2] Stop remoted from writing the sender counter into agent 0 rids on key reload. Keep the outbound counter on the keystore instead of keyentries[keysize], and replace the sleep(1) FreeKeys drain with an rwlock so a reload cannot close agent files while send still holds them. --- src/headers/pthreads_op.h | 6 + src/headers/sec.h | 14 ++ src/os_crypto/shared/keys.c | 114 +++++++--- src/os_crypto/shared/msgs.c | 197 ++++++++++++------ src/remoted/ar-forward.c | 2 +- src/remoted/manager.c | 48 ++++- src/remoted/remoted.h | 7 +- src/remoted/secure.c | 17 +- src/remoted/sendmsg.c | 32 +-- src/shared/pthreads_op.c | 40 ++++ src/tests/regressions/Makefile | 6 + .../regressions/issue_2065_sender_counter.c | 162 ++++++++++++++ 12 files changed, 525 insertions(+), 120 deletions(-) create mode 100644 src/tests/regressions/issue_2065_sender_counter.c diff --git a/src/headers/pthreads_op.h b/src/headers/pthreads_op.h index 718bba2ed..f3ebd5a20 100644 --- a/src/headers/pthreads_op.h +++ b/src/headers/pthreads_op.h @@ -23,6 +23,12 @@ void os_mutex_destroy(pthread_mutex_t *mutex); size_t os_thread_stack_size(void); +void os_rwlock_init(pthread_rwlock_t *rwlock, const pthread_rwlockattr_t *attr); +void os_rwlock_read(pthread_rwlock_t *rwlock); +void os_rwlock_write(pthread_rwlock_t *rwlock); +void os_rwlock_unlock(pthread_rwlock_t *rwlock); +void os_rwlock_destroy(pthread_rwlock_t *rwlock); + void os_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr); void os_cond_wait(pthread_cond_t *cond, pthread_mutex_t *mutex); void os_cond_signal(pthread_cond_t *cond); diff --git a/src/headers/sec.h b/src/headers/sec.h index 1949cd543..fd9e8fee5 100644 --- a/src/headers/sec.h +++ b/src/headers/sec.h @@ -52,6 +52,14 @@ typedef struct _keystore { /* Key file stat */ time_t file_change; + + /* + * Outbound (sender) counter file. Must not live at + * keyentries[keysize]: that index becomes 0 during reload and used + * to overwrite agent 0's rids file (GitHub issue 2065). + */ + FILE *sender_fp; + ino_t sender_inode; } keystore; /** Function prototypes -- key management **/ @@ -76,6 +84,12 @@ int OS_UpdateKeys(keystore *keys) __attribute((nonnull)); /* Start counter for all agents */ void OS_StartCounter(keystore *keys) __attribute((nonnull)); +/* Persist the outbound sender counter (not an agent rids file) */ +void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) __attribute((nonnull)); + +/* Close the dedicated sender-counter FILE* (locks the sender mutex) */ +void OS_CloseSenderCounter(keystore *keys) __attribute((nonnull)); + /* Remove counter for id */ void OS_RemoveCounter(const char *id) __attribute((nonnull)); diff --git a/src/os_crypto/shared/keys.c b/src/os_crypto/shared/keys.c index d05719db9..20dd411b7 100644 --- a/src/os_crypto/shared/keys.c +++ b/src/os_crypto/shared/keys.c @@ -38,7 +38,7 @@ static void __chash(keystore *keys, const char *id, const char *name, char *ip, /* Allocate for the whole structure */ keys->keyentries = (keyentry **)realloc(keys->keyentries, - (keys->keysize + 2) * sizeof(keyentry *)); + (keys->keysize + 1) * sizeof(keyentry *)); if (!keys->keyentries) { ErrorExit(MEM_ERROR, __local_name, errno, strerror(errno)); } @@ -168,9 +168,15 @@ void OS_ReadKeys(keystore *keys) ErrorExit(MEM_ERROR, __local_name, errno, strerror(errno)); } - /* Initialize structure */ + /* Initialize structure. Callers that already have a live keystore + * (remoted reload) must OS_FreeKeys first so sender_fp is closed. + * Do not fclose sender_fp here: CLI tools stack-allocate an + * uninitialized keystore and a garbage FILE* would be closed. + */ os_calloc(1, sizeof(keyentry*), keys->keyentries); keys->keysize = 0; + keys->sender_fp = NULL; + keys->sender_inode = 0; /* Zero the buffers */ __memclear(id, name, ip, key, KEYSIZE + 1); @@ -264,16 +270,12 @@ void OS_ReadKeys(keystore *keys) } } - /* Add additional entry for sender == keysize */ - os_calloc(1, sizeof(keyentry), keys->keyentries[keys->keysize]); - keys->keyentries[keys->keysize]->fp = NULL; - keys->keyentries[keys->keysize]->inode = 0; - pthread_mutex_init(&keys->keyentries[keys->keysize]->mutex, NULL); - return; } -/* Free the auth keys */ +/* Free the auth keys. + * remoted must hold key_lock_write() — there is no sleep-to-drain window. + */ void OS_FreeKeys(keystore *keys) { unsigned int i = 0; @@ -285,19 +287,22 @@ void OS_FreeKeys(keystore *keys) hashid = keys->keyhash_id; haship = keys->keyhash_ip; - /* Zero the entries */ - keys->keysize = 0; keys->keyhash_id = NULL; keys->keyhash_ip = NULL; - /* Sleep to give time to other threads to stop using them */ - sleep(1); - /* Free the hashes */ - OSHash_Free(hashid); - OSHash_Free(haship); + if (hashid) { + OSHash_Free(hashid); + } + if (haship) { + OSHash_Free(haship); + } + + if (keys->sender_fp) { + OS_CloseSenderCounter(keys); + } - for (i = 0; i <= _keysize; i++) { + for (i = 0; i < _keysize; i++) { if (keys->keyentries[i]) { if (keys->keyentries[i]->ip) { free(keys->keyentries[i]->ip->ip); @@ -346,25 +351,74 @@ int OS_CheckUpdateKeys(const keystore *keys) /* Update the keys if changed */ int OS_UpdateKeys(keystore *keys) { - if (keys->file_change != File_DateofChange(KEYS_FILE)) { - merror(ENCFILE_CHANGED, __local_name); - debug1("%s: DEBUG: Freekeys", __local_name); + unsigned int i; + unsigned int saved_count; + struct { + char *id; + char *ip; + time_t rcvd; + struct sockaddr_storage peer_info; + crypt_method crypto_method; + } *saved = NULL; + + if (keys->file_change == File_DateofChange(KEYS_FILE)) { + return (0); + } - OS_FreeKeys(keys); - debug1("%s: DEBUG: OS_ReadKeys", __local_name); + merror(ENCFILE_CHANGED, __local_name); + debug1("%s: DEBUG: Freekeys", __local_name); - /* Read keys */ - verbose(ENC_READ, __local_name); + saved_count = keys->keysize; + if (saved_count > 0) { + os_calloc(saved_count, sizeof(*saved), saved); + for (i = 0; i < saved_count; i++) { + if (!keys->keyentries[i] || !keys->keyentries[i]->id) { + continue; + } + os_strdup(keys->keyentries[i]->id, saved[i].id); + if (keys->keyentries[i]->ip && keys->keyentries[i]->ip->ip) { + os_strdup(keys->keyentries[i]->ip->ip, saved[i].ip); + } + saved[i].rcvd = keys->keyentries[i]->rcvd; + saved[i].peer_info = keys->keyentries[i]->peer_info; + saved[i].crypto_method = keys->keyentries[i]->crypto_method; + } + } - OS_ReadKeys(keys); - debug1("%s: DEBUG: OS_StartCounter", __local_name); + OS_FreeKeys(keys); + debug1("%s: DEBUG: OS_ReadKeys", __local_name); - OS_StartCounter(keys); - debug1("%s: DEBUG: OS_UpdateKeys completed", __local_name); + /* Read keys */ + verbose(ENC_READ, __local_name); - return (1); + OS_ReadKeys(keys); + debug1("%s: DEBUG: OS_StartCounter", __local_name); + + OS_StartCounter(keys); + + if (saved) { + for (i = 0; i < saved_count; i++) { + int id; + + if (!saved[i].id) { + continue; + } + id = OS_IsAllowedID(keys, saved[i].id); + if (id >= 0 && saved[i].ip && keys->keyentries[id]->ip && + keys->keyentries[id]->ip->ip && + strcmp(keys->keyentries[id]->ip->ip, saved[i].ip) == 0) { + keys->keyentries[id]->rcvd = saved[i].rcvd; + keys->keyentries[id]->peer_info = saved[i].peer_info; + keys->keyentries[id]->crypto_method = saved[i].crypto_method; + } + free(saved[i].id); + free(saved[i].ip); + } + free(saved); } - return (0); + + debug1("%s: DEBUG: OS_UpdateKeys completed", __local_name); + return (1); } /* Check if an IP address is allowed to connect */ diff --git a/src/os_crypto/shared/msgs.c b/src/os_crypto/shared/msgs.c index 1c8e00551..0e265d85d 100644 --- a/src/os_crypto/shared/msgs.c +++ b/src/os_crypto/shared/msgs.c @@ -23,10 +23,11 @@ static ino_t File_Inode(const char *file) { } /* Prototypes */ -static void StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) __attribute((nonnull)); static void StoreCounter(const keystore *keys, int id, unsigned int global, unsigned int local) __attribute((nonnull)); static void ReloadCounter(keystore *keys, unsigned int id, const char * cid) __attribute((nonnull)); +static void ReloadSenderCounter(keystore *keys) __attribute((nonnull)); static char *CheckSum(char *msg, size_t length) __attribute((nonnull)); +static FILE *fopen_rids(const char *rids_file) __attribute((nonnull)); /* Sending counts */ static unsigned int global_count = 0; @@ -85,72 +86,67 @@ void OS_StartCounter(keystore *keys) /* debug2("OS_StartCounter: keysize: %u", keys->keysize); */ - /* Start receiving counter */ - for (i = 0; i <= keys->keysize; i++) { - /* On i == keysize, we deal with the sender counter */ - if (i == keys->keysize) { - snprintf(rids_file, OS_FLSIZE, "%s/%s", - RIDS_DIR, - SENDER_COUNTER); - } else { - snprintf(rids_file, OS_FLSIZE, "%s/%s", - RIDS_DIR, - keys->keyentries[i]->id); - } + /* Start receiving counter (agents only; sender is not keyentries[keysize]) */ + for (i = 0; i < keys->keysize; i++) { + snprintf(rids_file, OS_FLSIZE, "%s/%s", + RIDS_DIR, + keys->keyentries[i]->id); - keys->keyentries[i]->fp = fopen(rids_file, "r+"); + keys->keyentries[i]->fp = fopen_rids(rids_file); - /* If nothing is there, try to open as write only */ if (!keys->keyentries[i]->fp) { - keys->keyentries[i]->fp = fopen(rids_file, "w"); - if (!keys->keyentries[i]->fp) { - int my_error = errno; + int my_error = errno; - /* Just in case we run out of file descriptors */ - if ((i > 10) && (keys->keyentries[i - 1]->fp)) { - fclose(keys->keyentries[i - 1]->fp); + /* Just in case we run out of file descriptors */ + if ((i > 10) && (keys->keyentries[i - 1]->fp)) { + fclose(keys->keyentries[i - 1]->fp); - if (keys->keyentries[i - 2]->fp) { - fclose(keys->keyentries[i - 2]->fp); - } + if (keys->keyentries[i - 2]->fp) { + fclose(keys->keyentries[i - 2]->fp); } - - merror("Unable to open agent file. errno: %d", my_error); - ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); } + + merror("Unable to open agent file. errno: %d", my_error); + ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); } else { unsigned int g_c = 0, l_c = 0; if (fscanf(keys->keyentries[i]->fp, "%u:%u", &g_c, &l_c) != 2) { - if (i == keys->keysize) { - debug2("No previous sender counter."); - } else { - debug2("No previous counter available for '%s'.", - keys->keyentries[i]->name); - } + debug2("No previous counter available for '%s'.", + keys->keyentries[i]->name); g_c = 0; l_c = 0; } - if (i == keys->keysize) { - debug2("Assigning sender counter: %u:%u", - g_c, l_c); - global_count = g_c; - local_count = l_c; - } else { - debug2("Assigning counter for agent %s: '%u:%u'.", - keys->keyentries[i]->name, g_c, l_c); + debug2("Assigning counter for agent %s: '%u:%u'.", + keys->keyentries[i]->name, g_c, l_c); - keys->keyentries[i]->global = g_c; - keys->keyentries[i]->local = l_c; - } + keys->keyentries[i]->global = g_c; + keys->keyentries[i]->local = l_c; } - /* Initialize mutex */ - pthread_mutex_init(&keys->keyentries[i]->mutex, NULL); keys->keyentries[i]->inode = File_Inode(rids_file); } + snprintf(rids_file, OS_FLSIZE, "%s/%s", RIDS_DIR, SENDER_COUNTER); + keys->sender_fp = fopen_rids(rids_file); + if (!keys->sender_fp) { + merror("Unable to open sender counter file. errno: %d", errno); + ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); + } else { + unsigned int g_c = 0, l_c = 0; + if (fscanf(keys->sender_fp, "%u:%u", &g_c, &l_c) != 2) { + debug2("No previous sender counter."); + g_c = 0; + l_c = 0; + } + + debug2("Assigning sender counter: %u:%u", g_c, l_c); + global_count = g_c; + local_count = l_c; + } + keys->sender_inode = File_Inode(rids_file); + debug2("Stored counter."); /* Get counter values */ @@ -183,13 +179,45 @@ void OS_RemoveCounter(const char *id) } } -/* Store sender counter */ -static void StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) +static FILE *fopen_rids(const char *rids_file) { + FILE *fp; + + fp = fopen(rids_file, "r+"); + if (!fp) { + /* r+ fails when the file does not exist; w+ creates it read/write + * so the following fscanf/fprintf paths both work. + */ + fp = fopen(rids_file, "w+"); + } + return (fp); +} + +/* Store sender counter on the dedicated keystore slot, never agent rids. */ +void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) +{ + if (!keys->sender_fp) { + return; + } + /* Write to the beginning of the file */ - fseek(keys->keyentries[keys->keysize]->fp, 0, SEEK_SET); - fprintf(keys->keyentries[keys->keysize]->fp, "%u:%u:", global, local); - fflush(keys->keyentries[keys->keysize]->fp); + if (fseek(keys->sender_fp, 0, SEEK_SET) != 0) { + merror("Unable to seek sender counter: %s (%d)", strerror(errno), errno); + return; + } + fprintf(keys->sender_fp, "%u:%u:", global, local); + fflush(keys->sender_fp); +} + +void OS_CloseSenderCounter(keystore *keys) +{ + os_mutex_lock(&sender_counter_mutex); + if (keys->sender_fp) { + fclose(keys->sender_fp); + keys->sender_fp = NULL; + keys->sender_inode = 0; + } + os_mutex_unlock(&sender_counter_mutex); } /* Store the global and local count of events */ @@ -227,26 +255,16 @@ static void ReloadCounter(keystore *keys, unsigned int id, const char * cid) unsigned int l_c = 0; if (fscanf(keys->keyentries[id]->fp, "%u:%u", &g_c, &l_c) != 2) { - if (id == keys->keysize) { - debug1("No previous sender counter."); - } else { - debug2("No previous counter available for '%s'.", keys->keyentries[id]->id); - } + debug2("No previous counter available for '%s'.", keys->keyentries[id]->id); g_c = 0; l_c = 0; } - if (id == keys->keysize) { - debug1("Reloading sender counter: %u:%u", g_c, l_c); - global_count = g_c; - local_count = l_c; - } else { - debug1("Reloading counter for agent %s: '%u:%u'.", keys->keyentries[id]->id, g_c, l_c); + debug1("Reloading counter for agent %s: '%u:%u'.", keys->keyentries[id]->id, g_c, l_c); - keys->keyentries[id]->global = g_c; - keys->keyentries[id]->local = l_c; - } + keys->keyentries[id]->global = g_c; + keys->keyentries[id]->local = l_c; } keys->keyentries[id]->inode = new_inode; @@ -260,6 +278,51 @@ static void ReloadCounter(keystore *keys, unsigned int id, const char * cid) merror("Unable to reload counter '%s': %s (%d)", cid, strerror(errno), errno); } +static void ReloadSenderCounter(keystore *keys) +{ + /* Caller (CreateSecMSG) holds sender_counter_mutex. */ + ino_t new_inode; + char rids_file[OS_FLSIZE + 1]; + + snprintf(rids_file, OS_FLSIZE, "%s/%s", RIDS_DIR, SENDER_COUNTER); + new_inode = File_Inode(rids_file); + + if (keys->sender_inode == new_inode && keys->sender_fp) { + return; + } + + if (keys->sender_fp) { + fclose(keys->sender_fp); + keys->sender_fp = NULL; + } + + keys->sender_fp = fopen_rids(rids_file); + if (!keys->sender_fp) { + merror("Unable to reload counter '%s': %s (%d)", SENDER_COUNTER, strerror(errno), errno); + keys->sender_inode = 0; + return; + } + + { + unsigned int g_c = 0; + unsigned int l_c = 0; + + if (fscanf(keys->sender_fp, "%u:%u", &g_c, &l_c) != 2) { + debug1("No previous sender counter."); + /* Keep in-memory global/local. Zeroing here would rewind the + * outbound counter and make agents reject later server messages. + */ + } else { + debug1("Reloading sender counter: %u:%u", g_c, l_c); + global_count = g_c; + local_count = l_c; + } + } + + /* Restat after create/reopen so we don't store inode 0 for a new file. */ + keys->sender_inode = File_Inode(rids_file); +} + /* Verify the checksum of the message * Returns NULL on error or the message on success */ @@ -559,7 +622,7 @@ size_t CreateSecMSG(const keystore *keys, const char *msg, size_t msg_length, ch msg_encrypted[OS_MAXSTR] = '\0'; os_mutex_lock(&sender_counter_mutex); - ReloadCounter((keystore *)keys, keys->keysize, SENDER_COUNTER); + ReloadSenderCounter((keystore *)keys); /* Increase local and global counters */ if (local_count >= 9997) { @@ -569,7 +632,7 @@ size_t CreateSecMSG(const keystore *keys, const char *msg, size_t msg_length, ch local_count++; msg_global = global_count; msg_local = local_count; - StoreSenderCounter(keys, msg_global, msg_local); + OS_StoreSenderCounter(keys, msg_global, msg_local); os_mutex_unlock(&sender_counter_mutex); length = snprintf(_tmpmsg, OS_MAXSTR, "%05hu%010u:%04u:", rand1, msg_global, msg_local); diff --git a/src/remoted/ar-forward.c b/src/remoted/ar-forward.c index 56edbb050..cdd4e6811 100644 --- a/src/remoted/ar-forward.c +++ b/src/remoted/ar-forward.c @@ -113,7 +113,7 @@ void *AR_Forward(__attribute__((unused)) void *arg) } /* Lock use of keys */ - key_lock(); + key_lock_read(); /* Send to ALL agents */ if (ar_location & ALL_AGENTS) { diff --git a/src/remoted/manager.c b/src/remoted/manager.c index be5054793..2e352cb5d 100644 --- a/src/remoted/manager.c +++ b/src/remoted/manager.c @@ -283,23 +283,31 @@ static int send_file_toagent(unsigned int agentid, const char *name, const char /* Send the file name first */ snprintf(buf, OS_SIZE_1024, "%s%s%s %s\n", CONTROL_HEADER, FILE_UPDATE_HEADER, sum, name); + key_lock_read(); if (send_msg(remoted_secure_listener, agentid, buf) == -1) { + key_unlock(); merror(SEC_ERROR, ARGV0); fclose(fp); return (-1); } + key_unlock(); /* Send the file contents */ while ((n = fread(buf, 1, 900, fp)) > 0) { buf[n] = '\0'; + key_lock_read(); if (send_msg(remoted_secure_listener, agentid, buf) == -1) { + key_unlock(); merror(SEC_ERROR, ARGV0); fclose(fp); return (-1); } + key_unlock(); - /* Sleep 1 every 30 messages -- no flood */ + /* Sleep 1 every 30 messages -- no flood. Do not hold the key + * read lock across this sleep or OS_UpdateKeys cannot run. + */ if (i > 30) { sleep(1); i = 0; @@ -309,11 +317,14 @@ static int send_file_toagent(unsigned int agentid, const char *name, const char /* Send the message to close the file */ snprintf(buf, OS_SIZE_1024, "%s%s", CONTROL_HEADER, FILE_CLOSE_HEADER); + key_lock_read(); if (send_msg(remoted_secure_listener, agentid, buf) == -1) { + key_unlock(); merror(SEC_ERROR, ARGV0); fclose(fp); return (-1); } + key_unlock(); fclose(fp); @@ -324,6 +335,20 @@ static int send_file_toagent(unsigned int agentid, const char *name, const char static void read_controlmsg(unsigned int agentid, char *msg) { int i; + char srcip[IPSIZE + 1]; + + srcip[0] = '\0'; + key_lock_read(); + if (agentid < keys.keysize && keys.keyentries[agentid] && + keys.keyentries[agentid]->ip && keys.keyentries[agentid]->ip->ip) { + strncpy(srcip, keys.keyentries[agentid]->ip->ip, IPSIZE); + srcip[IPSIZE] = '\0'; + } + key_unlock(); + if (!srcip[0]) { + merror("%s: Invalid message from '%u' (unknown agent)", ARGV0, agentid); + return; + } /* Remove uname */ msg = strchr(msg, '\n'); @@ -351,8 +376,7 @@ static void read_controlmsg(unsigned int agentid, char *msg) msg = strchr(msg, '\n'); if (!msg) { merror("%s: Invalid message from '%s' (strchr \\n)", - ARGV0, - keys.keyentries[agentid]->ip->ip); + ARGV0, srcip); break; } @@ -362,8 +386,7 @@ static void read_controlmsg(unsigned int agentid, char *msg) file = strchr(file, ' '); if (!file) { merror("%s: Invalid message from '%s' (strchr ' ')", - ARGV0, - keys.keyentries[agentid]->ip->ip); + ARGV0, srcip); break; } @@ -476,7 +499,10 @@ void *wait_for_msgs(__attribute__((unused)) void *none) return (NULL); } - /* Check if any agent is ready */ + /* Check if any agent is ready. Hold the key read lock so OS_FreeKeys + * cannot run (no sleep(1) drain) while we index keyentries. + */ + key_lock_read(); for (i = 0; i < keys.keysize; i++) { /* If agent wasn't changed, try next */ if (_changed[i] != 1) { @@ -510,9 +536,19 @@ void *wait_for_msgs(__attribute__((unused)) void *none) } if (id) { + /* Drop the key lock around shared-file I/O (send_file_toagent + * re-acquires around each send_msg). Holding it here would + * block OS_UpdateKeys for the duration of the transfer. + */ + key_unlock(); read_controlmsg(i, msg); + key_lock_read(); + if (i >= keys.keysize) { + break; + } } } + key_unlock(); } return (NULL); diff --git a/src/remoted/remoted.h b/src/remoted/remoted.h index 2fad26bc3..3d4eb7249 100644 --- a/src/remoted/remoted.h +++ b/src/remoted/remoted.h @@ -71,8 +71,11 @@ void sendmsg_unlock(void); int check_keyupdate(void); -void key_lock(void); - +/* Keystore rwlock: write for OS_UpdateKeys, read for lookup/send. + * send_msg() does not take this lock; the caller must hold a read (or write) lock. + */ +void key_lock_read(void); +void key_lock_write(void); void key_unlock(void); void keyupdate_init(void); diff --git a/src/remoted/secure.c b/src/remoted/secure.c index d7c563c3c..72e0b8ff7 100644 --- a/src/remoted/secure.c +++ b/src/remoted/secure.c @@ -130,7 +130,10 @@ void HandleSecure() satop((struct sockaddr *) &peer_info, srcip, IPSIZE); srcip[IPSIZE] = '\0'; - /* Get a valid agent id */ + /* Get a valid agent id. Hold a key read lock from lookup + * through the last use of keyentries (send_msg does not lock). + */ + key_lock_read(); if (buffer[0] == '!') { tmp_msg = buffer; tmp_msg++; @@ -145,6 +148,7 @@ void HandleSecure() } if (*tmp_msg != '!') { + key_unlock(); merror(ENCFORMAT_ERROR, __local_name, srcip); continue; } @@ -155,9 +159,12 @@ void HandleSecure() agentid = OS_IsAllowedDynamicID(&keys, buffer + 1, srcip); if (agentid == -1) { + key_unlock(); if (check_keyupdate()) { + key_lock_read(); agentid = OS_IsAllowedDynamicID(&keys, buffer + 1, srcip); if (agentid == -1) { + key_unlock(); merror(ENC_IP_ERROR, ARGV0, buffer + 1, srcip); continue; } @@ -169,9 +176,12 @@ void HandleSecure() } else { agentid = OS_IsAllowedIP(&keys, srcip); if (agentid < 0) { + key_unlock(); if (check_keyupdate()) { + key_lock_read(); agentid = OS_IsAllowedIP(&keys, srcip); if (agentid == -1) { + key_unlock(); merror(DENYIP_WARN, ARGV0, srcip); continue; } @@ -208,6 +218,7 @@ void HandleSecure() last_aes_reject = now; aes_reject_count = 0; } + key_unlock(); continue; } } else if (logr.crypto_accept == W_ACCEPT_AES) { @@ -234,6 +245,7 @@ void HandleSecure() last_bf_reject = now; bf_reject_count = 0; } + key_unlock(); continue; } } @@ -244,6 +256,7 @@ void HandleSecure() agentid, recv_b - 1, &final_size, srcip); if (tmp_msg == NULL) { /* If duplicated, a warning was already generated */ + key_unlock(); continue; } @@ -256,6 +269,7 @@ void HandleSecure() keys.keyentries[agentid]->rcvd = time(0); sendmsg_unlock(); save_controlmsg((unsigned)agentid, tmp_msg); + key_unlock(); continue; } @@ -263,6 +277,7 @@ void HandleSecure() snprintf(srcmsg, OS_FLSIZE, "(%s) %s", keys.keyentries[agentid]->name, keys.keyentries[agentid]->ip->ip); + key_unlock(); /* * If we can't send the message, try to connect to the diff --git a/src/remoted/sendmsg.c b/src/remoted/sendmsg.c index 6023b84a5..8e588ec22 100644 --- a/src/remoted/sendmsg.c +++ b/src/remoted/sendmsg.c @@ -14,24 +14,29 @@ /* pthread send_msg mutex */ static pthread_mutex_t sendmsg_mutex; -/* pthread key update mutex */ -static pthread_mutex_t keyupdate_mutex; +/* Keystore rwlock: exclusive reload vs concurrent lookup/send */ +static pthread_rwlock_t keyupdate_rwlock; /* Initializes mutex */ void keyupdate_init() { - os_mutex_init(&keyupdate_mutex, NULL); + os_rwlock_init(&keyupdate_rwlock, NULL); } -void key_lock() +void key_lock_read(void) { - os_mutex_lock(&keyupdate_mutex); + os_rwlock_read(&keyupdate_rwlock); } -void key_unlock() +void key_lock_write(void) { - os_mutex_unlock(&keyupdate_mutex); + os_rwlock_write(&keyupdate_rwlock); +} + +void key_unlock(void) +{ + os_rwlock_unlock(&keyupdate_rwlock); } /* Check for key updates */ @@ -42,18 +47,13 @@ int check_keyupdate() return (0); } - key_lock(); - - /* Lock before using */ - os_mutex_lock(&sendmsg_mutex); + key_lock_write(); if (OS_UpdateKeys(&keys)) { - os_mutex_unlock(&sendmsg_mutex); key_unlock(); return (1); } - os_mutex_unlock(&sendmsg_mutex); key_unlock(); return (0); @@ -79,6 +79,7 @@ void sendmsg_unlock(void) /* * Send message to an agent * Returns -1 on error + * Caller must hold key_lock_read() (or write). */ int send_msg(remoted_listener *listener, unsigned int agentid, const char *msg) @@ -94,6 +95,11 @@ int send_msg(remoted_listener *listener, unsigned int agentid, const char *msg) os_mutex_lock(&sendmsg_mutex); + if (agentid >= keys.keysize || !keys.keyentries[agentid]) { + os_mutex_unlock(&sendmsg_mutex); + return (-1); + } + /* If we don't have the agent id, ignore it */ if (keys.keyentries[agentid]->rcvd < (time(0) - (2 * NOTIFY_TIME))) { os_mutex_unlock(&sendmsg_mutex); diff --git a/src/shared/pthreads_op.c b/src/shared/pthreads_op.c index 62ae3aa82..95d7ad588 100644 --- a/src/shared/pthreads_op.c +++ b/src/shared/pthreads_op.c @@ -46,6 +46,46 @@ void os_mutex_destroy(pthread_mutex_t *mutex) #include +void os_rwlock_init(pthread_rwlock_t *rwlock, const pthread_rwlockattr_t *attr) +{ + int error = pthread_rwlock_init(rwlock, attr); + if (error != 0) { + ErrorExit("%s: At pthread_rwlock_init(): %s", __local_name, strerror(error)); + } +} + +void os_rwlock_read(pthread_rwlock_t *rwlock) +{ + int error = pthread_rwlock_rdlock(rwlock); + if (error != 0) { + ErrorExit("%s: At pthread_rwlock_rdlock(): %s", __local_name, strerror(error)); + } +} + +void os_rwlock_write(pthread_rwlock_t *rwlock) +{ + int error = pthread_rwlock_wrlock(rwlock); + if (error != 0) { + ErrorExit("%s: At pthread_rwlock_wrlock(): %s", __local_name, strerror(error)); + } +} + +void os_rwlock_unlock(pthread_rwlock_t *rwlock) +{ + int error = pthread_rwlock_unlock(rwlock); + if (error != 0) { + ErrorExit("%s: At pthread_rwlock_unlock(): %s", __local_name, strerror(error)); + } +} + +void os_rwlock_destroy(pthread_rwlock_t *rwlock) +{ + int error = pthread_rwlock_destroy(rwlock); + if (error != 0) { + ErrorExit("%s: At pthread_rwlock_destroy(): %s", __local_name, strerror(error)); + } +} + size_t os_thread_stack_size(void) { int stack_kb = getDefine_Int("ossec", "thread_stack_size", diff --git a/src/tests/regressions/Makefile b/src/tests/regressions/Makefile index 18dfa894c..af1b49f95 100644 --- a/src/tests/regressions/Makefile +++ b/src/tests/regressions/Makefile @@ -21,6 +21,7 @@ REGRESSION_BINS = \ issue_correlation_agent_shard \ issue_1748_sid_list_crash \ issue_1274_auth_ipv6_key \ + issue_2065_sender_counter \ jsonout_default .PHONY: all check clean @@ -54,6 +55,11 @@ issue_1274_auth_ipv6_key: tests/regressions/issue_1274_auth_ipv6_key.c addagent/ -Wl,--start-group shared.a os_regex.a os_crypto.a os_net.a os_xml.a os_zlib.a libcJSON.a -Wl,--end-group \ -lpcre2-8 -lm -lpthread -lssl -lcrypto -lz +issue_2065_sender_counter: tests/regressions/issue_2065_sender_counter.c os_crypto.a shared.a + $(CC) $(CFLAGS) -o $@ tests/regressions/issue_2065_sender_counter.c \ + -Wl,--start-group shared.a os_regex.a os_crypto.a os_net.a os_xml.a os_zlib.a -Wl,--end-group \ + -lpcre2-8 -lm -lpthread -lssl -lcrypto -lz + # GlobalConf lives in analysisd/config.c (defines _Config Config). # Link config.a from a current server build; do not also compile # config/global-config.c or Read_Global is defined twice. diff --git a/src/tests/regressions/issue_2065_sender_counter.c b/src/tests/regressions/issue_2065_sender_counter.c new file mode 100644 index 000000000..ee94d05ce --- /dev/null +++ b/src/tests/regressions/issue_2065_sender_counter.c @@ -0,0 +1,162 @@ +/* Copyright (C) 2026 Atomicorp, Inc. + * All rights reserved. + * + * This program is a free software; you can redistribute it + * and/or modify it under the terms of the GNU General Public + * License (version 2) as published by the FSF - Free Software + * Foundation. + * + * Issue 2065: persisting the outbound sender counter must not write into + * agent 0's rids file when keysize has been zeroed (OS_FreeKeys race). + */ + +#include +#include +#include +#include +#include + +#include "shared.h" +#include "headers/sec.h" + +static int failures; + +static int write_counter(const char *path, unsigned int g, unsigned int l) +{ + FILE *fp = fopen(path, "w+"); + if (!fp) { + fprintf(stderr, "FAIL: fopen %s: %s\n", path, strerror(errno)); + return -1; + } + if (fprintf(fp, "%u:%u:", g, l) < 0) { + fprintf(stderr, "FAIL: write %s\n", path); + fclose(fp); + return -1; + } + fflush(fp); + fclose(fp); + return 0; +} + +static int read_counter(const char *path, unsigned int *g, unsigned int *l) +{ + FILE *fp = fopen(path, "r"); + if (!fp) { + fprintf(stderr, "FAIL: reopen %s: %s\n", path, strerror(errno)); + return -1; + } + if (fscanf(fp, "%u:%u", g, l) != 2) { + fprintf(stderr, "FAIL: parse %s\n", path); + fclose(fp); + return -1; + } + fclose(fp); + return 0; +} + +static FILE *open_rw(const char *path) +{ + FILE *fp = fopen(path, "r+"); + if (!fp) { + fprintf(stderr, "FAIL: r+ %s: %s\n", path, strerror(errno)); + } + return fp; +} + +static int expect_pair(const char *label, const char *path, unsigned int want_g, unsigned int want_l) +{ + unsigned int g = 0, l = 0; + + if (read_counter(path, &g, &l) != 0) { + failures++; + return -1; + } + if (g != want_g || l != want_l) { + fprintf(stderr, "FAIL: %s is %u:%u (want %u:%u)\n", label, g, l, want_g, want_l); + failures++; + return -1; + } + printf("OK: %s %u:%u\n", label, g, l); + return 0; +} + +int main(void) +{ + char agent_path[] = "/tmp/ossec-2065-agent-XXXXXX"; + char sender_path[] = "/tmp/ossec-2065-sender-XXXXXX"; + int agent_fd, sender_fd; + keystore keys; + keyentry agent; + keyentry *agent_ptr; + FILE *agent_fp; + FILE *sender_fp; + + agent_fd = mkstemp(agent_path); + sender_fd = mkstemp(sender_path); + if (agent_fd < 0 || sender_fd < 0) { + fprintf(stderr, "FAIL: mkstemp: %s\n", strerror(errno)); + return 1; + } + close(agent_fd); + close(sender_fd); + + if (write_counter(agent_path, 1, 2) != 0 || + write_counter(sender_path, 100, 200) != 0) { + unlink(agent_path); + unlink(sender_path); + return 1; + } + + memset(&keys, 0, sizeof(keys)); + memset(&agent, 0, sizeof(agent)); + agent_ptr = &agent; + keys.keyentries = &agent_ptr; + keys.keysize = 1; + + agent_fp = open_rw(agent_path); + sender_fp = open_rw(sender_path); + if (!agent_fp || !sender_fp) { + if (agent_fp) { + fclose(agent_fp); + } + if (sender_fp) { + fclose(sender_fp); + } + unlink(agent_path); + unlink(sender_path); + return 1; + } + + agent.fp = agent_fp; + keys.sender_fp = sender_fp; + + /* Normal path: keysize is still the agent count. */ + OS_StoreSenderCounter(&keys, 300, 400); + expect_pair("agent rids after keyed write", agent_path, 1, 2); + expect_pair("sender after keyed write", sender_path, 300, 400); + + /* Simulate OS_FreeKeys: keysize is zeroed while sender_fp stays open. */ + keys.keysize = 0; + OS_StoreSenderCounter(&keys, 99999, 8888); + + fclose(agent_fp); + fclose(sender_fp); + keys.sender_fp = NULL; + + expect_pair("agent 0 rids while keysize==0", agent_path, 1, 2); + expect_pair("sender counter dedicated file", sender_path, 99999, 8888); + + /* NULL sender_fp must be a no-op (reload window after close). */ + OS_StoreSenderCounter(&keys, 1, 1); + + unlink(agent_path); + unlink(sender_path); + + if (failures) { + fprintf(stderr, "FAIL: issue 2065 sender-counter isolation\n"); + return 1; + } + + printf("PASS: issue #2065 sender counter does not overwrite agent 0\n"); + return 0; +} From 84fb9a43a25ff98db1991bfe731b6f724260bd30 Mon Sep 17 00:00:00 2001 From: "Scott R. Shinn" Date: Wed, 26 Aug 2026 12:20:09 -0400 Subject: [PATCH 2/2] Harden sender-counter reload against races flagged in review. Keep outbound counters across a bad sender file parse, re-resolve agent IDs after dropping the key lock, and serialize the public store path so close/reload cannot write a closed FILE*. --- src/headers/sec.h | 5 ++++- src/os_crypto/shared/keys.c | 1 + src/os_crypto/shared/msgs.c | 36 ++++++++++++++++++++++++------------ src/remoted/manager.c | 23 ++++++++++++++++++++++- 4 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/headers/sec.h b/src/headers/sec.h index fd9e8fee5..0874dcf62 100644 --- a/src/headers/sec.h +++ b/src/headers/sec.h @@ -84,7 +84,10 @@ int OS_UpdateKeys(keystore *keys) __attribute((nonnull)); /* Start counter for all agents */ void OS_StartCounter(keystore *keys) __attribute((nonnull)); -/* Persist the outbound sender counter (not an agent rids file) */ +/* Persist the outbound sender counter (not an agent rids file). + * Takes the sender-counter mutex; CreateSecMSG already holds that lock + * and uses the internal unlocked helper instead. + */ void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) __attribute((nonnull)); /* Close the dedicated sender-counter FILE* (locks the sender mutex) */ diff --git a/src/os_crypto/shared/keys.c b/src/os_crypto/shared/keys.c index 20dd411b7..8397f9f27 100644 --- a/src/os_crypto/shared/keys.c +++ b/src/os_crypto/shared/keys.c @@ -284,6 +284,7 @@ void OS_FreeKeys(keystore *keys) OSHash *haship; _keysize = keys->keysize; + keys->keysize = 0; hashid = keys->keyhash_id; haship = keys->keyhash_ip; diff --git a/src/os_crypto/shared/msgs.c b/src/os_crypto/shared/msgs.c index 0e265d85d..30b25e662 100644 --- a/src/os_crypto/shared/msgs.c +++ b/src/os_crypto/shared/msgs.c @@ -26,6 +26,7 @@ static ino_t File_Inode(const char *file) { static void StoreCounter(const keystore *keys, int id, unsigned int global, unsigned int local) __attribute((nonnull)); static void ReloadCounter(keystore *keys, unsigned int id, const char * cid) __attribute((nonnull)); static void ReloadSenderCounter(keystore *keys) __attribute((nonnull)); +static void store_sender_counter(const keystore *keys, unsigned int global, unsigned int local) __attribute((nonnull)); static char *CheckSum(char *msg, size_t length) __attribute((nonnull)); static FILE *fopen_rids(const char *rids_file) __attribute((nonnull)); @@ -107,7 +108,7 @@ void OS_StartCounter(keystore *keys) } merror("Unable to open agent file. errno: %d", my_error); - ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); + ErrorExit(FOPEN_ERROR, __local_name, rids_file, my_error, strerror(my_error)); } else { unsigned int g_c = 0, l_c = 0; if (fscanf(keys->keyentries[i]->fp, "%u:%u", &g_c, &l_c) != 2) { @@ -125,25 +126,28 @@ void OS_StartCounter(keystore *keys) keys->keyentries[i]->local = l_c; } + /* Per-entry mutex is initialized in __chash() at allocation. */ keys->keyentries[i]->inode = File_Inode(rids_file); } snprintf(rids_file, OS_FLSIZE, "%s/%s", RIDS_DIR, SENDER_COUNTER); keys->sender_fp = fopen_rids(rids_file); if (!keys->sender_fp) { - merror("Unable to open sender counter file. errno: %d", errno); - ErrorExit(FOPEN_ERROR, __local_name, rids_file, errno, strerror(errno)); + int my_error = errno; + merror("Unable to open sender counter file. errno: %d", my_error); + ErrorExit(FOPEN_ERROR, __local_name, rids_file, my_error, strerror(my_error)); } else { unsigned int g_c = 0, l_c = 0; if (fscanf(keys->sender_fp, "%u:%u", &g_c, &l_c) != 2) { debug2("No previous sender counter."); - g_c = 0; - l_c = 0; + /* Keep in-memory counters. Zeroing here rewinds the outbound + * counter on reload and agents reject later server messages. + */ + } else { + debug2("Assigning sender counter: %u:%u", g_c, l_c); + global_count = g_c; + local_count = l_c; } - - debug2("Assigning sender counter: %u:%u", g_c, l_c); - global_count = g_c; - local_count = l_c; } keys->sender_inode = File_Inode(rids_file); @@ -193,8 +197,8 @@ static FILE *fopen_rids(const char *rids_file) return (fp); } -/* Store sender counter on the dedicated keystore slot, never agent rids. */ -void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) +/* Caller must hold sender_counter_mutex. */ +static void store_sender_counter(const keystore *keys, unsigned int global, unsigned int local) { if (!keys->sender_fp) { return; @@ -209,6 +213,14 @@ void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned i fflush(keys->sender_fp); } +/* Persist the outbound sender counter. Never writes agent rids. */ +void OS_StoreSenderCounter(const keystore *keys, unsigned int global, unsigned int local) +{ + os_mutex_lock(&sender_counter_mutex); + store_sender_counter(keys, global, local); + os_mutex_unlock(&sender_counter_mutex); +} + void OS_CloseSenderCounter(keystore *keys) { os_mutex_lock(&sender_counter_mutex); @@ -632,7 +644,7 @@ size_t CreateSecMSG(const keystore *keys, const char *msg, size_t msg_length, ch local_count++; msg_global = global_count; msg_local = local_count; - OS_StoreSenderCounter(keys, msg_global, msg_local); + store_sender_counter(keys, msg_global, msg_local); os_mutex_unlock(&sender_counter_mutex); length = snprintf(_tmpmsg, OS_MAXSTR, "%05hu%010u:%04u:", rand1, msg_global, msg_local); diff --git a/src/remoted/manager.c b/src/remoted/manager.c index 2e352cb5d..396899025 100644 --- a/src/remoted/manager.c +++ b/src/remoted/manager.c @@ -536,12 +536,33 @@ void *wait_for_msgs(__attribute__((unused)) void *none) } if (id) { + char saved_id[KEYSIZE]; + int agent_idx = -1; + + saved_id[0] = '\0'; + if (keys.keyentries[i] && keys.keyentries[i]->id) { + strncpy(saved_id, keys.keyentries[i]->id, KEYSIZE - 1); + saved_id[KEYSIZE - 1] = '\0'; + } + /* Drop the key lock around shared-file I/O (send_file_toagent * re-acquires around each send_msg). Holding it here would * block OS_UpdateKeys for the duration of the transfer. + * Re-resolve by agent ID after the unlocked window so a + * reload cannot send to a reshuffled slot. */ key_unlock(); - read_controlmsg(i, msg); + + key_lock_read(); + if (saved_id[0] != '\0') { + agent_idx = OS_IsAllowedID(&keys, saved_id); + } + key_unlock(); + + if (agent_idx >= 0) { + read_controlmsg((unsigned)agent_idx, msg); + } + key_lock_read(); if (i >= keys.keysize) { break;