From 0b68da0b31815aa9509592c6628c7a961d4609a4 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 12 Oct 2012 16:40:43 +0200 Subject: [PATCH] libhtp: don't use internal iterator It violates thread safety. #601. Suricata assures thread safety on the flow level for HTTP tracking. Part of the flow is (in case of HTTP) libhtp's htp_connp_t state. At startup the libhtp glue layer, app-layer-htp initializes as many htp_cfg_t instances as there are libhtp server configurations in the yaml. At HTTP session start, we look up the proper htp_cfg_t based on the server ip and pass it to htp_connp_create. A ptr to the relevant htp_cfg_t is part of the htp_connp_t. The htp_cfg_t contains "hooks". The are registered based on yaml config at init time. The hooks have lists of type list_t. The list is run with a built in iterator. The iterator is reset at the start of each "hook_run_all". Since multiple flows share the same htp_cfg_t flow A can reset the iterator while flow B is using it. The flow lock has no effect as flows share the htp_cfg_t. This has been observed in real traffic. hook_response_body_data was run on the same data multiple times, leading to corrupt extracted files. --- libhtp/htp/hooks.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/libhtp/htp/hooks.c b/libhtp/htp/hooks.c index 371fce0785..37fd2c9226 100644 --- a/libhtp/htp/hooks.c +++ b/libhtp/htp/hooks.c @@ -130,6 +130,19 @@ int hook_run_all(htp_hook_t *hook, void *data) { return HOOK_OK; } + /* HACK https://redmine.openinfosecfoundation.org/issues/601 */ + size_t i = 0; + for (i = 0; i < ((list_array_t *)hook->callbacks)->current_size; i++) { + void *r = ((list_array_t *)hook->callbacks)->elements[i]; + if (r == NULL) + continue; + + htp_callback_t *callback = r; + if (callback->fn(data) == HOOK_ERROR) { + return HOOK_ERROR; + } + } +#if 0 htp_callback_t *callback = NULL; list_iterator_reset(hook->callbacks); while ((callback = list_iterator_next(hook->callbacks)) != NULL) { @@ -137,7 +150,7 @@ int hook_run_all(htp_hook_t *hook, void *data) { return HOOK_ERROR; } } - +#endif return HOOK_OK; } @@ -153,6 +166,20 @@ int hook_run_one(htp_hook_t *hook, void *data) { return HOOK_DECLINED; } + /* HACK https://redmine.openinfosecfoundation.org/issues/601 */ + size_t i = 0; + for (i = 0; i < ((list_array_t *)hook->callbacks)->current_size; i++) { + void *r = ((list_array_t *)hook->callbacks)->elements[i]; + if (r == NULL) + continue; + + htp_callback_t *callback = r; + int status = callback->fn(data); + if (status != HOOK_DECLINED) { + return status; + } + } +#if 0 htp_callback_t *callback = NULL; list_iterator_reset(hook->callbacks); while ((callback = list_iterator_next(hook->callbacks)) != NULL) { @@ -162,6 +189,6 @@ int hook_run_one(htp_hook_t *hook, void *data) { return status; } } - +#endif return HOOK_DECLINED; }