fix: process request body once at EOS and correct status/filter bugs

src/mod_security3.c

@@ -145,6 +145,9 @@ static msc_t *create_tx_context(request_rec *r) {
     }
 
     msr->r = r;
+    msr->request_body_processed = 0;  /* Initialize flag */
+    msr->body_replay_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+
     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 +368,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 +403,53 @@ 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 */
+        }
+
+        if (len == -1)
+        {
+            return HTTP_BAD_REQUEST;
+        }
+
+        /* ap_get_client_block() marks the classic client-block API as
+         * exhausted once it sees EOS (r->read_length becomes non-zero and
+         * r->remaining drops to 0), so a later handler calling
+         * ap_should_client_block()/ap_get_client_block() itself (e.g.
+         * mod_cgi) would see "no body" even though the input filter is
+         * holding a buffered copy ready to replay. Reset that bookkeeping
+         * so those handlers still read the body. (Chunked bodies never
+         * reach here: REQUEST_CHUNKED_ERROR above rejects them outright.) */
+        r->read_length = 0;
+        r->remaining = 1;
+    }
+
+    /* 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 +497,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 +624,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);

src/mod_security3.h

@@ -51,6 +51,15 @@ typedef struct
 {
     request_rec *r;
     Transaction *t;
+    int request_body_processed;  /* Flag to track if body was processed */
+    apr_bucket_brigade *body_replay_bb;  /* Buffered body for replay to the
+                                           * real content handler, since
+                                           * hook_request_late consumes the
+                                           * body from the network first.
+                                           * ponytail: whole body buffered in
+                                           * RAM alongside libmodsecurity's
+                                           * own copy; spool to a temp file
+                                           * if large uploads matter. */
 } msc_t;
 
 

src/msc_filters.c

@@ -3,6 +3,76 @@
 #include "msc_utils.h"
 
 
+/* Moves up to nbytes worth of buckets from msr->body_replay_bb into pbbOut,
+ * splitting at a bucket boundary so callers (e.g. mod_proxy_fcgi) that
+ * relay the body onward in fixed-size chunks don't get handed more than
+ * they asked for in one call. */
+static apr_status_t replay_body_bytes(msc_t *msr, apr_bucket_brigade *pbbOut,
+        apr_off_t nbytes)
+{
+    apr_bucket *split;
+    apr_status_t rv;
+
+    rv = apr_brigade_partition(msr->body_replay_bb, nbytes, &split);
+    if (rv != APR_SUCCESS && rv != APR_INCOMPLETE)
+    {
+        return rv;
+    }
+
+    while (!APR_BRIGADE_EMPTY(msr->body_replay_bb)
+            && APR_BRIGADE_FIRST(msr->body_replay_bb) != split)
+    {
+        apr_bucket *b = APR_BRIGADE_FIRST(msr->body_replay_bb);
+        APR_BUCKET_REMOVE(b);
+        APR_BRIGADE_INSERT_TAIL(pbbOut, b);
+    }
+    return APR_SUCCESS;
+}
+
+/* hook_request_late already drained the body from the network so
+ * ModSecurity could inspect it before the real content handler runs.
+ * Replay the buffered copy here instead of reading the (now empty)
+ * network stream again, so the content handler still sees the body. Once
+ * fully replayed, remove ourselves so later reads (or a body left unread
+ * by the handler, drained via ap_discard_request_body) fall straight
+ * through to the network filter, which is already at EOS. */
+static apr_status_t replay_request_body(ap_filter_t *f,
+        apr_bucket_brigade *pbbOut, ap_input_mode_t mode,
+        apr_read_type_e block, apr_off_t nbytes)
+{
+    msc_t *msr = (msc_t *)f->ctx;
+    apr_status_t rv;
+
+    /* Nothing left to replay -- either this filter already replayed the
+     * whole body, or (subrequests/internal redirects share the same msr
+     * as the main request via retrieve_tx_context, while hook_insert_filter
+     * adds a fresh MODSECURITY_IN instance per request) this is a later
+     * request that never captured anything itself. Either way, remove
+     * ourselves and delegate: an input filter must not return success with
+     * an empty brigade. */
+    if (APR_BRIGADE_EMPTY(msr->body_replay_bb))
+    {
+        ap_remove_input_filter(f);
+        return ap_get_brigade(f->next, pbbOut, mode, block, nbytes);
+    }
+
+    if (mode == AP_MODE_READBYTES && nbytes > 0)
+    {
+        rv = replay_body_bytes(msr, pbbOut, nbytes);
+    }
+    else
+    {
+        APR_BRIGADE_CONCAT(pbbOut, msr->body_replay_bb);
+        rv = APR_SUCCESS;
+    }
+
+    if (rv == APR_SUCCESS && APR_BRIGADE_EMPTY(msr->body_replay_bb))
+    {
+        ap_remove_input_filter(f);
+    }
+    return rv;
+}
+
 apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *pbbOut,
         ap_input_mode_t mode, apr_read_type_e block, apr_off_t nbytes)
 {
@@ -19,32 +89,49 @@ 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))
+    if (mode == AP_MODE_EATCRLF)
     {
-        ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes);
+        pbbTmp = apr_brigade_create(r->pool, c->bucket_alloc);
+        return ap_get_brigade(f->next, pbbTmp, mode, block, nbytes);
+    }
 
-        if (mode == AP_MODE_EATCRLF || ret != APR_SUCCESS)
-            return ret;
+    if (msr->request_body_processed)
+    {
+        return replay_request_body(f, pbbOut, mode, block, nbytes);
     }
 
+    pbbTmp = apr_brigade_create(r->pool, c->bucket_alloc);
+
+    ret = ap_get_brigade(f->next, pbbTmp, mode, block, nbytes);
+
+    if (ret != APR_SUCCESS)
+        return ret;
+
     while (!APR_BRIGADE_EMPTY(pbbTmp))
     {
         apr_bucket *pbktIn = APR_BRIGADE_FIRST(pbbTmp);
         apr_bucket *pbktOut;
+        apr_bucket *pbktSave;
         const char *data;
         apr_size_t len;
         apr_size_t n;
         int it;
 
         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);
+            APR_BRIGADE_INSERT_TAIL(msr->body_replay_bb,
+                    apr_bucket_eos_create(c->bucket_alloc));
             break;
         }
 
@@ -54,18 +141,12 @@ 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_bucket_copy(pbktOut, &pbktSave);
+        APR_BRIGADE_INSERT_TAIL(msr->body_replay_bb, pbktSave);
         APR_BRIGADE_INSERT_TAIL(pbbOut, pbktOut);
         apr_bucket_delete(pbktIn);
     }
@@ -132,7 +213,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);

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