From 230e14d755bc5912d96e13947aa4b8ef73dbb4fa Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Sat, 25 Jul 2026 15:47:20 -0300 Subject: [PATCH 1/3] fix: process request body once at EOS and correct status/filter bugs - Move msc_process_request_body() into the handler phase (hook_request_late) instead of calling it per input-filter bucket - Register hook_request_late via ap_hook_handler (not fixups) and read the body with ap_setup_client_block()/ap_get_client_block() so the input filter runs; create the transaction context in hook_insert_filter if missing - Set r->status in addition to r->status_line so interventions return the configured HTTP status - Fix input_filter() calling ap_remove_output_filter() instead of ap_remove_input_filter() - Check apr_bucket_read() return value in output_filter() Co-Authored-By: Claude Sonnet 5 --- src/mod_security3.c | 58 +++++++++++++++++++++++++++++++++++++-------- src/mod_security3.h | 1 + src/msc_filters.c | 38 ++++++++++++++++------------- src/msc_utils.c | 3 ++- 4 files changed, 72 insertions(+), 28 deletions(-) diff --git a/src/mod_security3.c b/src/mod_security3.c index 76b3fb7..fd92dde 100644 --- a/src/mod_security3.c +++ b/src/mod_security3.c @@ -145,6 +145,8 @@ static msc_t *create_tx_context(request_rec *r) { } msr->r = r; + msr->request_body_processed = 0; /* Initialize flag */ + unique_id = getenv("UNIQUE_ID"); if (unique_id != NULL && strlen(unique_id) > 0) { msr->t = msc_new_transaction_with_id(msc_apache->modsec, @@ -365,17 +367,17 @@ static int hook_request_late(request_rec *r) /* Find the transaction context and make sure * we are supposed to proceed. */ -#ifdef REQUEST_EARLY msr = retrieve_tx_context(r); -#else - msr = create_tx_context(r); -#endif if (msr == NULL) { - /* If we can't find the context that probably means it's - * a subrequest that was not initiated from the outside. + /* Context should have been created by hook_insert_filter, + * but create it now if it doesn't exist for some reason. */ - return DECLINED; + msr = create_tx_context(r); + if (msr == NULL) + { + return DECLINED; + } } #ifdef LATE_CONNECTION_PROCESS @@ -400,7 +402,37 @@ static int hook_request_late(request_rec *r) #endif + /* Set up to read the request body. + * This is necessary to trigger the input filter which buffers the body. + */ + int rc = ap_setup_client_block(r, REQUEST_CHUNKED_ERROR); + if (rc != OK) + { + return rc; + } + + /* If there's a request body, read it to trigger the input filter */ + if (ap_should_client_block(r)) + { + char buffer[HUGE_STRING_LEN]; + apr_off_t len; + + /* Read body using the simpler ap_get_client_block API + * This should trigger our input filter for each chunk */ + while ((len = ap_get_client_block(r, buffer, sizeof(buffer))) > 0) + { + /* The input filter intercepts this and appends to ModSecurity */ + /* We don't need to do anything with the data here */ + } + } + + /* Process request body. + * The input filter has buffered body data during ap_get_brigade above. + * Now we process it. This handler can properly return HTTP status codes + * for interventions, unlike the input filter. + */ msc_process_request_body(msr->t); + it = process_intervention(msr->t, r); if (it != N_INTERVENTION_STATUS) { @@ -448,11 +480,15 @@ static void hook_insert_filter(request_rec *r) { msc_t *msr = NULL; - /* Find the transaction context first. */ + /* Find the transaction context, or create it if it doesn't exist yet. */ msr = retrieve_tx_context(r); if (msr == NULL) { - return; + msr = create_tx_context(r); + if (msr == NULL) + { + return; + } } #if 1 @@ -571,7 +607,9 @@ static void msc_register_hooks(apr_pool_t *pool) /* still, we don't have location configuration yet. */ ap_hook_process_connection(hook_connection_early, NULL, NULL, APR_HOOK_FIRST); - ap_hook_fixups(hook_request_late, fixups_beforeme_list, NULL, APR_HOOK_REALLY_FIRST); + /* Register as handler to read request body in the proper phase + * Don't use fixups - body reading must happen in handler phase */ + ap_hook_handler(hook_request_late, NULL, NULL, APR_HOOK_REALLY_FIRST); /* Lets add the remaining hooks */ ap_hook_insert_filter(hook_insert_filter, NULL, NULL, APR_HOOK_FIRST); diff --git a/src/mod_security3.h b/src/mod_security3.h index b1e9b28..32d0e39 100644 --- a/src/mod_security3.h +++ b/src/mod_security3.h @@ -51,6 +51,7 @@ typedef struct { request_rec *r; Transaction *t; + int request_body_processed; /* Flag to track if body was processed */ } msc_t; diff --git a/src/msc_filters.c b/src/msc_filters.c index 3a18e21..01dbce9 100644 --- a/src/msc_filters.c +++ b/src/msc_filters.c @@ -19,18 +19,16 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut, { ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_NOERRNO, 0, f->r->server, "ModSecurity: Internal Error: msr is null in input filter."); - ap_remove_output_filter(f); + ap_remove_input_filter(f); return send_error_bucket(msr, f, HTTP_INTERNAL_SERVER_ERROR); } pbbTmp = apr_brigade_create(r->pool, c->bucket_alloc); - if (APR_BRIGADE_EMPTY(pbbTmp)) - { - ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes); - if (mode == AP_MODE_EATCRLF || ret != APR_SUCCESS) - return ret; - } + ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes); + + if (mode == AP_MODE_EATCRLF || ret != APR_SUCCESS) + return ret; while (!APR_BRIGADE_EMPTY(pbbTmp)) { @@ -43,6 +41,11 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut, if (APR_BUCKET_IS_EOS(pbktIn)) { + /* Mark that we've buffered the complete request body */ + /* The actual processing and intervention handling will be done + * by hook_request_late, which can properly return HTTP status codes */ + msr->request_body_processed = 1; + APR_BUCKET_REMOVE(pbktIn); APR_BRIGADE_INSERT_TAIL(pbbOut, pbktIn); break; @@ -54,16 +57,8 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut, return ret; } + /* Append body chunk - processing will happen in hook_request_late */ msc_append_request_body(msr->t, data, len); - it = process_intervention(msr->t, r); - if (it != N_INTERVENTION_STATUS) - { - ap_remove_output_filter(f); - return send_error_bucket(msr, f, it); - } - - // FIXME: Now we should have the body. Is this sane? - msc_process_request_body(msr->t); pbktOut = apr_bucket_heap_create(data, len, 0, c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(pbbOut, pbktOut); @@ -132,7 +127,16 @@ apr_status_t output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) { const char *data; apr_size_t len; - apr_bucket_read(pbktIn, &data, &len, APR_BLOCK_READ); + apr_status_t rv; + + rv = apr_bucket_read(pbktIn, &data, &len, APR_BLOCK_READ); + if (rv != APR_SUCCESS) + { + ap_log_error(APLOG_MARK, APLOG_ERR, rv, f->r->server, + "ModSecurity: Error reading response body bucket"); + return rv; + } + msc_append_response_body(msr->t, data, len); } msc_process_response_body(msr->t); diff --git a/src/msc_utils.c b/src/msc_utils.c index 1b4d16c..6ef6830 100644 --- a/src/msc_utils.c +++ b/src/msc_utils.c @@ -21,7 +21,8 @@ apr_status_t send_error_bucket(msc_t *msr, ap_filter_t *f, int status) apr_bucket_brigade *brigade = NULL; apr_bucket *bucket = NULL; - /* Set the status line explicitly for the error document */ + /* Set both status code and status line */ + f->r->status = status; f->r->status_line = ap_get_status_line(status); brigade = apr_brigade_create(f->r->pool, f->r->connection->bucket_alloc); From 5ea3fc9da876195706375cf35f321de2a1f35ce1 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Sat, 25 Jul 2026 19:13:02 -0300 Subject: [PATCH 2/3] fix: free the rules_set on config-pool cleanup msc_create_rules_set() heap-allocates a RulesSet outside of any APR pool, and nothing ever freed it. Every graceful restart re-parses the config and creates a new one without releasing the old, leaking one RulesSet per restart -- this is issue #82. Register an apr_pool_cleanup that calls msc_rules_cleanup() (the public API's documented counterpart to msc_create_rules_set(), also used by the nginx connector for this exact purpose) when the config pool is destroyed, tying the RulesSet's lifetime to the config generation it belongs to. Confirmed with tools/soak.sh: a memcheck soak with periodic graceful restarts previously reported a "definitely lost" block in msc_create_rules_set (rules_set.cc:278) per restart; after this fix it reports none. Co-Authored-By: Claude Sonnet 5 --- src/msc_config.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/msc_config.c b/src/msc_config.c index 387fae1..1ee8b69 100644 --- a/src/msc_config.c +++ b/src/msc_config.c @@ -117,6 +117,23 @@ static const char *msc_config_load_rules_remote(cmd_parms *cmd, void *_cnf, return NULL; } +/* + * msc_create_rules_set() heap-allocates a RulesSet outside of any APR + * pool. Tie its lifetime to the config pool it was created for, so a + * graceful restart (which tears down the previous generation's config + * pool) frees it instead of leaking it -- see issue #82. + */ +static apr_status_t msc_rules_set_cleanup(void *data) +{ + if (data != NULL) + { + msc_rules_cleanup(data); + } + + return APR_SUCCESS; +} + + void *msc_hook_create_config_directory(apr_pool_t *mp, char *path) { msc_conf_t *cnf = NULL; @@ -132,6 +149,11 @@ void *msc_hook_create_config_directory(apr_pool_t *mp, char *path) #endif cnf->rules_set = msc_create_rules_set(); + if (cnf->rules_set != NULL) + { + apr_pool_cleanup_register(mp, cnf->rules_set, msc_rules_set_cleanup, + apr_pool_cleanup_null); + } if (path != NULL) { cnf->name_for_debug = strdup(path); From 1e07559819163e4c23338d646859422b0efd5c0e Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Sat, 25 Jul 2026 19:13:10 -0300 Subject: [PATCH 3/3] fix: free the intervention log/url strings after use msc_intervention() heap-allocates intervention.url and intervention.log for the caller to free (see libmodsecurity's intervention::free() in intervention.h), but process_intervention() never did, leaking one allocation per blocked request -- unbounded growth under sustained attack traffic. Switch the Location header from apr_table_setn() to apr_table_set() so the copy stored in r->headers_out survives freeing the original, then free both strings before returning. Also drops the dead "(no log message was specified)" fallback, which was never read and would otherwise get passed to free() as a string literal. Confirmed with tools/soak.sh: a memcheck soak previously reported hundreds of KB "definitely lost" per run in modsecurity::Transaction::intervention (transaction.cc:1382) via strdup; after this fix it reports none. Co-Authored-By: Claude Sonnet 5 --- src/mod_security3.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/mod_security3.c b/src/mod_security3.c index fd92dde..8f67573 100644 --- a/src/mod_security3.c +++ b/src/mod_security3.c @@ -36,6 +36,8 @@ void modsecurity_log_cb(void *log, const void* data) int process_intervention (Transaction *t, request_rec *r) { ModSecurityIntervention intervention; + int status = N_INTERVENTION_STATUS; + intervention.status = N_INTERVENTION_STATUS; intervention.url = NULL; intervention.log = NULL; @@ -48,27 +50,32 @@ int process_intervention (Transaction *t, request_rec *r) return N_INTERVENTION_STATUS; } - if (intervention.log == NULL) - { - intervention.log = "(no log message was specified)"; - } - if (intervention.status == 301 || intervention.status == 302 ||intervention.status == 303 || intervention.status == 307) { if (intervention.url != NULL) { - apr_table_setn(r->headers_out, "Location", intervention.url); - return HTTP_MOVED_TEMPORARILY; + /* apr_table_set() copies the value into r->pool, unlike + * apr_table_setn(), so the heap string msc_intervention() + * handed us can still be freed below. */ + apr_table_set(r->headers_out, "Location", intervention.url); + status = HTTP_MOVED_TEMPORARILY; } } - if (intervention.status != N_INTERVENTION_STATUS) + if (status == N_INTERVENTION_STATUS && intervention.status != N_INTERVENTION_STATUS) { - return intervention.status; + status = intervention.status; } - return N_INTERVENTION_STATUS; + /* msc_intervention() heap-allocates url/log for the caller to free + * (see libmodsecurity's intervention::free() in intervention.h); + * this connector never did, leaking one allocation per blocked + * request. */ + free(intervention.url); + free(intervention.log); + + return status; }