detect/http1: inspect HTTP/1 block buffers only when complete

The raw HTTP header buffer had to be registered one state past the
header state. The reason is that htp reports progress as soon as a block
starts: request_progress reaches HEADERS right after the request line
and TRAILER at the zero-length chunk, so a detection round at HEADERS
can run while the block is still arriving. For a byte buffer that shows
- negated content or a within: relative match would decide on a
truncated block. HEADERS + 1 avoided it by accident of numbering, BODY
being what htp reaches once the block is done.

Make completion an event instead of inferring it from numbering. htp's
REQUEST_HEADERS and RESPONSE_HEADERS hooks only ran in test builds,
which is why production had nothing to key on; they now run always and
are bound through the C API. Suricata marks per transaction that a block
ended, from those hooks and from the trailer hooks that already ran, and
the four HTTP/1 block buffers - http.header.raw, http.trailer,
http.trailer.raw and http.trailer_names - report no data until then.
Each registers at the state where its content completes, and no state
offset remains in the tree.

A block that ends because the stream closes needs no special case: htp's
closure handling advances progress past the block state and runs the
trailer hook, so the gate opens on its own and the partial content is
inspected as it is today. Suricata-Verify tests/http1-raw-header-
truncated covers both directions.

htp progress values, and the transaction lifecycle built on them, are
left alone: delaying those changes when firewall decisions become
definitive and fails the ruletype-firewall state hook tests.

Ticket: #8256.

doc/userguide/upgrade.rst

@@ -71,6 +71,12 @@ Major Changes
   ``tx:request_headers()``, ``tx:request_headers_raw()`` and the response twins read the
   header tables and the raw header buffer, and that library has no trailer accessor.
 
+- A HTTP/1 header block spanning several packets is no longer re-inspected while it is still
+  arriving: a rule on ``http.header`` or ``http.header.raw`` now alerts once per transaction
+  instead of once per inspection round. Which transactions alert is unchanged, so alert counts
+  drop on captures where such a block arrived in pieces. This is what makes a negated match or a
+  ``within:`` distance on a header buffer decide on the whole block.
+
 - HTTP/2 gained a transaction state for the trailer block,
   ``http2:stream:request_trailers`` and ``http2:stream:response_trailers``, between
   ``..._data`` and ``..._closed``. Firewall rules with a state range spanning them (for

rust/htp/src/c_api/config.rs

@@ -112,6 +112,32 @@ pub unsafe extern "C" fn htp_config_register_request_start(
     }
 }
 
+/// Registers a REQUEST_HEADERS callback, run when the request header block
+/// is complete.
+/// # Safety
+/// When calling this method, you have to ensure that cfg is either properly initialized or NULL
+#[no_mangle]
+pub unsafe extern "C" fn htp_config_register_request_headers(
+    cfg: *mut Config, cbk_fn: TxExternalCallbackFn,
+) {
+    if let Some(cfg) = cfg.as_mut() {
+        cfg.hook_request_headers.register_extern(cbk_fn)
+    }
+}
+
+/// Registers a RESPONSE_HEADERS callback, run when the response header block
+/// is complete.
+/// # Safety
+/// When calling this method, you have to ensure that cfg is either properly initialized or NULL
+#[no_mangle]
+pub unsafe extern "C" fn htp_config_register_response_headers(
+    cfg: *mut Config, cbk_fn: TxExternalCallbackFn,
+) {
+    if let Some(cfg) = cfg.as_mut() {
+        cfg.hook_response_headers.register_extern(cbk_fn)
+    }
+}
+
 /// Registers a HTP_REQUEST_TRAILER callback.
 /// # Safety
 /// When calling this method, you have to ensure that cfg is either properly initialized or NULL

rust/htp/src/config.rs

@@ -42,7 +42,6 @@ pub struct Config {
     /// they don't use headers).
     pub(crate) hook_request_header_data: DataHook,
     /// Request headers hook, invoked after all request headers are seen.
-    #[cfg(test)]
     pub(crate) hook_request_headers: TxHook,
     /// Request body data hook, invoked every time body data is available. Each
     /// invocation will provide a Data instance. Chunked data
@@ -71,7 +70,6 @@ pub struct Config {
     /// they don't have response headers).
     pub(crate) hook_response_header_data: DataHook,
     /// Response headers book, invoked after all response headers have been seen.
-    #[cfg(test)]
     pub(crate) hook_response_headers: TxHook,
     /// Response body data hook, invoked every time body data is available. Each
     /// invocation will provide a Data instance. Chunked data
@@ -119,7 +117,6 @@ impl Default for Config {
             hook_request_start: TxHook::default(),
             hook_request_line: TxHook::default(),
             hook_request_header_data: DataHook::default(),
-            #[cfg(test)]
             hook_request_headers: TxHook::default(),
             hook_request_body_data: DataHook::default(),
             hook_request_trailer_data: DataHook::default(),
@@ -129,7 +126,6 @@ impl Default for Config {
             #[cfg(test)]
             hook_response_line: TxHook::default(),
             hook_response_header_data: DataHook::default(),
-            #[cfg(test)]
             hook_response_headers: TxHook::default(),
             hook_response_body_data: DataHook::default(),
             hook_response_trailer_data: DataHook::default(),

rust/htp/src/connection_parser.rs

@@ -692,7 +692,6 @@ impl ConnectionParser {
             }
             req.process_request_headers()?;
             // Run hook REQUEST_HEADERS.
-            #[cfg(test)]
             self.cfg
                 .hook_request_headers
                 .clone()
@@ -851,7 +850,6 @@ impl ConnectionParser {
         // Finalize sending raw header data.
         self.response_receiver_finalize_clear(input)?;
         // Run hook RESPONSE_HEADERS.
-        #[cfg(test)]
         self.cfg
             .hook_response_headers
             .clone()

src/app-layer-htp.c

@@ -322,6 +322,7 @@ static const char *HtpStateGetStateNameById(const int id, const uint8_t directio
 
 static void *HTPStateGetTx(void *alstate, uint64_t tx_id);
 static int HTPStateGetAlstateProgress(void *tx, uint8_t direction);
+
 static uint64_t HTPStateGetTxCnt(void *alstate);
 #ifdef UNITTESTS
 static void HTPParserRegisterTests(void);
@@ -1832,6 +1833,53 @@ static int HTPCallbackRequestLine(const htp_connp_t *connp, htp_tx_t *tx)
     return HTP_STATUS_OK;
 }
 
+/* The htp state completion hooks mark the block as finished. Progress is not delayed:
+ * the block buffers withhold their data until the bit is set, so detection never sees a
+ * header or trailer block that is still arriving. */
+static int HTPCallbackRequestHeaders(const htp_connp_t *connp, htp_tx_t *tx)
+{
+    SCEnter();
+    if (tx == NULL)
+        SCReturnInt(HTP_STATUS_ERROR);
+    HtpTxUserData *tx_ud = (HtpTxUserData *)htp_tx_get_user_data(tx);
+    if (tx_ud != NULL)
+        tx_ud->ts_hooks_done |= HTP_HOOK_TS_HEADERS;
+    SCReturnInt(HTP_STATUS_OK);
+}
+
+static int HTPCallbackRequestTrailer(const htp_connp_t *connp, htp_tx_t *tx)
+{
+    SCEnter();
+    if (tx == NULL)
+        SCReturnInt(HTP_STATUS_ERROR);
+    HtpTxUserData *tx_ud = (HtpTxUserData *)htp_tx_get_user_data(tx);
+    if (tx_ud != NULL)
+        tx_ud->ts_hooks_done |= HTP_HOOK_TS_TRAILER;
+    SCReturnInt(HTP_STATUS_OK);
+}
+
+static int HTPCallbackResponseHeaders(const htp_connp_t *connp, htp_tx_t *tx)
+{
+    SCEnter();
+    if (tx == NULL)
+        SCReturnInt(HTP_STATUS_ERROR);
+    HtpTxUserData *tx_ud = (HtpTxUserData *)htp_tx_get_user_data(tx);
+    if (tx_ud != NULL)
+        tx_ud->tc_hooks_done |= HTP_HOOK_TC_HEADERS;
+    SCReturnInt(HTP_STATUS_OK);
+}
+
+static int HTPCallbackResponseTrailer(const htp_connp_t *connp, htp_tx_t *tx)
+{
+    SCEnter();
+    if (tx == NULL)
+        SCReturnInt(HTP_STATUS_ERROR);
+    HtpTxUserData *tx_ud = (HtpTxUserData *)htp_tx_get_user_data(tx);
+    if (tx_ud != NULL)
+        tx_ud->tc_hooks_done |= HTP_HOOK_TC_TRAILER;
+    SCReturnInt(HTP_STATUS_OK);
+}
+
 static int HTPCallbackRequestHeaderData(const htp_connp_t *connp, htp_tx_data_t *tx_data)
 {
     void *ptmp;
@@ -1944,6 +1992,10 @@ static void HTPConfigSetDefaultsPhase1(HTPCfgRec *cfg_prec)
 
     htp_config_register_request_header_data(cfg_prec->cfg, HTPCallbackRequestHeaderData);
     htp_config_register_request_trailer_data(cfg_prec->cfg, HTPCallbackRequestTrailerData);
+    htp_config_register_request_headers(cfg_prec->cfg, HTPCallbackRequestHeaders);
+    htp_config_register_request_trailer(cfg_prec->cfg, HTPCallbackRequestTrailer);
+    htp_config_register_response_headers(cfg_prec->cfg, HTPCallbackResponseHeaders);
+    htp_config_register_response_trailer(cfg_prec->cfg, HTPCallbackResponseTrailer);
     htp_config_register_response_header_data(cfg_prec->cfg, HTPCallbackResponseHeaderData);
     htp_config_register_response_trailer_data(cfg_prec->cfg, HTPCallbackResponseTrailerData);
 
@@ -5688,7 +5740,7 @@ static int HTPParserTest27(void)
 
     uint32_t len = 1000;
 
-    HtpTxUserData *tx_ud = SCMalloc(sizeof(HtpTxUserData));
+    HtpTxUserData *tx_ud = SCCalloc(1, sizeof(HtpTxUserData));
     FAIL_IF_NULL(tx_ud);
 
     tx_ud->tsflags |= HTP_STREAM_DEPTH_SET;

src/app-layer-htp.h

@@ -150,6 +150,11 @@ typedef struct HtpBody_ {
 
 /** Now the Body Chunks will be stored per transaction, at
   * the tx user data */
+#define HTP_HOOK_TS_HEADERS 0x01
+#define HTP_HOOK_TS_TRAILER 0x02
+#define HTP_HOOK_TC_HEADERS 0x01
+#define HTP_HOOK_TC_TRAILER 0x02
+
 typedef struct HtpTxUserData_ {
     /* Body of the request (if any) */
     uint8_t request_body_init;
@@ -158,6 +163,11 @@ typedef struct HtpTxUserData_ {
     uint8_t tsflags;
     uint8_t tcflags;
 
+    /* set as htp runs the state completion hooks; the block buffers check them
+     * before handing data out, progress itself is not delayed */
+    uint8_t ts_hooks_done;
+    uint8_t tc_hooks_done;
+
     uint8_t request_body_type;
 
     HtpBody request_body;
@@ -184,6 +194,40 @@ typedef struct HtpTxUserData_ {
     FileContainer files_tc;
 } HtpTxUserData;
 
+/* Block-shaped buffers must never be seen half-finished. htp reports a state as soon as
+ * its block starts, so completion comes from the state hook; on closure no header hook runs,
+ * and progress leaves the block state - request via request_headers()' CLOSED arm, response
+ * via state_response_complete() - which opens the gate. The third way out is the reassembly
+ * depth limit: htp is never closed and no hook ever runs there, so a depth-cut block must
+ * report what was parsed, which the flow flags say. */
+static inline bool HtpHeaderBlockIsComplete(htp_tx_t *tx, uint8_t flow_flags)
+{
+    HtpTxUserData *tx_ud = htp_tx_get_user_data(tx);
+    if (tx_ud == NULL)
+        return true;
+    if (flow_flags & STREAM_TOSERVER)
+        return (flow_flags & STREAM_DEPTH) != 0 ||
+               htp_tx_request_progress(tx) != HTP_REQUEST_PROGRESS_HEADERS ||
+               (tx_ud->ts_hooks_done & HTP_HOOK_TS_HEADERS) != 0;
+    return (flow_flags & STREAM_DEPTH) != 0 ||
+           htp_tx_response_progress(tx) != HTP_RESPONSE_PROGRESS_HEADERS ||
+           (tx_ud->tc_hooks_done & HTP_HOOK_TC_HEADERS) != 0;
+}
+
+static inline bool HtpTrailerBlockIsComplete(htp_tx_t *tx, uint8_t flow_flags)
+{
+    HtpTxUserData *tx_ud = htp_tx_get_user_data(tx);
+    if (tx_ud == NULL)
+        return true;
+    if (flow_flags & STREAM_TOSERVER)
+        return (flow_flags & STREAM_DEPTH) != 0 ||
+               htp_tx_request_progress(tx) != HTP_REQUEST_PROGRESS_TRAILER ||
+               (tx_ud->ts_hooks_done & HTP_HOOK_TS_TRAILER) != 0;
+    return (flow_flags & STREAM_DEPTH) != 0 ||
+           htp_tx_response_progress(tx) != HTP_RESPONSE_PROGRESS_TRAILER ||
+           (tx_ud->tc_hooks_done & HTP_HOOK_TC_TRAILER) != 0;
+}
+
 typedef struct HtpState_ {
     /* Connection parser structure for each connection */
     htp_connp_t *connp;

src/detect-http-raw-header.c

@@ -96,19 +96,15 @@ void DetectHttpRawHeaderRegister(void)
     sigmatch_table[DETECT_HTTP_RAW_HEADER].Setup = DetectHttpRawHeaderSetupSticky;
     sigmatch_table[DETECT_HTTP_RAW_HEADER].flags |= SIGMATCH_NOOPT|SIGMATCH_INFO_STICKY_BUFFER;
 
-    /* one state past the headers, unlike the other header buffers: the raw bytes arrive
-     * through a data hook that htp only flushes as it leaves the header state, so a round at
-     * HEADERS would decide on a truncated block (negated content and within: would answer
-     * differently than on the complete block) */
     DetectAppLayerInspectEngineRegister("http_raw_header", ALPROTO_HTTP1, SIG_FLAG_TOSERVER,
-            HTP_REQUEST_PROGRESS_HEADERS + 1, DetectEngineInspectBufferGeneric, GetData);
+            HTP_REQUEST_PROGRESS_HEADERS, DetectEngineInspectBufferGeneric, GetData);
     DetectAppLayerInspectEngineRegister("http_raw_header", ALPROTO_HTTP1, SIG_FLAG_TOCLIENT,
-            HTP_RESPONSE_PROGRESS_HEADERS + 1, DetectEngineInspectBufferGeneric, GetData);
+            HTP_RESPONSE_PROGRESS_HEADERS, DetectEngineInspectBufferGeneric, GetData);
 
     DetectAppLayerMpmRegister("http_raw_header", SIG_FLAG_TOSERVER, 2, PrefilterGenericMpmRegister,
-            GetData, ALPROTO_HTTP1, HTP_REQUEST_PROGRESS_HEADERS + 1);
+            GetData, ALPROTO_HTTP1, HTP_REQUEST_PROGRESS_HEADERS);
     DetectAppLayerMpmRegister("http_raw_header", SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister,
-            GetData, ALPROTO_HTTP1, HTP_RESPONSE_PROGRESS_HEADERS + 1);
+            GetData, ALPROTO_HTTP1, HTP_RESPONSE_PROGRESS_HEADERS);
 
     DetectAppLayerInspectEngineRegisterSubState("http_raw_header", ALPROTO_HTTP2, SIG_FLAG_TOSERVER,
             HTTP2TxTypeStream, HTTP2ProgHeaders, DetectEngineInspectBufferGeneric, GetData2);
@@ -196,6 +192,8 @@ static InspectionBuffer *GetData(DetectEngineThreadCtx *det_ctx,
         HtpTxUserData *tx_ud = htp_tx_get_user_data(tx);
 
         const bool ts = ((flow_flags & STREAM_TOSERVER) != 0);
+        if (!HtpHeaderBlockIsComplete(tx, flow_flags))
+            return NULL;
         const uint8_t *data = ts ?
             tx_ud->request_headers_raw : tx_ud->response_headers_raw;
         if (data == NULL)

src/detect-http-trailer-names.c

@@ -121,6 +121,9 @@ static uint8_t *GetBufferForTX(
 {
     *buffer_len = 0;
 
+    if (!HtpTrailerBlockIsComplete(tx, flags))
+        return NULL;
+
     HttpHeaderThreadData *hdr_td = NULL;
     HttpHeaderBuffer *buf = HttpHeaderGetBufferSpace(det_ctx, flags, g_keyword_thread_id, &hdr_td);
     if (unlikely(buf == NULL)) {

src/detect-http-trailer-raw.c

@@ -117,6 +117,8 @@ static InspectionBuffer *GetData(DetectEngineThreadCtx *det_ctx,
         HtpTxUserData *tx_ud = htp_tx_get_user_data(tx);
 
         const bool ts = ((flow_flags & STREAM_TOSERVER) != 0);
+        if (!HtpTrailerBlockIsComplete(tx, flow_flags))
+            return NULL;
         const uint8_t *data = ts ? tx_ud->request_trailers_raw : tx_ud->response_trailers_raw;
         if (data == NULL)
             return NULL;

src/detect-http-trailer.c

@@ -132,6 +132,9 @@ static uint8_t *GetBufferForTX(
 {
     *buffer_len = 0;
 
+    if (!HtpTrailerBlockIsComplete(tx, flags))
+        return NULL;
+
     HttpHeaderThreadData *hdr_td = NULL;
     HttpHeaderBuffer *buf = HttpHeaderGetBufferSpace(det_ctx, flags, g_keyword_thread_id, &hdr_td);
     if (unlikely(buf == NULL)) {