From c0fc78ca263de4a29c051710c36755096fb1add5 Mon Sep 17 00:00:00 2001 From: Konstantin Kondrashov Date: Mon, 6 Jul 2026 16:46:16 +0300 Subject: [PATCH] fix(esp_event): skip dispatch for internal cleanup events (SEC-221) After processing an esp_event_handler_cleanup sentinel, execution fell through into the regular dispatch block. Every loop-level (ANY_BASE/ ANY_ID) handler was invoked with base="cleanup" and event_data pointing at the internal esp_event_remove_handler_context_t struct. Consequences: - Information disclosure: internal handler addresses and loop instance pointer are exposed to every loop-level handler. - UAF: if a handler stores event_data for later use, post_instance_delete frees the ctx, turning the stored pointer into a dangling reference. - Logic corruption: handlers that switch on base with a default branch misbehave on every unregister anywhere in the system. Fix: wrap the regular dispatch block in an else clause so it is skipped entirely for cleanup events. post_instance_delete, ticks accounting, and xSemaphoreGiveRecursive remain in the shared tail executed for both paths. Closes SEC_221 --- components/esp_event/esp_event.c | 74 ++++++++++++++++---------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/components/esp_event/esp_event.c b/components/esp_event/esp_event.c index f0ca930561c..40ce60a0761 100644 --- a/components/esp_event/esp_event.c +++ b/components/esp_event/esp_event.c @@ -645,6 +645,8 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick // The event has already been unqueued, so ensure it gets executed. xSemaphoreTakeRecursive(loop->mutex, portMAX_DELAY); + bool exec = false; + // check if the event retrieve from the queue is the internal event that is // triggered when a handler needs to be removed.. if (post.base == esp_event_handler_cleanup) { @@ -658,47 +660,45 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick if (ctx->legacy) { free(ctx->handler_ctx); } - } + } else { + loop->running_task = xTaskGetCurrentTaskHandle(); - loop->running_task = xTaskGetCurrentTaskHandle(); + esp_event_handler_node_t *handler, *temp_handler; + esp_event_loop_node_t *loop_node, *temp_node; + esp_event_base_node_t *base_node, *temp_base; + esp_event_id_node_t *id_node, *temp_id_node; - bool exec = false; - - esp_event_handler_node_t *handler, *temp_handler; - esp_event_loop_node_t *loop_node, *temp_node; - esp_event_base_node_t *base_node, *temp_base; - esp_event_id_node_t *id_node, *temp_id_node; - - SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) { - // Execute loop level handlers - SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } - } - - SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { - if (base_node->base == post.base) { - // Execute base level handlers - SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } + SLIST_FOREACH_SAFE(loop_node, &(loop->loop_nodes), next, temp_node) { + // Execute loop level handlers + SLIST_FOREACH_SAFE(handler, &(loop_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; } + } - SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) { - if (id_node->id == post.id) { - // Execute id level handlers - SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { - if (!handler->unregistered) { - handler_execute(loop, handler, post); - exec |= true; - } + SLIST_FOREACH_SAFE(base_node, &(loop_node->base_nodes), next, temp_base) { + if (base_node->base == post.base) { + // Execute base level handlers + SLIST_FOREACH_SAFE(handler, &(base_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } + } + + SLIST_FOREACH_SAFE(id_node, &(base_node->id_nodes), next, temp_id_node) { + if (id_node->id == post.id) { + // Execute id level handlers + SLIST_FOREACH_SAFE(handler, &(id_node->handlers), next, temp_handler) { + if (!handler->unregistered) { + handler_execute(loop, handler, post); + exec |= true; + } + } + // Skip to next base node + break; } - // Skip to next base node - break; } } } @@ -726,7 +726,7 @@ esp_err_t esp_event_loop_run(esp_event_loop_handle_t event_loop, TickType_t tick xSemaphoreGiveRecursive(loop->mutex); - if (!exec) { + if (!exec && base != esp_event_handler_cleanup) { // No handlers were registered, not even loop/base level handlers ESP_LOGD(TAG, "no handlers have been registered for event %s:%"PRIu32" posted to loop %p", base, id, event_loop); }