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);