From 4241dae1adf30bff06f854784f71e3564c5ac277 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 19 Oct 2026 06:49:23 +0000 Subject: [PATCH] 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 | 6 ++++ rust/htp/src/c_api/config.rs | 26 +++++++++++++++ rust/htp/src/config.rs | 4 --- rust/htp/src/connection_parser.rs | 2 -- src/app-layer-htp.c | 54 ++++++++++++++++++++++++++++++- src/app-layer-htp.h | 44 +++++++++++++++++++++++++ src/detect-http-raw-header.c | 14 ++++---- src/detect-http-trailer-names.c | 3 ++ src/detect-http-trailer-raw.c | 2 ++ src/detect-http-trailer.c | 3 ++ 10 files changed, 143 insertions(+), 15 deletions(-) diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index 67144e2127..4d382ce05f 100644 --- a/doc/userguide/upgrade.rst +++ b/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 diff --git a/rust/htp/src/c_api/config.rs b/rust/htp/src/c_api/config.rs index f56238b47a..e6f9e01838 100644 --- a/rust/htp/src/c_api/config.rs +++ b/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 diff --git a/rust/htp/src/config.rs b/rust/htp/src/config.rs index d165cb98d2..0c20207fd6 100644 --- a/rust/htp/src/config.rs +++ b/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(), diff --git a/rust/htp/src/connection_parser.rs b/rust/htp/src/connection_parser.rs index 6e9f6119f1..d60b944064 100644 --- a/rust/htp/src/connection_parser.rs +++ b/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() diff --git a/src/app-layer-htp.c b/src/app-layer-htp.c index 0c33375ea2..c18a84adbc 100644 --- a/src/app-layer-htp.c +++ b/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; diff --git a/src/app-layer-htp.h b/src/app-layer-htp.h index 05ef1391b7..d5486b6aa5 100644 --- a/src/app-layer-htp.h +++ b/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; diff --git a/src/detect-http-raw-header.c b/src/detect-http-raw-header.c index a4705687ba..3ac3bbbe13 100644 --- a/src/detect-http-raw-header.c +++ b/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) diff --git a/src/detect-http-trailer-names.c b/src/detect-http-trailer-names.c index ed7c388854..3f7ed76d71 100644 --- a/src/detect-http-trailer-names.c +++ b/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)) { diff --git a/src/detect-http-trailer-raw.c b/src/detect-http-trailer-raw.c index 7cc5f63a34..62626c75dd 100644 --- a/src/detect-http-trailer-raw.c +++ b/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; diff --git a/src/detect-http-trailer.c b/src/detect-http-trailer.c index d68eadc453..4dd83bfae0 100644 --- a/src/detect-http-trailer.c +++ b/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)) {