From 4498295d8be85a559373db14f333f0d8da7193fd Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sun, 4 Oct 2026 13:56:36 +0000 Subject: [PATCH 01/10] firewall: let a higher hook cover a pending rule An LTE (`flags & SIG_FLAG_FW_HOOK_LTE) == 0 || !AppLayerParserSupportsSubStates(f->alproto)) { + return false; + } + return progress < AppLayerParserGetTxEndState(f->proto, f->alproto, txv, flags); +} + /** \internal * \brief is the engine's data final for this tx? * - * Past the engine's phase the answer is yes; at or beyond the tx end state it - * is final too, even for engines registered at the completion state (no P+1). - * AppLayerParserGetStateProgress() returns the end progress for disrupted - * flows, so progress == end stays a valid finality signal there. + * Past the engine's phase the answer is yes, unless the rule is an LTE rule + * that can still match in a later state of its sub-state (DetectLteRevisit()). + * At or beyond the tx end state it is final too, even for engines registered + * at the completion state (no P+1). AppLayerParserGetStateProgress() returns + * the end progress for disrupted flows, so progress == end stays a valid + * finality signal there. */ -static bool DetectTxCompleted( - Flow *f, void *txv, uint8_t flags, const DetectEngineAppInspectionEngine *engine) +static bool DetectTxCompleted(Flow *f, void *txv, uint8_t flags, + const DetectEngineAppInspectionEngine *engine, const Signature *s) { if (f->alproto == ALPROTO_DOH2 && engine->alproto == ALPROTO_DOH2) { // the DNS tx from DetectGetInnerTx is always complete @@ -2144,6 +2163,9 @@ static bool DetectTxCompleted( return false; } if (progress > engine->progress) { + if (DetectLteRevisit(f, txv, flags, s, progress)) { + return false; + } return true; } if (progress < engine->progress) { @@ -2173,7 +2195,7 @@ uint8_t DetectEngineInspectGenericList(DetectEngineCtx *de_ctx, DetectEngineThre if ((sigmatch_table[smd->type].flags & SIGMATCH_STATEFUL) != 0) { return DETECT_ENGINE_INSPECT_SIG_NO_MATCH; } - return DetectTxCompleted(f, txv, flags, engine) + return DetectTxCompleted(f, txv, flags, engine, s) ? DETECT_ENGINE_INSPECT_SIG_CANT_MATCH : DETECT_ENGINE_INSPECT_SIG_NO_MATCH; } @@ -2211,7 +2233,7 @@ uint8_t DetectEngineInspectBufferSingle(DetectEngineCtx *de_ctx, DetectEngineThr const int list_id = engine->sm_list; SCLogDebug("running inspect on %d", list_id); - const bool eof = DetectTxCompleted(f, txv, flags, engine); + const bool eof = DetectTxCompleted(f, txv, flags, engine, s); SCLogDebug("list %d mpm? %s transforms %p", engine->sm_list, engine->mpm ? "true" : "false", engine->v2.transforms); @@ -2271,7 +2293,7 @@ uint8_t DetectEngineInspectBufferGeneric(DetectEngineCtx *de_ctx, DetectEngineTh const int list_id = engine->sm_list; SCLogDebug("running inspect on %d", list_id); - const bool eof = DetectTxCompleted(f, txv, flags, engine); + const bool eof = DetectTxCompleted(f, txv, flags, engine, s); SCLogDebug("list %d mpm? %s transforms %p", engine->sm_list, engine->mpm ? "true" : "false", engine->v2.transforms); @@ -2405,7 +2427,7 @@ uint8_t DetectEngineInspectMultiBufferGeneric(DetectEngineCtx *de_ctx, } while (1); if (local_id == 0) { // That means we did not get even one buffer value from the multi-buffer - const bool eof = DetectTxCompleted(f, txv, flags, engine); + const bool eof = DetectTxCompleted(f, txv, flags, engine, s); if (eof && engine->match_on_null) { return DETECT_ENGINE_INSPECT_SIG_MATCH; } diff --git a/src/detect.c b/src/detect.c index 4e5e41d5f4..be63014841 100644 --- a/src/detect.c +++ b/src/detect.c @@ -2453,7 +2453,9 @@ static void DetectRunTxFirewallRuleFullMatch(DetectEngineThreadCtx *det_ctx, con * * A default accept is appended. TD has a chance to override this accept. * - * \retval 1 accept partial, caller must break loop + * \retval 1 partial accept handled: the caller breaks, unless the rule is an + * LTE rule the tx has moved past its hook - then it skips the rule's + * hook and the ones below it and continues with the higher hooks * \retval 0 ok, caller must continue as normal */ static int DetectRunTxFirewallRulePartialMatch( @@ -2863,7 +2865,21 @@ static void DetectRunTx(ThreadVars *tv, } else if (r == 0) { SCLogDebug("sid %u partial match", s->id); if (DetectRunTxFirewallRulePartialMatch(det_ctx, s, &tx, p) == 1) { - break; + /* partial matches are only produced for LTE rules today; + * keep the check as defence in case that changes */ + if ((s->flags & SIG_FLAG_FW_HOOK_LTE) == 0 || + tx.tx_progress <= s->app_progress_hook) { + break; + } + /* The tx moved past the pending rule's hook: a rule hooked + * above it still has to run, as a match there covers the + * pending rule and its policy decides the flow. Skip this + * hook and the ones below it, like an accept:hook does. + * The states above the hook are not swept while the rule is + * pending: they are decided when it resolves, so a higher + * state's default cannot preempt the pending rule's own. */ + fw_state.skip_fw_hook = true; + fw_state.skip_before_progress = s->app_progress_hook; } } else if (r == -1) { if ((s->flags & SIG_FLAG_FIREWALL) != 0 && From 5d32758fa30c38eaa154d97f2c09b089976b8a42 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 6 Oct 2026 20:19:23 +0000 Subject: [PATCH 02/10] doc: document pending LTE hook rule behavior A ` Date: Sat, 3 Oct 2026 10:21:40 +0000 Subject: [PATCH 03/10] app-layer: register the sub-state completion A sub-state's buffers are final at a progress value the parser knows but the API did not expose: AppLayerParserGetSubStateCompletion() had the http2 values hard-coded and asserted for every other protocol. Register the value with the sub-state and resolve it from there. The per-tx tx_type_eop_ts/tc fields become redundant: the tx type is the registered sub-state, so AppLayerParserGetTxEndState() returns its completion. http2 no longer sets the fields. Ticket: #9149. --- rust/src/http2/http2.rs | 10 ++-------- rust/sys/src/sys.rs | 8 ++------ src/app-layer-parser.c | 44 +++++++++++++++++++++++++---------------- src/app-layer-parser.h | 15 +++++++------- 4 files changed, 38 insertions(+), 39 deletions(-) diff --git a/rust/src/http2/http2.rs b/rust/src/http2/http2.rs index de51ff4aab..6490fb576e 100644 --- a/rust/src/http2/http2.rs +++ b/rust/src/http2/http2.rs @@ -883,10 +883,6 @@ impl HTTP2State { let mut tx = HTTP2Transaction::new(); tx.tx_data = AppLayerTxData::for_direction(dir); tx.tx_data.0.tx_type = HTTP2TxType::HTTP2TxTypeGlobal as u8; - // a parser with tx types must fill both eop fields: the end state - // helper asserts on 0 - tx.tx_data.0.tx_type_eop_ts = HTTP2TxGlobalProgress::HTTP2ProgGlobalComplete as u8; - tx.tx_data.0.tx_type_eop_tc = HTTP2TxGlobalProgress::HTTP2ProgGlobalComplete as u8; self.tx_id += 1; tx.tx_id = self.tx_id; tx.progress = HTTP2Progress::GLOBAL(HTTP2GlobalProgress::complete()); @@ -973,10 +969,6 @@ impl HTTP2State { tx.update_file_flags(tx.tx_data.0.file_flags); tx.tx_data.0.file_tx = STREAM_TOSERVER | STREAM_TOCLIENT; // might hold files in both directions tx.tx_data.0.tx_type = HTTP2TxType::HTTP2TxTypeStream as u8; - // a parser with tx types must fill both eop fields: the end state - // helper asserts on 0 - tx.tx_data.0.tx_type_eop_ts = HTTP2TxProgress::HTTP2ProgComplete as u8; - tx.tx_data.0.tx_type_eop_tc = HTTP2TxProgress::HTTP2ProgComplete as u8; self.transactions.push_back(tx); return Some(self.transactions.back_mut().unwrap()); } @@ -1871,12 +1863,14 @@ pub unsafe extern "C" fn SCRegisterHttp2Parser() { HTTP2TxType::HTTP2TxTypeStream as u8, Some(HTTP2TxProgress::ffi_id_from_name), Some(HTTP2TxProgress::ffi_name_from_id), + HTTP2TxProgress::HTTP2ProgComplete as u8, ); SCAppLayerParserRegisterGetTxSubStateFuncs( ALPROTO_HTTP2, HTTP2TxType::HTTP2TxTypeGlobal as u8, Some(HTTP2TxGlobalProgress::ffi_id_from_name), Some(HTTP2TxGlobalProgress::ffi_name_from_id), + HTTP2TxGlobalProgress::HTTP2ProgGlobalComplete as u8, ); SCLogDebug!("Rust http2 parser registered."); diff --git a/rust/sys/src/sys.rs b/rust/sys/src/sys.rs index 67097e5edb..5945d001c4 100644 --- a/rust/sys/src/sys.rs +++ b/rust/sys/src/sys.rs @@ -1517,12 +1517,8 @@ pub struct AppLayerTxData { #[doc = " detection engine progress tracking for use by detection engine\n Reflects the \"progress\" of prefilter engines into this TX, where\n the value is offset by 1. So if for progress state 0 the engines\n are done, the value here will be 1. So a value of 0 means, no\n progress tracked yet.\n"] pub detect_progress_ts: u8, pub detect_progress_tc: u8, - #[doc = " Type of transaction. Meaning is defined by the parser. Used to\n select a state machine. 0 means it is not used."] + #[doc = " Type of transaction. Meaning is defined by the parser. Used to\n select a state machine. 0 means it is not used. A non-zero value is\n also the sub-state id registered by the parser, whose completion is\n the tx's end state."] pub tx_type: u8, - #[doc = " End of TX progress values\n\n toserver end of tx progress value"] - pub tx_type_eop_ts: u8, - #[doc = " toclient end of tx progress value"] - pub tx_type_eop_tc: u8, pub de_state: *mut DetectEngineState, pub events: *mut AppLayerDecoderEvents, pub txbits: *mut GenericVar, @@ -1570,7 +1566,7 @@ extern "C" { #[doc = " \\brief register state<>name funcs for a substate"] pub fn SCAppLayerParserRegisterGetTxSubStateFuncs( alproto: AppProto, sub_state: u8, GetIdByNameFunc: AppLayerParserGetStateIdByNameFn, - GetNameByIdFunc: AppLayerParserGetStateNameByIdFn, + GetNameByIdFunc: AppLayerParserGetStateNameByIdFn, completion: u8, ); } extern "C" { diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c index dab0b17f53..014e85c872 100644 --- a/src/app-layer-parser.c +++ b/src/app-layer-parser.c @@ -150,6 +150,10 @@ typedef struct AppLayerParserProtoCtx_ /* max value of a sub state * only set for FLOW_PROTO_DEFAULT */ uint8_t max_sub_state; + /* progress value at which the sub-state's buffers are final, indexed by + * the sub state id + * only set for FLOW_PROTO_DEFAULT */ + uint8_t sub_state_completion[APP_LAYER_MAX_SUB_STATES]; } AppLayerParserProtoCtx; typedef struct AppLayerParserCtx_ { @@ -618,13 +622,19 @@ void AppLayerParserRegisterGetEventInfoById(uint8_t ipproto, AppProto alproto, void SCAppLayerParserRegisterGetTxSubStateFuncs(AppProto alproto, const uint8_t sub_state, AppLayerParserGetStateIdByNameFn GetIdByNameFunc, - AppLayerParserGetStateNameByIdFn GetNameByIdFunc) + AppLayerParserGetStateNameByIdFn GetNameByIdFunc, const uint8_t completion) { SCEnter(); /* validate input */ BUG_ON(sub_state == 0); BUG_ON(GetIdByNameFunc == NULL); BUG_ON(GetNameByIdFunc == NULL); + if (completion == 0 || sub_state >= APP_LAYER_MAX_SUB_STATES) { + /* 0 would make the first state look final, so it must not reach the + * end state helper */ + FatalError("invalid sub state %u (completion %u) for %s", sub_state, completion, + AppProtoToString(alproto)); + } AppLayerParserProtoCtx *p = &alp_ctx.ctxs[alproto][FLOW_PROTO_DEFAULT]; /* double registration not allowed */ @@ -640,6 +650,7 @@ void SCAppLayerParserRegisterGetTxSubStateFuncs(AppProto alproto, const uint8_t m->GetStateNameById = GetNameByIdFunc; m->next = p->sub_state_mappings; p->sub_state_mappings = m; + p->sub_state_completion[sub_state] = completion; p->max_sub_state = MAX(p->max_sub_state, sub_state); SCLogDebug("alproto %u:%s, sub_state:%u max:%u %p:%p", alproto, AppProtoToString(alproto), @@ -1180,15 +1191,16 @@ uint8_t AppLayerParserGetTxEndState(uint8_t ipproto, AppProto alproto, void *tx, if (txd == NULL || txd->tx_type == 0) { return (uint8_t)AppLayerParserGetStateProgressCompletionStatus(alproto, flags); } - const uint8_t eop = (flags & STREAM_TOSERVER) ? txd->tx_type_eop_ts : txd->tx_type_eop_tc; - if (unlikely(eop == 0)) { - /* a parser with tx types must fill both end-of-phase fields; without - * the fallback 0 would make the first state look final in release - * builds */ + /* the tx type is the sub-state the parser registered; its completion is + * the tx's end state */ + const uint8_t completion = AppLayerParserGetSubStateCompletion(alproto, txd->tx_type); + if (unlikely(completion == 0)) { + /* a parser with tx types must register the sub-state's completion: + * without the fallback 0 would make the first state look final */ DEBUG_VALIDATE_BUG_ON(1); return (uint8_t)AppLayerParserGetStateProgressCompletionStatus(alproto, flags); } - return eop; + return completion; } /** @@ -1308,18 +1320,16 @@ uint8_t AppLayerParserGetSubStateCompletion(const AppProto alproto, const uint8_ if (alproto == ALPROTO_DOH2) return AppLayerParserGetSubStateCompletion(ALPROTO_HTTP2, sub_state); - BUG_ON(alp_ctx.ctxs[alproto][FLOW_PROTO_DEFAULT].max_sub_state == 0); + const AppLayerParserProtoCtx *p = &alp_ctx.ctxs[alproto][FLOW_PROTO_DEFAULT]; - /* TODO hard coded for now */ - BUG_ON(alproto != ALPROTO_HTTP2); - - if (sub_state == 1) { - return 4; - } else if (sub_state == 2) { - return 1; - } else { - BUG_ON(1); + /* no max_sub_state guard here on purpose: an unregistered protocol lands on + * the fallback below, which is what a recovery path wants */ + if (likely(sub_state < APP_LAYER_MAX_SUB_STATES && p->sub_state_completion[sub_state] != 0)) { + return p->sub_state_completion[sub_state]; } + /* a tx type without a registered sub-state is a parser bug: the caller + * falls back to the protocol's completion status */ + DEBUG_VALIDATE_BUG_ON(1); return 0; } diff --git a/src/app-layer-parser.h b/src/app-layer-parser.h index 5e348e1eb0..42eb5d9ce7 100644 --- a/src/app-layer-parser.h +++ b/src/app-layer-parser.h @@ -80,6 +80,9 @@ uint8_t AppLayerParserGetTxEndState(uint8_t ipproto, AppProto alproto, void *tx, /** progress values need to stay under this. */ #define APP_LAYER_MAX_PROGRESS 48 +/** maximum number of sub-states a parser can register (ids 1 to N-1) */ +#define APP_LAYER_MAX_SUB_STATES 8 + /***** transaction handling *****/ int AppLayerParserSetup(void); @@ -211,14 +214,10 @@ typedef struct AppLayerTxData { uint8_t detect_progress_tc; /// Type of transaction. Meaning is defined by the parser. Used to - /// select a state machine. 0 means it is not used. + /// select a state machine. 0 means it is not used. A non-zero value is + /// also the sub-state id registered by the parser, whose completion is + /// the tx's end state. uint8_t tx_type; - /// End of TX progress values - /// - /// toserver end of tx progress value - uint8_t tx_type_eop_ts; - /// toclient end of tx progress value - uint8_t tx_type_eop_tc; DetectEngineState *de_state; AppLayerDecoderEvents *events; @@ -304,7 +303,7 @@ void AppLayerParserRegisterGetStateFuncs(uint8_t ipproto, AppProto alproto, /** \brief register state<>name funcs for a substate */ void SCAppLayerParserRegisterGetTxSubStateFuncs(AppProto alproto, const uint8_t sub_state, AppLayerParserGetStateIdByNameFn GetIdByNameFunc, - AppLayerParserGetStateNameByIdFn GetNameByIdFunc); + AppLayerParserGetStateNameByIdFn GetNameByIdFunc, uint8_t completion); void AppLayerParserRegisterTxDataFunc(uint8_t ipproto, AppProto alproto, AppLayerTxData *(*GetTxData)(void *tx)); From c9c1c635bec9821cc359fe991b691039483234f9 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sat, 3 Oct 2026 22:11:33 +0000 Subject: [PATCH 04/10] prefilter: bound tx engines by progress Tx engines run from a minimal tx progress upwards, with no way to stop at a state: once such an engine fires, its rules stay candidates for every higher state. The firewall "less than or equal" hook handling needs the opposite: a rule that must be considered while the tx is still below its hook, but not at the hook itself. Add tx_max_progress to PrefilterEngine.ctx.app (skips the engine once the tx reached it, 0 for no bound) and to PrefilterEngineList so the value survives setup. Nothing sets it yet, so behavior is unchanged. Ticket: #9149. --- doc/userguide/firewall/firewall-design.rst | 53 +++++++++++++++++++--- src/detect-engine-prefilter.c | 1 + src/detect.h | 6 +++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index d61b78d732..032e734818 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -295,16 +295,16 @@ no-match does not apply the default policy of its own state. A sub-state's buffers can also grow after the transaction moved past the rule's hook: a http2 trailer HEADERS frame updates the header buffer above the -``request_headers`` hook. Engine data is therefore only final once the sub- -state completed, and the rule stays revisitable until then. The default -policies of the states it covers stay deferred with it. +``request_headers`` hook. The fast pattern at the hook decides regardless: above +it the group is retired, so the rule is not run again there. The default +policies of the states it covers are deferred by the window at the hook only. The bound is the transaction's end state. For a parser with per-direction sub- states that can be later than the direction's own close - a http2 stream completes when both sides closed, while its request buffers are final once the -client sent END_STREAM - so the rule stays revisitable longer than strictly -needed. That is deliberate: the tighter bound needs a per-direction completion -the parsers do not expose yet. +client sent END_STREAM - so a window can open where that direction's own buffers +are final already. That is deliberate: the tighter bound needs a per-direction +completion the parsers do not expose yet. Firewall pipeline ----------------- @@ -456,3 +456,44 @@ reaches them:: A ``<`` hook rule at the protocol's first state (progress 0) is accepted and is equivalent to the plain hook form, as there are no prior states to auto-accept. + +The pending window +------------------ + +Two words for two sets of rules, both rebuilt on every update of a transaction: + +* the *group* of a hook: the LTE rules registered for one progress state of the + transaction, held as a list of rule ids in one prefilter engine. +* the *pending window*: the rules of that group the fast pattern did not add as + candidates at the hook state. It is a set of rules, not a range of progress states; + the states only say when the set is filled and when it is dropped. + +An LTE rule has no say below its hook, and at its hook the fast pattern decides which +of them are worth inspecting. The rules the pattern did not add hold the hook open: +they are not candidates, so the walk cannot decide them, but the default policy cannot +resolve the hook either. + +:: + + progress 0 ............ H (hook) C (a miss becomes final) + | | | + window | empty | filled | emptied here + (a set of | | | + rules) | | | + v v v + policy while the window is not empty the hook is left unresolved, so a match + anywhere else takes it and only an empty outcome falls through to the + default policy + +Two moments, for the rules of a group hooked at ``H``: + +* at ``H``: the pattern runs; matches go to the candidate list, the rules it did not + add are the window and the hook stays pending. +* at ``C``: a miss is final. The window contributes its last rule only, so that any + other match still wins the state and the policy decides when nothing does. + +``C`` is ``H + 1``, whatever the protocol: a hook buffer that fills in above ``H`` +(an http2 header buffer frame by frame, a stream buffer across segments) does +not reopen the window. A transaction that ends at or below ``H`` never opens a +window: its end state decides, and a window opens only for a tx that keeps +going past ``H``. diff --git a/src/detect-engine-prefilter.c b/src/detect-engine-prefilter.c index 6368382a00..1ce6719cc6 100644 --- a/src/detect-engine-prefilter.c +++ b/src/detect-engine-prefilter.c @@ -1388,6 +1388,7 @@ int PrefilterSetupRuleGroup(DetectEngineCtx *de_ctx, SigGroupHead *sgh) e->local_id = local_id++; e->alproto = el->alproto; e->ctx.app.tx_min_progress = el->tx_min_progress; + e->ctx.app.tx_max_progress = el->tx_max_progress; e->ctx.app.sub_state = el->sub_state; e->run_always = el->run_always; e->cb.PrefilterTx = el->PrefilterTx; diff --git a/src/detect.h b/src/detect.h index 605b54693a..e298c74825 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1620,6 +1620,9 @@ typedef struct PrefilterEngineList_ { /** Minimal Tx progress we need before running the engine. Only used * with Tx Engine. Set to -1 for all states. */ int8_t tx_min_progress; + /** Upper bound, exclusive: skip the engine once the tx reached it. + * 0 means no bound. */ + uint8_t tx_max_progress; uint8_t frame_type; @@ -1669,6 +1672,9 @@ typedef struct PrefilterEngine_ { /** Minimal Tx progress we need before running the engine. Only used * with Tx Engine. Set to -1 for all states. */ int8_t tx_min_progress; + /** Upper bound, exclusive: skip the engine once the tx reached it. + * 0 means no bound. */ + uint8_t tx_max_progress; uint8_t sub_state; } app; uint8_t frame_type; From 65c26a8ec359b6e9a204e7a77a5b27d5fa554c59 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sat, 10 Oct 2026 01:26:18 +0000 Subject: [PATCH 05/10] firewall: fast pattern decides LTE candidacy A "less than or equal" hook rule is only resolved once the transaction reaches its hook state, so nothing decides it there: the rule ends up in the non-prefilter list for every state from its hook upwards, which makes all of them candidates. A ruleset with a few thousand of these rules pays that per flow, in the thousands. Set up the fast pattern for these rules too. What is needed to keep the walk as it is: - A rule the pattern does not add is not final at its hook, as the buffer may still fill in, so the rules are registered for the states below the hook with an upper bound. Above the bound a group is retired once its last rule there has been inspected, which is legitimate only if no other rule of the group can match anymore. A buffer of the hook decides that: SigValidateCheckBuffers() requires the engines of an app hooked rule to sit at its hook, and a protocol which declares sub-states can rewrite a buffer above the hook, an http2 trailer HEADERS frame updating the header buffers being the case in point. A group which holds a rule with no fast pattern is inspected whole above the bound, as it is without it: nothing can bring such a rule back. - The rules the pattern did not add are missing from the list, while a list with all rules in it reaches the lowest of them before any rule that takes a higher id, and stops the walk there. Both the hook coverage and the walk take that rule into account, so a rule that matches cannot decide the transaction before a rule that is still in the running had its chance. A pending rule whose hook the transaction has moved past keeps its hook covered, which is what stops a default policy from deciding through it, and the candidates above it are inspected as they would be without the bound. An accept state which skips the firewall rules is consulted first, as it is for a rule in the list. The coverage count a pending rule borrows at walk start is kept rather than returned when the walk steps past it: the rule is still in the running, and a hook which reads as uncovered is exactly what lets a default policy decide. The counts are per walk, so none of them outlives it. The inspect engines of a LTE rule keep their order: the pass through engines cover the states below the hook and have to run before the buffer engine at the hook. They are not marked as an mpm engine either, as that would turn a miss above the hook into a no match for the whole list, while the own end of the buffer already knows when a rule is final. While a rule is pending at the current state no default policy may decide the transaction, the packet one included: this is not the packet to decide the flow. A rule whose packet match reads a variable - a flowbit, xbit, flowint, flowvar, pktvar or a lua test - is given no fast pattern at all. Its liveness cannot be settled before the rules below it in the walk have run, since one of them may set the bit it reads, so it stays a candidate in every update and is inspected where main would inspect it. Removes DetectLteRevisit(), added earlier in this stack: the candidate list above the bound already covers the buffer growth it was meant to catch, and keeping it would leave the whole engine carrying an HTTP/2-only sub-state hook and its buffers for a decision nothing uses. DetectTxCompleted() loses a signature argument it never read. Ticket: #9149. --- doc/userguide/firewall/firewall-design.rst | 54 +++- src/detect-engine-prefilter.c | 299 +++++++++++++++++++-- src/detect-engine-prefilter.h | 21 +- src/detect-engine.c | 63 ++--- src/detect-parse.c | 33 ++- src/detect.c | 234 ++++++++++++++-- src/detect.h | 17 +- src/tests/detect.c | 160 +++++++++++ 8 files changed, 774 insertions(+), 107 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 032e734818..678daee9dc 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -288,16 +288,28 @@ While such a rule is still pending - it ran the states before its hook without matching, but a buffer of its hook can still grow - a rule hooked at a higher state is still evaluated once the transaction has moved past the pending rule's hook; while the transaction is at or before that hook the walk stops, so no -higher state's policy can decide the flow early. A higher-hook rule inspects +higher state's policy can decide the flow early. A rule of a higher id at the +pending rule's own hook is evaluated too: stepping over the pending one must not +stall the hook, because a rule which can never resolve would otherwise hold it +open for the whole transaction. That is a choice, and its price is that a +drop:flow rule at that hook can decide before a pending accept resolves. +A higher-hook rule inspects the buffers of every state below its own hook, so its match covers the pending rule and its action decides the flow: when it matches, the pending rule's later -no-match does not apply the default policy of its own state. +no-match does not apply the default policy of its own state. Stepping over a rule +the transaction has moved past keeps that rule's accept for the packet, as a +listed partial match of it would: without it the packet default policy drops +packets of a flow which then ends up accepted. A sub-state's buffers can also grow after the transaction moved past the rule's -hook: a http2 trailer HEADERS frame updates the header buffer above the -``request_headers`` hook. The fast pattern at the hook decides regardless: above -it the group is retired, so the rule is not run again there. The default -policies of the states it covers are deferred by the window at the hook only. +hook: a http2 trailer HEADERS frame updates the header lists above the +``request_headers`` hook. That growth stops with the direction's own END_STREAM, +not with the transaction: a http2 stream only completes once both sides closed, +and holding a rule pending that long defers the default policy past the request. The fast pattern at the hook decides there: above it a +group is retired once its rules are, so a rule whose buffer is final at that +point is not run again. A rule whose pattern can still arrive later - a raw +stream ``content``, or a buffer that is not complete yet - keeps the window +filled, and with it the default policies of the states it covers. The bound is the transaction's end state. For a parser with per-direction sub- states that can be later than the direction's own close - a http2 stream @@ -489,11 +501,25 @@ Two moments, for the rules of a group hooked at ``H``: * at ``H``: the pattern runs; matches go to the candidate list, the rules it did not add are the window and the hook stays pending. -* at ``C``: a miss is final. The window contributes its last rule only, so that any - other match still wins the state and the policy decides when nothing does. - -``C`` is ``H + 1``, whatever the protocol: a hook buffer that fills in above ``H`` -(an http2 header buffer frame by frame, a stream buffer across segments) does -not reopen the window. A transaction that ends at or below ``H`` never opens a -window: its end state decides, and a window opens only for a tx that keeps -going past ``H``. +* at ``C``: a miss is final for the group's rules whose buffers are complete at + ``H``. The window contributes its last rule only, so that any other match still + wins the state and the policy decides when nothing does. A group that also holds + a rule whose pattern can still grow above ``C`` is not retired there, so the + retirement cannot apply a default policy through it. Tracking such a rule keeps + its hook covered while the transaction has moved past it, without ending the walk + there: a match at a higher rule still gets its inspection, and the rule keeps its + packet accept. When no candidate sits above it, the walk takes the pending rules + at its end, so they are accounted for either way. + +``C`` is ``H + 1`` for the app buffers of a protocol which declares no sub-states: an +app hooked rule can only hold patterns of buffers whose engine sits at its hook, so such +a buffer cannot grow once the transaction has moved past it. Two cases do grow: a raw +stream pattern, which arrives with a later segment, and an app buffer of a protocol +declaring sub-states, because a frame can rewrite it above the hook -- an http2 trailer +HEADERS frame updates the header buffers. The first still leaves the fast pattern to say +which update brings it, so the group keeps its window filled there; the second has no +pattern which covers that update, so all the rules of the group are inspected. A rule +with no fast pattern at all has no update which brings it, so a group which holds one is +inspected whole there as well. A +transaction that ends at or below ``H`` never opens a window: its end state decides, +and a window opens only for a tx that keeps going past ``H``. diff --git a/src/detect-engine-prefilter.c b/src/detect-engine-prefilter.c index 1ce6719cc6..a10c5a0027 100644 --- a/src/detect-engine-prefilter.c +++ b/src/detect-engine-prefilter.c @@ -49,6 +49,9 @@ #include "detect-engine.h" #include "detect-engine-prefilter.h" +#ifdef UNITTESTS +#include "util-unittest.h" +#endif #include "detect-engine-mpm.h" #include "detect-engine-frame.h" #include "detect-engine-uint.h" @@ -65,6 +68,22 @@ static int PrefilterStoreGetId(DetectEngineCtx *de_ctx, static const PrefilterStore *PrefilterStoreGetStore(const DetectEngineCtx *de_ctx, const uint32_t id); +enum FwLteGrows { + LTE_GROWS_NONE, /**< every pattern of the group is final above the hook */ + LTE_GROWS_LIST, /**< the buffer can be rewritten above the hook: inspect */ +}; + +struct PrefilterNonPFDataTx { + uint32_t size; + /** some rule of the group has a fast pattern in a buffer that keeps growing + * above the hook, so its miss is not final there. */ + enum FwLteGrows lte_grows; + uint32_t array[]; +}; + +static bool PrefilterLteWindowAdd( + DetectEngineThreadCtx *det_ctx, const struct PrefilterNonPFDataTx *data); + static inline void QuickSortSigIntId(SigIntId *sids, uint32_t n) { if (n < 2) @@ -89,20 +108,35 @@ static inline void QuickSortSigIntId(SigIntId *sids, uint32_t n) QuickSortSigIntId(l, (uint32_t)(sids + n - l)); } +/** LTE windows exist for firewall rules only: the `<` hook form is refused for any + * other rule, so `tx_max_progress` is set only when the engine groups LTE rules. + * Keep the dependency on the engine mode explicit rather than implicit. */ +static bool PrefilterEngineIsFwLteWindow(const PrefilterEngine *engine) +{ + /* tx_max_progress doubles as "no bound" and "hooked at progress 0": a rule at the + * very start has no state below its hook, so the plain run the base does already + * covers it and a window would only add a per-packet store. */ + if (engine->ctx.app.tx_max_progress == 0) + return false; + DEBUG_VALIDATE_BUG_ON(!EngineModeIsFirewall()); + return EngineModeIsFirewall(); +} + /** * \brief run prefilter engines on a transaction + * + * \retval true if an LTE pending window engine was skipped because the tx has + * reached the rules hook: the fast pattern decides there, and its miss + * is not final yet, so default policies must wait. */ -void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, - const SigGroupHead *sgh, - Packet *p, - const uint8_t ipproto, - const uint8_t flow_flags, - const AppProto alproto, - void *alstate, +bool DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, const SigGroupHead *sgh, Packet *p, + const uint8_t ipproto, const uint8_t flow_flags, const AppProto alproto, void *alstate, DetectTransaction *tx) { /* reset rule store */ det_ctx->pmq.rule_id_array_cnt = 0; + det_ctx->fw_lte_window_cnt = 0; + bool lte_at_hook = false; SCLogDebug("packet %" PRIu64 " tx %p id %" PRIu64 " progress %d tx->detect_progress %02x", PcapPacketCntGet(p), tx->tx_ptr, tx->tx_id, tx->tx_progress, tx->detect_progress); @@ -143,6 +177,39 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, /* if engine needs tx state to be higher, break out. */ if (engine->ctx.app.tx_min_progress > tx->tx_progress) break; + /* A pending window engine covers LTE rules that have their hook at + * `bound`. At that state the fast pattern decides which of them match, + * so the list is left to it; above it the group is retired on its last + * rule, unless a rule of it can still grow there. */ + const uint8_t bound = engine->ctx.app.tx_max_progress; + bool run = true; + if (PrefilterEngineIsFwLteWindow(engine) && tx->tx_progress >= bound) { + run = false; + /* only the pending windows carry a bound, so this is the + * non-prefilter data */ + const struct PrefilterNonPFDataTx *data = engine->pectx; + if (tx->tx_progress == bound && tx->tx_progress < tx->tx_end_state) { + /* the rules the pattern did not add may still match: keep track of + * the group so the walk can act as if it had them in its list */ + if (PrefilterLteWindowAdd(det_ctx, data)) { + lte_at_hook = true; + } else { + PrefilterAddSids(&det_ctx->pmq, data->array, data->size); + } + } else if (data->size > 0) { + /* The last rule can only retire the group if no other rule of it can + * match anymore. A protocol which declares sub-states rewrites a buffer + * above its hook - an http2 trailer HEADERS frame - so such a group is + * still live and its rules go in the list whole; a rule whose pattern + * can only arrive from the stream cannot load at an LTE hook (see + * DetectFirewallRuleValidate). */ + if (data->lte_grows == LTE_GROWS_LIST) { + PrefilterAddSids(&det_ctx->pmq, data->array, data->size); + } else { + PrefilterAddSids(&det_ctx->pmq, &data->array[data->size - 1], 1); + } + } + } if (tx->tx_progress > engine->ctx.app.tx_min_progress) { SCLogDebug("tx->tx_progress %u > engine->ctx.app.tx_min_progress %d", tx->tx_progress, engine->ctx.app.tx_min_progress); @@ -163,10 +230,12 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, #ifdef DEBUG uint32_t old = det_ctx->pmq.rule_id_array_cnt; #endif - PREFILTER_PROFILING_START(det_ctx); - engine->cb.PrefilterTx(det_ctx, engine->pectx, p, p->flow, tx_ptr, tx->tx_id, - tx->tx_data_ptr, flow_flags); - PREFILTER_PROFILING_END(det_ctx, engine->gid); + if (run) { + PREFILTER_PROFILING_START(det_ctx); + engine->cb.PrefilterTx(det_ctx, engine->pectx, p, p->flow, tx_ptr, tx->tx_id, + tx->tx_data_ptr, flow_flags); + PREFILTER_PROFILING_END(det_ctx, engine->gid); + } SCLogDebug("engine %p min_progress %d %s:%s: results %u", engine, engine->ctx.app.tx_min_progress, AppProtoToString(engine->alproto), pname, det_ctx->pmq.rule_id_array_cnt - old); @@ -210,6 +279,7 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, QuickSortSigIntId(det_ctx->pmq.rule_id_array, det_ctx->pmq.rule_id_array_cnt); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_PF_SORT1); } + return lte_at_hook; } /** \brief invoke post-rule match "prefilter" engines @@ -597,11 +667,6 @@ struct PrefilterNonPFData { struct PrefilterNonPFDataSig array[]; }; -struct PrefilterNonPFDataTx { - uint32_t size; - uint32_t array[]; -}; - /** \internal * \brief wrapper for use in APIs */ static void PrefilterNonPFDataFree(void *data) @@ -617,6 +682,113 @@ static void PrefilterTxNonPF(DetectEngineThreadCtx *det_ctx, const void *pectx, PrefilterAddSids(&det_ctx->pmq, data->array, data->size); } +static int PrefilterNonPFDataSortU32(const void *a, const void *b) +{ + const uint32_t x = *(const uint32_t *)a, y = *(const uint32_t *)b; + return x < y ? -1 : (x > y ? 1 : 0); +} + +/** \brief keep track of a pending window that went silent at its bound. The number + * of groups bound at a state is set by the ruleset, so the array grows to + * fit: a window that could not be tracked would leave the walk unable to + * tell which rule is still pending. + */ +static bool PrefilterLteWindowAdd( + DetectEngineThreadCtx *det_ctx, const struct PrefilterNonPFDataTx *data) +{ + for (uint32_t w = 0; w < det_ctx->fw_lte_window_cnt; w++) { + /* the engines of one group can share a set of rules, and the window is + * merged against itself: a duplicate only costs the merge */ + if (det_ctx->fw_lte_windows[w] == data) { + return true; + } + } + if (det_ctx->fw_lte_window_cnt == det_ctx->fw_lte_window_size) { + const uint32_t size = + det_ctx->fw_lte_window_size == 0 ? 8 : det_ctx->fw_lte_window_size * 2; + void *ptmp = SCRealloc((void *)det_ctx->fw_lte_windows, size * sizeof(void *)); + if (ptmp == NULL) { + /* out of memory: tell the caller to keep the group as candidates + * instead of killing the engine on the packet path */ + return false; + } + det_ctx->fw_lte_windows = (const void **)ptmp; + det_ctx->fw_lte_window_size = size; + } + det_ctx->fw_lte_windows[det_ctx->fw_lte_window_cnt++] = data; + return true; +} + +void DetectPrefilterLteWindowFree(DetectEngineThreadCtx *det_ctx) +{ + SCFree((void *)det_ctx->fw_lte_windows); + det_ctx->fw_lte_windows = NULL; + det_ctx->fw_lte_window_size = 0; + det_ctx->fw_lte_window_cnt = 0; +} + +static uint32_t LteArrayLowerBound(const uint32_t *a, const uint32_t n_, const uint32_t v) +{ + uint32_t lo = 0, hi = n_; + while (lo < hi) { + const uint32_t mid = lo + (hi - lo) / 2; + if (a[mid] < v) + lo = mid + 1; + else + hi = mid; + } + return lo; +} + +static uint32_t LteCandidateLowerBound( + const RuleMatchCandidateTx *cands, const uint32_t cand_cnt, const uint32_t v) +{ + uint32_t lo = 0, hi = cand_cnt; + while (lo < hi) { + const uint32_t mid = lo + (hi - lo) / 2; + if (cands[mid].id < v) + lo = mid + 1; + else + hi = mid; + } + return lo; +} + +/** \brief find the lowest rule of a silenced LTE pending window that is not a + * candidate: the fast pattern did not add it, but the tx is not final at + * the hook, so it may still match. A walk that had all rules in it would + * inspect this one before any rule with a higher iid. + * Both arrays are in iid order, so the search enters them at the cursor. + */ +static uint32_t LteWindowFirstMissing(const struct PrefilterNonPFDataTx *data, + const RuleMatchCandidateTx *cands, const uint32_t cand_cnt, const uint32_t min_iid) +{ + /* both arrays are sorted, so enter them where min_iid lies rather than at the + * start: a caller that retries past rejected rules would otherwise rewalk + * everything below the cursor on every retry. */ + uint32_t i = LteArrayLowerBound(data->array, data->size, min_iid); + while (i < data->size) { + const uint32_t iid = data->array[i]; + const uint32_t j = LteCandidateLowerBound(cands, cand_cnt, iid); + if (j == cand_cnt || cands[j].id != iid) + return iid; + i++; + } + return UINT32_MAX; +} + +uint32_t DetectPrefilterLtePendingIid( + DetectEngineThreadCtx *det_ctx, const uint32_t cand_cnt, const uint32_t min_iid) +{ + uint32_t lowest = UINT32_MAX; + for (uint32_t i = 0; i < det_ctx->fw_lte_window_cnt; i++) { + const struct PrefilterNonPFDataTx *data = det_ctx->fw_lte_windows[i]; + lowest = + MIN(lowest, LteWindowFirstMissing(data, det_ctx->tx_candidates, cand_cnt, min_iid)); + } + return lowest; +} + #ifdef NONPF_PKT_STATS static thread_local uint64_t prefilter_pkt_nonpf_called = 0; static thread_local uint64_t prefilter_pkt_nonpf_mask_fail = 0; @@ -750,6 +922,11 @@ struct TxNonPFData { * update (engine progress -1) so a provisional miss can be revisited as * the transaction advances. */ bool run_always; + /** LTE pending window bound: the engine goes silent once the tx reaches this + * hook, as the fast pattern decides there. 0 means always add the sids. */ + uint8_t lte_hook; + /** like #lte_hook, but for the group: #FwLteGrows of its rules */ + enum FwLteGrows lte_grows; uint32_t sigs_cnt; struct PrefilterNonPFDataSig *sigs; const char *engine_name; /**< pointer to name owned by DetectEngineCtx::non_pf_engine_names */ @@ -758,7 +935,8 @@ struct TxNonPFData { static uint32_t TxNonPFHash(HashListTable *h, void *data, uint16_t _len) { struct TxNonPFData *d = data; - return (d->alproto + d->sub_state + d->progress + d->dir + d->sig_list + d->run_always) % + return (d->alproto + d->sub_state + d->progress + d->dir + d->sig_list + d->run_always + + d->lte_hook) % h->array_size; } @@ -768,7 +946,7 @@ static char TxNonPFCompare(void *data1, uint16_t _len1, void *data2, uint16_t le struct TxNonPFData *d2 = data2; return d1->alproto == d2->alproto && d1->sub_state == d2->sub_state && d1->progress == d2->progress && d1->dir == d2->dir && d1->sig_list == d2->sig_list && - d1->run_always == d2->run_always; + d1->run_always == d2->run_always && d1->lte_hook == d2->lte_hook; } static void TxNonPFFree(void *data) @@ -778,9 +956,30 @@ static void TxNonPFFree(void *data) SCFree(d); } +/** \internal \brief does the rule's fast pattern keep growing above its hook? + * An app hooked rule can only hold patterns from buffers whose engine sits at + * the hook progress (`SigValidateCheckBuffers`), so such a buffer is complete + * once the tx is past the hook and a miss there is final. Two cases escape + * that: a raw stream pattern has no hook tied buffer and completes with the tx, + * and a protocol which declares sub-states rewrites a buffer above its hook - + * an http2 trailer HEADERS frame updates the header buffers. */ +static enum FwLteGrows FwLtePatternGrowsAboveBound(const Signature *s) +{ + if (s->init_data->mpm_sm == NULL) { + /* no fast pattern at all: nothing can bring the rule back into the + * candidates above the bound, so the group cannot be retired there */ + return LTE_GROWS_LIST; + } + if (AppLayerParserSupportsSubStates(s->alproto)) { + return LTE_GROWS_LIST; /* a frame rewrites the buffer above the hook */ + } + return LTE_GROWS_NONE; +} + static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash, const AppProto alproto, const uint8_t sub_state, const int dir, const uint8_t progress, - const int sig_list, const char *name, const Signature *s, const bool run_always) + const int sig_list, const char *name, const Signature *s, const bool run_always, + const uint8_t lte_hook) { const uint32_t max_sids = DetectEngineGetMaxSigId(de_ctx); @@ -791,12 +990,16 @@ static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash .progress = progress, .sig_list = sig_list, .run_always = run_always, + .lte_hook = lte_hook, .sigs_cnt = 0, .sigs = NULL, .engine_name = NULL, }; + const enum FwLteGrows grows = lte_hook != 0 ? FwLtePatternGrowsAboveBound(s) : LTE_GROWS_NONE; + struct TxNonPFData *e = HashListTableLookup(tx_engines_hash, &lookup, 0); if (e != NULL) { + e->lte_grows = MAX(e->lte_grows, grows); bool found = false; // avoid adding same sid multiple times for (uint32_t y = 0; y < e->sigs_cnt; y++) { @@ -824,6 +1027,8 @@ static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash add->progress = progress; add->sig_list = sig_list; add->run_always = run_always; + add->lte_hook = lte_hook; + add->lte_grows = grows; add->sigs = SCCalloc(max_sids, sizeof(struct PrefilterNonPFDataSig)); if (add->sigs == NULL) { SCFree(add); @@ -939,11 +1144,17 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) if (s == NULL) continue; SCLogDebug("checking sid %u for non-prefilter", s->id); + /* pf_hook: the rules hook state is covered by a fast pattern or a keyword + * prefilter engine, so the rule needs no non-prefilter entry there. */ + bool pf_hook = false; if (s->init_data->mpm_sm != NULL && (s->flags & SIG_FLAG_MPM_NEG) == 0) - continue; + pf_hook = true; if (s->init_data->prefilter_sm != NULL) - continue; + pf_hook = true; if ((s->flags & (SIG_FLAG_PREFILTER | SIG_FLAG_MPM_NEG)) == SIG_FLAG_PREFILTER) + pf_hook = true; + const bool lte = (s->flags & SIG_FLAG_FW_HOOK_LTE) != 0; + if (pf_hook && !lte) continue; SCLogDebug("setting up sid %u for non-prefilter", s->id); @@ -994,6 +1205,14 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) * hook 0 it could have a preceding rule that makes sure this sig isn't triggered, but then * for hook 1 we would need to be called. */ if (s->flags & SIG_FLAG_FW_HOOK_LTE) { + /* With a fast pattern the pattern engine is the only one that may drop + * rules that do not match, so the hook state is left to it: the window + * covers the states below the hook and the engine bound takes care of + * the hook and the states above it. Without one the rule needs to be in + * the list at its hook, as it has always been. A hook of 0 has no state + * below it to defer, and gating it would leave the rule with neither a + * window nor an engine. */ + const bool gated = pf_hook && s->app_progress_hook > 0; for (uint8_t state = 0; state < s->app_progress_hook; state++) { SCLogDebug("handle HOOK %u LTE", state); const int dir = (s->flags & SIG_FLAG_TOSERVER) ? 0 : 1; @@ -1007,11 +1226,17 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) const int sm_list = DetectEngineAppHookToSmlist(s->alproto, sub_state, state, direction); if (TxNonPFAddSig(de_ctx, tx_engines_hash, s->alproto, sub_state, dir, state, - sm_list, pname, s, false) != 0) { + sm_list, pname, s, false, gated ? s->app_progress_hook : 0) != 0) { goto error; } tx_non_pf = true; } + if (gated) { + /* skip the buffer loop: it would add the rule at the hook + * regardless of what the pattern engine made of it */ + SCLogDebug("sid %u: LTE pending window only", s->id); + continue; + } } for (uint32_t x = 0; x < s->init_data->buffer_index; x++) { @@ -1082,7 +1307,7 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) const bool run_always = buf != NULL && buf->run_always; const uint8_t sub_state = app->sub_state; if (TxNonPFAddSig(de_ctx, tx_engines_hash, app->alproto, sub_state, app->dir, - app->progress, sig_list, buf->name, s, run_always) != 0) { + app->progress, sig_list, buf->name, s, run_always, 0) != 0) { goto error; } tx_non_pf = true; @@ -1105,7 +1330,7 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) uint8_t sub_state = s->init_data->hook.t.app.sub_state; if (TxNonPFAddSig(de_ctx, tx_engines_hash, s->alproto, sub_state, dir, s->init_data->hook.t.app.app_progress, s->init_data->hook.sm_list, pname, s, - false) != 0) { + false, 0) != 0) { goto error; } tx_non_pf = true; @@ -1196,22 +1421,35 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh) if (data == NULL) goto error; data->size = t->sigs_cnt; + data->lte_grows = t->lte_grows; for (uint32_t i = 0; i < t->sigs_cnt; i++) { data->array[i] = t->sigs[i].sid; } + if (t->lte_hook) { + /* DetectPrefilterLtePendingIid merges the window against the sorted + * candidates, so the window has to be in iid order. */ + qsort(data->array, data->size, sizeof(data->array[0]), PrefilterNonPFDataSortU32); + } if (PrefilterAppendTxEngineSubState(de_ctx, sgh, PrefilterTxNonPF, t->alproto, t->sub_state, engine_progress, (void *)data, PrefilterNonPFDataFree, t->engine_name) < 0) { SCFree(data); goto error; } - if (t->run_always) { - /* A stateful keyword must be revisited as the tx advances. Keep - * its real progress (so the progress bookkeeping runs and the - * proto-agnostic -1 invariant holds) and carry an explicit flag. */ + if (t->run_always || t->lte_hook) { PrefilterEngineList *tail = sgh->init->tx_engines; while (tail->next != NULL) tail = tail->next; - tail->run_always = true; + if (t->run_always) { + /* A stateful keyword must be revisited as the tx advances. Keep + * its real progress (so the progress bookkeeping runs and the + * proto-agnostic -1 invariant holds) and carry an explicit flag. */ + tail->run_always = true; + } + if (t->lte_hook) { + /* LTE pending window: stop adding the sids once the tx reached + * the rules hook, where the fast pattern takes over. */ + tail->tx_max_progress = t->lte_hook; + } } } HashListTableFree(tx_engines_hash); @@ -1906,3 +2144,6 @@ void PostRuleMatchWorkQueueAppend( det_ctx->post_rule_work_queue.len++; SCLogDebug("det_ctx->post_rule_work_queue.len %u", det_ctx->post_rule_work_queue.len); } + +#ifdef UNITTESTS +#endif /* UNITTESTS */ diff --git a/src/detect-engine-prefilter.h b/src/detect-engine-prefilter.h index 662a518042..da9e5095f7 100644 --- a/src/detect-engine-prefilter.h +++ b/src/detect-engine-prefilter.h @@ -80,13 +80,16 @@ int PrefilterAppendPostRuleEngine(DetectEngineCtx *de_ctx, SigGroupHead *sgh, DetectEngineThreadCtx *det_ctx, const void *pectx, Packet *p, Flow *f), void *pectx, void (*FreeFunc)(void *pectx), const char *name); -void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, - const SigGroupHead *sgh, - Packet *p, - const uint8_t ipproto, - const uint8_t flow_flags, - const AppProto alproto, - void *alstate, +void DetectPrefilterLteWindowFree(DetectEngineThreadCtx *det_ctx); + +/** \brief the iid of the lowest pending LTE rule at or above \a min_iid that the + * fast pattern did not add, UINT32_MAX if there is none + */ +uint32_t DetectPrefilterLtePendingIid( + DetectEngineThreadCtx *det_ctx, uint32_t cand_cnt, uint32_t min_iid); + +bool DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx, const SigGroupHead *sgh, Packet *p, + const uint8_t ipproto, const uint8_t flow_flags, const AppProto alproto, void *alstate, DetectTransaction *tx); void PrefilterFreeEnginesList(PrefilterEngineList *list); @@ -114,4 +117,8 @@ void PostRuleMatchWorkQueueAppend( void PrefilterPktNonPFStatsDump(void); +#ifdef UNITTESTS +void DetectPrefilterRegisterTests(void); +#endif + #endif diff --git a/src/detect-engine.c b/src/detect-engine.c index 2188bda40d..1227c75ec2 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -663,8 +663,13 @@ static void AppendFrameInspectEngine(DetectEngineCtx *de_ctx, } if (mpm_list == u->sm_list) { SCLogDebug("%s is mpm", DetectEngineBufferTypeGetNameById(de_ctx, u->sm_list)); - prepend = true; - new_engine->mpm = true; + /* defensive only: the frame path has no mpm finality handling in the walk, so + * the flag would just reorder this engine ahead of the others. Keep the LTE + * rule in progress order like the stream and app engines. */ + if ((s->flags & SIG_FLAG_FW_HOOK_LTE) == 0) { + prepend = true; + new_engine->mpm = true; + } } new_engine->type = u->type; @@ -772,9 +777,16 @@ static void AppendAppInspectEngine(DetectEngineCtx *de_ctx, bool prepend = false; if (mpm_list == t->sm_list) { SCLogDebug("%s is mpm", DetectEngineBufferTypeGetNameById(de_ctx, t->sm_list)); - prepend = true; - *head_is_mpm = true; - new_engine->mpm = true; + /* An LTE rule must keep its engines in progress order: the pass through + * engines cover the states below the hook, and a not yet reached hook has + * to stay a partial match instead of breaking out as a no match. The mpm + * flag is left off as well: it would make a no match above the hook final + * (`mpm_before_progress`), while LTE rules rely on the buffers own eof. */ + if ((s->flags & SIG_FLAG_FW_HOOK_LTE) == 0) { + prepend = true; + *head_is_mpm = true; + new_engine->mpm = true; + } } new_engine->alproto = t->alproto; @@ -2124,35 +2136,18 @@ int DetectEngineReloadIsIdle(void) return r; } -/** \internal - * \brief can the rule still match in a later state of its sub-state? - * - * An LTE rule is a candidate below its hook, so a miss is not final while a - * sub-state can still grow the buffer the engine reads: a http2 trailer - * updates the header buffer above the rule's hook, and the tx only reaches - * its end state after that. - */ -static bool DetectLteRevisit( - Flow *f, void *txv, uint8_t flags, const Signature *s, const int progress) -{ - if ((s->flags & SIG_FLAG_FW_HOOK_LTE) == 0 || !AppLayerParserSupportsSubStates(f->alproto)) { - return false; - } - return progress < AppLayerParserGetTxEndState(f->proto, f->alproto, txv, flags); -} - /** \internal * \brief is the engine's data final for this tx? * - * Past the engine's phase the answer is yes, unless the rule is an LTE rule - * that can still match in a later state of its sub-state (DetectLteRevisit()). - * At or beyond the tx end state it is final too, even for engines registered - * at the completion state (no P+1). AppLayerParserGetStateProgress() returns + * Past the engine's phase the answer is yes: a rule hooked below is a candidate + * in the list at those updates, so it is inspected there without needing the + * engine kept open. At or beyond the tx end state it is final too, even for + * engines registered at the completion state (no P+1). AppLayerParserGetStateProgress() returns * the end progress for disrupted flows, so progress == end stays a valid * finality signal there. */ -static bool DetectTxCompleted(Flow *f, void *txv, uint8_t flags, - const DetectEngineAppInspectionEngine *engine, const Signature *s) +static bool DetectTxCompleted( + Flow *f, void *txv, uint8_t flags, const DetectEngineAppInspectionEngine *engine) { if (f->alproto == ALPROTO_DOH2 && engine->alproto == ALPROTO_DOH2) { // the DNS tx from DetectGetInnerTx is always complete @@ -2163,9 +2158,6 @@ static bool DetectTxCompleted(Flow *f, void *txv, uint8_t flags, return false; } if (progress > engine->progress) { - if (DetectLteRevisit(f, txv, flags, s, progress)) { - return false; - } return true; } if (progress < engine->progress) { @@ -2195,7 +2187,7 @@ uint8_t DetectEngineInspectGenericList(DetectEngineCtx *de_ctx, DetectEngineThre if ((sigmatch_table[smd->type].flags & SIGMATCH_STATEFUL) != 0) { return DETECT_ENGINE_INSPECT_SIG_NO_MATCH; } - return DetectTxCompleted(f, txv, flags, engine, s) + return DetectTxCompleted(f, txv, flags, engine) ? DETECT_ENGINE_INSPECT_SIG_CANT_MATCH : DETECT_ENGINE_INSPECT_SIG_NO_MATCH; } @@ -2233,7 +2225,7 @@ uint8_t DetectEngineInspectBufferSingle(DetectEngineCtx *de_ctx, DetectEngineThr const int list_id = engine->sm_list; SCLogDebug("running inspect on %d", list_id); - const bool eof = DetectTxCompleted(f, txv, flags, engine, s); + const bool eof = DetectTxCompleted(f, txv, flags, engine); SCLogDebug("list %d mpm? %s transforms %p", engine->sm_list, engine->mpm ? "true" : "false", engine->v2.transforms); @@ -2293,7 +2285,7 @@ uint8_t DetectEngineInspectBufferGeneric(DetectEngineCtx *de_ctx, DetectEngineTh const int list_id = engine->sm_list; SCLogDebug("running inspect on %d", list_id); - const bool eof = DetectTxCompleted(f, txv, flags, engine, s); + const bool eof = DetectTxCompleted(f, txv, flags, engine); SCLogDebug("list %d mpm? %s transforms %p", engine->sm_list, engine->mpm ? "true" : "false", engine->v2.transforms); @@ -2427,7 +2419,7 @@ uint8_t DetectEngineInspectMultiBufferGeneric(DetectEngineCtx *de_ctx, } while (1); if (local_id == 0) { // That means we did not get even one buffer value from the multi-buffer - const bool eof = DetectTxCompleted(f, txv, flags, engine, s); + const bool eof = DetectTxCompleted(f, txv, flags, engine); if (eof && engine->match_on_null) { return DETECT_ENGINE_INSPECT_SIG_MATCH; } @@ -3865,6 +3857,7 @@ static void DetectEngineThreadCtxFree(DetectEngineThreadCtx *det_ctx) SCFree(det_ctx->replace); RuleMatchCandidateTxArrayFree(det_ctx); + DetectPrefilterLteWindowFree(det_ctx); AlertQueueFree(det_ctx); diff --git a/src/detect-parse.c b/src/detect-parse.c index 045f660f04..9888ea5fb2 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -2777,14 +2777,43 @@ SigMatchData* SigMatchList2DataArray(SigMatch *head) extern int g_skip_prefilter; +/** \internal + * \brief does a packet condition of the rule read a variable that a rule + * matching earlier on the same packet can set + * + * The list is the variable readers, not only those a firewall rule can use today. + */ +static bool SigPacketMatchReadsVars(const Signature *s) +{ + for (const SigMatch *sm = s->init_data->smlists[DETECT_SM_LIST_MATCH]; sm != NULL; + sm = sm->next) { + switch (sm->type) { + case DETECT_FLOWBITS: + case DETECT_XBITS: + case DETECT_HOSTBITS: + case DETECT_FLOWINT: + case DETECT_FLOWVAR: + case DETECT_PKTVAR: + case DETECT_LUA: + return true; + } + } + return false; +} + static void SigSetupPrefilter(DetectEngineCtx *de_ctx, Signature *s) { SCEnter(); SCLogDebug("s %u: set up prefilter/mpm", s->id); DEBUG_VALIDATE_BUG_ON(s->init_data->mpm_sm != NULL); - if (s->flags & SIG_FLAG_FW_HOOK_LTE) { - SCLogDebug("no prefilter for SIG_FLAG_FW_HOOK_LTE sig"); + /* An LTE rule that the prefilter does not add at its hook is pending, and the walk + * decides at its start whether that rule is still in the running + * (DetectFwLtePendingRule()). A variable that a rule earlier in the same walk sets + * is only known when the walk reaches the rule, so a rule that reads one gets no + * prefilter: it is a candidate on every update and inspected in order. */ + if ((s->flags & SIG_FLAG_FW_HOOK_LTE) && SigPacketMatchReadsVars(s)) { + SCLogDebug("%u: no prefilter for a SIG_FLAG_FW_HOOK_LTE sig reading variables", s->id); SCReturn; } diff --git a/src/detect.c b/src/detect.c index be63014841..d5fe14b5cc 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1476,6 +1476,11 @@ static int DetectRunTxInspectRule(ThreadVars *tv, DetectEngineCtx *de_ctx, file_no_match = 1; } /* implied DETECT_ENGINE_INSPECT_SIG_NO_MATCH */ + /* LTE rules are in the prefilter fast pattern but carry no + * engine->mpm, so this arm cannot fire for them. Their candidacy + * comes from the pattern instead, and above the retirement bound + * the whole group goes into the list, so a pre-progress miss is + * still retested there. */ if (engine->mpm && mpm_before_progress) { inspect_flags |= DE_STATE_FLAG_SIG_CANT_MATCH; inspect_flags |= BIT_U32(engine->id); @@ -1741,6 +1746,16 @@ struct DetectFirewallAppTxState { * rules cover it (their own hook and every prior hook). Progress values * stay under APP_LAYER_MAX_PROGRESS. */ bool fw_lte_cover_active; + /** An LTE rule is pending at the tx's current hook: no default policy may + * decide the packet yet, as the rule may still match as data arrives. */ + bool fw_defer_defaults; + /** LTE rule the fast pattern could not add at its hook while the tx is not + * final there. A walk that has all rules in it reaches this one before any + * rule of a higher iid, and breaks the walk at it. */ + const Signature *fw_lte_pending; + /** iid of the pending rule the walk credited in fw_lte_cover, or UINT32_MAX: only the + * head of the chain borrowed a count, so only it has to give one back. */ + uint32_t fw_lte_credited_iid; }; /** \internal @@ -1864,6 +1879,66 @@ static inline void DetectFwCountAppendedLteRule(DetectEngineThreadCtx *det_ctx, det_ctx->tx_candidates[can_idx].fw_lte_header_ok = true; } +/** \internal + * \brief would a full transaction walk still consider this rule on this packet + * + * Runs the filters `DetectRunTxInspectRule()` applies before a rule can hold a + * hook, so a rule the walk would drop here cannot defer defaults or mask a higher + * id. Ordered by cost: scope, then the proto check that stream mpm and negated mpm + * signatures can fail, then the packet engines. + */ +static bool DetectFwLteRuleStillLive(ThreadVars *tv, DetectEngineCtx *de_ctx, + DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f, const Signature *s) +{ + if (!DetectRunInspectRuleHeader(p, f, s, s->flags)) { + return false; + } + if (!(AppProtoEquals(s->alproto, f->alproto) || s->alproto == ALPROTO_UNKNOWN)) { + return false; + } + return DetectEnginePktInspectionRun(tv, det_ctx, s, f, p, NULL); +} + +/** \internal + * \brief the lowest LTE rule the fast pattern did not add at this state + * \note a rule that is out of the flows scope is no barrier: the walk would + * inspect it, get a header no match and carry on, so the search moves to the + * next pending rule + * \note the filters a walk runs before a rule can hold the hook apply here too, or + * a rule it would drop on this packet defers defaults and masks higher ids. + * Such a rule is never in the candidates, so this is not work already done. + * \note a rule resolved in an earlier pass cannot come out of here either. Resolved + * state lives in the tx store and every store item becomes a candidate in + * `DetectRunTx()`, while the search only yields iids missing from the candidate + * array. The other way in, a fast pattern in a state below the hook, is refused + * at load: `SigValidateCheckBuffers()` requires every same direction buffer + * engine of an app hooked rule to sit at the hook progress, so none of its + * engines can run and CANT before the hook. `DetectFwBuildLteCoverage()` tests + * the store flags because it walks the candidates; this walks their complement. + * \note a stale engine bit cannot decide a firewall outcome on either path: the state + * clear for a new file leaves the engine bits set but drops the resolution + * flags, which only ever moves a rule back into "not yet resolved". + */ +static const Signature *DetectFwLtePendingRule(ThreadVars *tv, DetectEngineCtx *de_ctx, + DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f, const uint32_t cand_cnt, + const uint32_t min_iid) +{ + uint32_t next = min_iid; + while (true) { + const uint32_t iid = DetectPrefilterLtePendingIid(det_ctx, cand_cnt, next); + if (iid == UINT32_MAX) { + return NULL; + } + const Signature *s = de_ctx->sig_array[iid]; + if ((s->flags & (SIG_FLAG_FW_HOOK_LTE | SIG_FLAG_FIREWALL)) == + (SIG_FLAG_FW_HOOK_LTE | SIG_FLAG_FIREWALL) && + DetectFwLteRuleStillLive(tv, de_ctx, det_ctx, p, f, s)) { + return s; + } + next = iid + 1; + } +} + /** \internal * \brief remove an LTE rule that resolved definitively from the hook coverage. */ @@ -2000,10 +2075,21 @@ static struct DetectFirewallPolicy DetectFirewallApplyDefaultAppPolicy( static enum DetectTxFirewallFlowControl DetectFirewallApplyDefaultPolicies( DetectEngineThreadCtx *det_ctx, const struct DetectFirewallPolicies *policies, DetectTransaction *tx, Packet *p, const AppProto alproto, const uint8_t direction, - const uint8_t start_hook, const uint8_t end_hook) + const uint8_t start_hook, const uint8_t end_hook, const bool defer) { DEBUG_VALIDATE_BUG_ON(start_hook > end_hook); + if (defer) { + SCLogDebug("LTE rule pending at the current hook: no default policy yet"); + /* The rule is still in the running, so this packet is not the one to + * decide the flow: keep it, same as a rule that was inspected but is + * not final yet. */ + if (tx->is_last) { + DetectRunAppendDefaultAccept(det_ctx, p); + } + return DETECT_TX_FW_FC_OK; + } + const bool need_verdict = tx->is_last && (end_hook == tx->tx_end_state || end_hook == tx->tx_progress); SCLogDebug("need_verdict:%s is_last:%s end_hook:%u tx->tx_end_state:%u tx->progress: %u", @@ -2147,9 +2233,10 @@ static enum DetectTxFirewallFlowControl DetectRunTxPreCheckFirewallPolicy( s->app_progress_hook, tx->detect_progress, tx->detect_progress_orig); /* if this rule was after the state we expected meaning that there are * no rules for that state. Invoke the default policies. */ - enum DetectTxFirewallFlowControl r = DetectFirewallApplyDefaultPolicies(det_ctx, - det_ctx->de_ctx->fw_policies, tx, p, s->alproto, direction, - tx->detect_progress_orig, s->app_progress_hook - 1); + enum DetectTxFirewallFlowControl r = + DetectFirewallApplyDefaultPolicies(det_ctx, det_ctx->de_ctx->fw_policies, tx, p, + s->alproto, direction, tx->detect_progress_orig, + s->app_progress_hook - 1, fw_state->fw_defer_defaults); if (r != DETECT_TX_FW_FC_OK) { /* both SKIP and BREAK mean: no more fw rules to inspect. * SKIP applies to just this TX. @@ -2322,9 +2409,9 @@ static void DetectRunTxFirewallApplyAccept(DetectEngineThreadCtx *det_ctx, Packe const uint8_t last_hook = fw_state->last_fw_rule ? tx->tx_progress : MIN(tx->tx_progress, s->app_progress_hook + 1); - enum DetectTxFirewallFlowControl r = - DetectFirewallApplyDefaultPolicies(det_ctx, det_ctx->de_ctx->fw_policies, tx, p, - s->alproto, direction, s->app_progress_hook + 1, last_hook); + enum DetectTxFirewallFlowControl r = DetectFirewallApplyDefaultPolicies(det_ctx, + det_ctx->de_ctx->fw_policies, tx, p, s->alproto, direction, + s->app_progress_hook + 1, last_hook, fw_state->fw_defer_defaults); if (r == DETECT_TX_FW_FC_BREAK) { fw_state->fw_skip_app_filter = true; return; @@ -2354,7 +2441,8 @@ static void DetectRunTxFirewallApplyAccept(DetectEngineThreadCtx *det_ctx, Packe * \retval 0 ok, continue as normal. No policies applied. */ static int DetectTxFirewallNoRulesApplyPolicies(DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f, - DetectTransaction *tx, const AppProto alproto, const uint8_t flow_flags, const int rule_cnt) + DetectTransaction *tx, const AppProto alproto, const uint8_t flow_flags, const int rule_cnt, + const bool defer) { /* if there are no rules / rule candidates, handling invoking the default * policy. */ @@ -2388,7 +2476,7 @@ static int DetectTxFirewallNoRulesApplyPolicies(DetectEngineThreadCtx *det_ctx, enum DetectTxFirewallFlowControl r = DetectFirewallApplyDefaultPolicies(det_ctx, det_ctx->de_ctx->fw_policies, tx, p, alproto, flow_flags & (STREAM_TOSERVER | STREAM_TOCLIENT), - tx->detect_progress_orig, tx->tx_progress); + tx->detect_progress_orig, tx->tx_progress, defer); SCLogDebug("r %u", r); if (r == DETECT_TX_FW_FC_BREAK) return 1; @@ -2477,6 +2565,63 @@ static int DetectRunTxFirewallRulePartialMatch( return 0; } +/** \internal + * \brief behave as a walk with all rules in it would before it reaches iid `below` + * + * "below" and "above" are the iid space: the position in the sorted signature list, + * which is the order the walk inspects in. Not the order of the rules in the file. + * + * The rules the fast pattern did not add are pending, and more than one of them can + * sit below the same candidate: the walk has to reach each in iid order, not only the + * lowest, or a pending accept loses its priority over the rules above it. + * + * \retval 1 the walk has to end here + * \retval 0 the walk continues, *pending is the first rule above `below` or none + */ +static int DetectFwLtePendingThrough(ThreadVars *tv, DetectEngineCtx *de_ctx, + DetectEngineThreadCtx *det_ctx, const Signature **pending, Packet *p, Flow *f, + DetectTransaction *tx, struct DetectFirewallAppTxState *fw_state, const uint32_t cand_cnt, + const uint32_t below, uint32_t *credited_iid) +{ + /* a pending rule is a firewall rule: the walk's skip state applies to it too, + * or running it here would end the walk and cost the candidates above it + * their inspection, threat detection included. `DetectRunTxPreCheckFirewall- + * Policy()` is what a listed rule would go through. */ + const bool skip_firewall = fw_state->fw_skip_app_filter || + (p->flow->flags & FLOW_ACTION_ACCEPT) != 0 || + (tx->tx_data_ptr->flags & APP_LAYER_TX_ACCEPT) != 0; + while (*pending != NULL && (*pending)->iid < below) { + bool skip = skip_firewall; + /* skip_fw_hook ends at the first rule it does not skip; a rule already + * skipped by the accept state is not that rule */ + if (fw_state->skip_fw_hook && !skip) { + if ((*pending)->app_progress_hook <= fw_state->skip_before_progress) + skip = true; + else + fw_state->skip_fw_hook = false; + } + if (!skip && DetectRunTxFirewallRulePartialMatch(det_ctx, *pending, tx, p) == 1) { + return 1; + } + if ((*pending)->iid == *credited_iid) { + /* out of the running: the count its hooks borrowed at walk start goes + * back. fw_lte_cover is read by DetectFwOtherLteCoversHook(), not only + * when deciding about a pending rule. */ + const Signature *done = *pending; + const uint16_t last = MIN(done->app_progress_hook, APP_LAYER_MAX_PROGRESS - 1); + for (uint16_t h = 0; h <= last; h++) { + DEBUG_VALIDATE_BUG_ON(det_ctx->fw_lte_cover[h] == 0); + if (det_ctx->fw_lte_cover[h] > 0) { + det_ctx->fw_lte_cover[h]--; + } + } + *credited_iid = UINT32_MAX; + } + *pending = DetectFwLtePendingRule(tv, de_ctx, det_ctx, p, f, cand_cnt, (*pending)->iid + 1); + } + return 0; +} + /** \internal * \brief handle the no-match case for a firewall rule * @@ -2492,7 +2637,8 @@ static int DetectRunTxFirewallRuleNoMatch(DetectEngineThreadCtx *det_ctx, const DetectTransaction *tx, struct DetectFirewallAppTxState *fw_state, Packet *p, const uint8_t flow_flags) { - if (fw_state->fw_last_for_progress && (s->flags & SIG_FLAG_FIREWALL)) { + if (fw_state->fw_last_for_progress && (s->flags & SIG_FLAG_FIREWALL) && + !fw_state->fw_defer_defaults) { SCLogDebug("%" PRIu64 ": %s default policy for progress %u", PcapPacketCntGet(p), flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", s->app_progress_hook); /* if this rule was the last for our progress state, and it didn't match, @@ -2521,9 +2667,9 @@ static int DetectRunTxFirewallRuleNoMatch(DetectEngineThreadCtx *det_ctx, const const uint8_t last_hook = fw_state->last_fw_rule ? tx->tx_progress : MIN(tx->tx_progress, s->app_progress_hook + 1); - enum DetectTxFirewallFlowControl r = - DetectFirewallApplyDefaultPolicies(det_ctx, det_ctx->de_ctx->fw_policies, tx, p, - s->alproto, flow_flags, s->app_progress_hook + 1, last_hook); + enum DetectTxFirewallFlowControl r = DetectFirewallApplyDefaultPolicies(det_ctx, + det_ctx->de_ctx->fw_policies, tx, p, s->alproto, flow_flags, + s->app_progress_hook + 1, last_hook, fw_state->fw_defer_defaults); if (r == DETECT_TX_FW_FC_BREAK) { /* the sweep dropped the packet: same as a current-hook * policy drop, the caller must stop the walk */ @@ -2618,10 +2764,13 @@ static void DetectRunTx(ThreadVars *tv, total_rules += (tx.de_state ? tx.de_state->cnt : 0); /* run prefilter engines and merge results into a candidates array */ + /* set if an LTE rule has its hook at the current tx progress while the + * tx is not final there: the fast pattern decides candidacy there. */ + bool lte_at_hook = false; if (sgh && sgh->tx_engines) { PACKET_PROFILING_DETECT_START(p, PROF_DETECT_PF_TX); - DetectRunPrefilterTx(det_ctx, sgh, p, ipproto, flow_flags, alproto, - alstate, &tx); + lte_at_hook = DetectRunPrefilterTx( + det_ctx, sgh, p, ipproto, flow_flags, alproto, alstate, &tx); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_PF_TX); SCLogDebug("%p/%"PRIu64" rules added from prefilter: %u candidates", tx.tx_ptr, tx.tx_id, det_ctx->pmq.rule_id_array_cnt); @@ -2729,8 +2878,31 @@ static void DetectRunTx(ThreadVars *tv, * tracked where the candidates are added: the list mixes fw and TD * rules across tables in iid order, so a scan cannot stop early. */ struct DetectFirewallAppTxState fw_state = { 0 }; - if (have_fw_rules && fw_lte_candidates) { + fw_state.fw_lte_credited_iid = UINT32_MAX; + if (have_fw_rules && lte_at_hook) { + /* The lowest of those rules is still in the running: it covers the hooks + * as if it were in the list, and it ends the walk before any rule that + * takes a higher id. It is not inspected here, so a matcher that fails + * definitively on a buffer that can still grow (which would retire it in + * a full walk) leaves it pending: the defaults wait for the hook. */ + fw_state.fw_lte_pending = + DetectFwLtePendingRule(tv, de_ctx, det_ctx, p, f, array_idx, 0); + } + /* The fast pattern decided candidacy at the rules hook, so the rules it did + * not add are missing from the list. They are not final at this state, so the + * default policies have to wait for them. Only for those that are actually in + * the running: a window whose misses are all out of the flows scope defers no + * policy, or the one of this hook would be dropped on the floor. */ + fw_state.fw_defer_defaults = have_fw_rules && fw_state.fw_lte_pending != NULL; + if (have_fw_rules && (fw_lte_candidates || fw_state.fw_lte_pending != NULL)) { DetectFwBuildLteCoverage(det_ctx, p, f, &fw_state, array_idx); + if (fw_state.fw_lte_pending != NULL) { + const Signature *ps = fw_state.fw_lte_pending; + const uint16_t last = MIN(ps->app_progress_hook, APP_LAYER_MAX_PROGRESS - 1); + for (uint16_t h = 0; h <= last; h++) + det_ctx->fw_lte_cover[h]++; + fw_state.fw_lte_credited_iid = ps->iid; + } } SCLogDebug("%s: tx_progress %u tx %p have_fw_rules %s array_idx %u detect_progress_orig %u " @@ -2743,7 +2915,7 @@ static void DetectRunTx(ThreadVars *tv, /* if there are no firewall rules to consider, handle invoking the default * policies. */ const int r = DetectTxFirewallNoRulesApplyPolicies( - det_ctx, p, f, &tx, alproto, flow_flags, array_idx); + det_ctx, p, f, &tx, alproto, flow_flags, array_idx, fw_state.fw_defer_defaults); if (r == 1) { SCLogDebug("done"); return; @@ -2753,7 +2925,8 @@ static void DetectRunTx(ThreadVars *tv, } /* run rules: inspect the match candidates */ - for (uint32_t i = 0; i < array_idx; i++) { + uint32_t i = 0; + for (; i < array_idx; i++) { RuleMatchCandidateTx *can = &det_ctx->tx_candidates[i]; const Signature *s = det_ctx->tx_candidates[i].s; uint32_t *inspect_flags = det_ctx->tx_candidates[i].flags; @@ -2762,6 +2935,22 @@ static void DetectRunTx(ThreadVars *tv, flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", tx.tx_progress, tx.detect_progress, tx.detect_progress_orig, s->app_progress_hook); + if (have_fw_rules && fw_state.fw_lte_pending != NULL && + s->iid > fw_state.fw_lte_pending->iid) { + /* A walk that has all rules in it reaches the pending rules before + * this candidate: what they do there is what would have happened. */ + SCLogDebug( + "sid %u is beyond pending LTE rule %u", s->id, fw_state.fw_lte_pending->id); + if (DetectFwLtePendingThrough(tv, de_ctx, det_ctx, &fw_state.fw_lte_pending, p, f, + &tx, &fw_state, array_idx, s->iid, + &fw_state.fw_lte_credited_iid) == 1) { + break; + } + /* the chain can empty here, and the policy paths read the bool rather than + * the pointer: it has to follow what it was derived from */ + fw_state.fw_defer_defaults = fw_state.fw_lte_pending != NULL; + } + if (have_fw_rules) { const enum DetectTxFirewallFlowControl fw_r = DetectRunTxPreCheckFirewallPolicy( det_ctx, p, &tx, flow_flags & (STREAM_TOSERVER | STREAM_TOCLIENT), s, i, @@ -2940,6 +3129,15 @@ static void DetectRunTx(ThreadVars *tv, } } + if (have_fw_rules && i == array_idx && fw_state.fw_lte_pending != NULL) { + /* no candidate took an id above a pending rule, so the loop never stepped + * through them. A full walk would still have run them: do their treatment + * here, at the end of the walk. */ + (void)DetectFwLtePendingThrough(tv, de_ctx, det_ctx, &fw_state.fw_lte_pending, p, f, + &tx, &fw_state, array_idx, UINT32_MAX, &fw_state.fw_lte_credited_iid); + fw_state.fw_defer_defaults = fw_state.fw_lte_pending != NULL; + } + det_ctx->tx_id = 0; det_ctx->tx_id_set = false; det_ctx->p = NULL; diff --git a/src/detect.h b/src/detect.h index e298c74825..5cde6fdfff 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1422,6 +1422,18 @@ typedef struct DetectEngineThreadCtx_ { * Cleared by the firewall coverage build on first use in a walk. */ uint32_t fw_lte_cover[APP_LAYER_MAX_PROGRESS]; + /** LTE rules still pending at this state: the tx reached the bound of their + * window engines, so the fast pattern decided their candidacy and the rules it + * did not add are absent from the candidates while they can still match. One + * entry per engine bound here: a rule with hook P has P of them, one per state + * below the hook, and another per sub-state, direction and buffer list it + * registered in. The walk merges them with the candidates to find the lowest + * rule that is still in the running. Grows with the engines, freed on thread + * deinit. */ + const void **fw_lte_windows; /**< PrefilterNonPFDataTx, opaque here */ + uint32_t fw_lte_window_cnt; + uint32_t fw_lte_window_size; + MpmThreadCtx mtc; /**< thread ctx for the mpm */ /* work queue for post-rule matching affecting prefilter */ PostRuleMatchWorkQueue post_rule_work_queue; @@ -1620,6 +1632,7 @@ typedef struct PrefilterEngineList_ { /** Minimal Tx progress we need before running the engine. Only used * with Tx Engine. Set to -1 for all states. */ int8_t tx_min_progress; + /** Upper bound, exclusive: skip the engine once the tx reached it. * 0 means no bound. */ uint8_t tx_max_progress; @@ -1672,10 +1685,10 @@ typedef struct PrefilterEngine_ { /** Minimal Tx progress we need before running the engine. Only used * with Tx Engine. Set to -1 for all states. */ int8_t tx_min_progress; + uint8_t sub_state; /** Upper bound, exclusive: skip the engine once the tx reached it. - * 0 means no bound. */ + * 0 means no bound. Only set for the non-prefilter engines. */ uint8_t tx_max_progress; - uint8_t sub_state; } app; uint8_t frame_type; } ctx; diff --git a/src/tests/detect.c b/src/tests/detect.c index f0d87f4770..df7cf73b06 100644 --- a/src/tests/detect.c +++ b/src/tests/detect.c @@ -4805,6 +4805,164 @@ static int DetectFwLteCoverageTest03(void) PASS; } +/** \test a pending LTE rule the walk would discard must not hold the hook. + * Pins the filters in DetectFwLteRuleStillLive(): a rule whose header and protocol + * checks pass but whose packet engines fail is no barrier, as the full walk would + * drop it before reaching the rules above it. */ +static int DetectFwLtePendingLiveTest01(void) +{ + EngineModeSetFirewall(ENGINE_HOST_IS_ROUTER); + + ThreadVars th_v; + memset(&th_v, 0, sizeof(th_v)); + StatsThreadInit(&th_v.stats); + + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + FAIL_IF_NULL(de_ctx); + de_ctx->flags |= DE_QUIET; + FAIL_IF(DetectFirewallInitDefaultPolicies(de_ctx) != 0); + + /* live in every respect on the packet below */ + Signature *s_ok = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any any " + "(content:\"abc\"; sid:10;)"); + FAIL_IF_NULL(s_ok); + /* same rule, but a packet predicate that cannot hold on a small packet */ + Signature *s_pkt = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any any " + "(content:\"abc\"; dsize:>9000; sid:11;)"); + FAIL_IF_NULL(s_pkt); + /* port does not match, so the rule header already rejects it */ + Signature *s_hdr = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any 9999 " + "(content:\"abc\"; sid:12;)"); + FAIL_IF_NULL(s_hdr); + SigGroupBuild(de_ctx); + + DetectEngineThreadCtx *det_ctx = NULL; + DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); + FAIL_IF_NULL(det_ctx); + + Flow f; + memset(&f, 0, sizeof(f)); + FLOW_INITIALIZE(&f); + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_HTTP1; + + Packet p; + memset(&p, 0, sizeof(p)); + p.proto = IPPROTO_TCP; + p.dp = 80; + p.sp = 1234; + p.flow = &f; + p.flowflags |= FLOW_PKT_TOSERVER; + + FAIL_IF(!DetectFwLteRuleStillLive(&th_v, de_ctx, det_ctx, &p, &f, s_ok)); + /* the case the scan used to miss: header and protocol pass, packet engines fail */ + FAIL_IF(DetectFwLteRuleStillLive(&th_v, de_ctx, det_ctx, &p, &f, s_pkt)); + FAIL_IF(DetectFwLteRuleStillLive(&th_v, de_ctx, det_ctx, &p, &f, s_hdr)); + /* a rule for another protocol is not a barrier either */ + const AppProto keep = s_ok->alproto; + s_ok->alproto = ALPROTO_TLS; + FAIL_IF(DetectFwLteRuleStillLive(&th_v, de_ctx, det_ctx, &p, &f, s_ok)); + s_ok->alproto = keep; + FAIL_IF(!DetectFwLteRuleStillLive(&th_v, de_ctx, det_ctx, &p, &f, s_ok)); + + DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx); + DetectEngineCtxFree(de_ctx); + FLOW_DESTROY(&f); + StatsThreadCleanup(&th_v.stats); + EngineModeSetIDS(); + PASS; +} + +/** \test a pending LTE rule that the walk would drop on a packet predicate must + * not defer the default policy of its hook (ticket #9149). + * + * The only accept rule of the state is blocked by `dsize`, so no rule is in the + * running at the hook: the http1 app default (drop:flow) has to decide on the + * first packet. Counting the rule as pending instead defers the policy, which is a + * fail-open window opened by a rule that cannot be inspected at all. */ +static int DetectFwLtePendingLivePolicyTest01(void) +{ + EngineModeSetFirewall(ENGINE_HOST_IS_ROUTER); + + ThreadVars th_v; + memset(&th_v, 0, sizeof(th_v)); + StatsThreadInit(&th_v.stats); + + Flow f; + TcpSession ssn; + memset(&f, 0, sizeof(f)); + memset(&ssn, 0, sizeof(ssn)); + FLOW_INITIALIZE(&f); + f.protoctx = (void *)&ssn; + f.flags |= FLOW_IPV4; + f.proto = IPPROTO_TCP; + f.alproto = ALPROTO_HTTP1; + + AppLayerParserThreadCtx *alp_tctx = AppLayerParserThreadCtxAlloc(); + FAIL_IF_NULL(alp_tctx); + StreamTcpInitConfig(true); + + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + FAIL_IF_NULL(de_ctx); + de_ctx->flags |= DE_QUIET; + FAIL_IF(DetectFirewallInitDefaultPolicies(de_ctx) != 0); + + /* accept the packet hooks, so only the app state is under test */ + Signature *scaf1 = DetectFirewallRuleAppendNew( + de_ctx, "accept:hook tcp:all any any -> any any (flow:not_established; sid:1021;)"); + FAIL_IF_NULL(scaf1); + Signature *scaf2 = DetectFirewallRuleAppendNew( + de_ctx, "accept:hook tcp:all any any -> any any (flow:established; sid:1022;)"); + FAIL_IF_NULL(scaf2); + /* The only LTE rule of the state. The pattern is absent, so the fast pattern + * leaves it in the pending window, and a packet predicate blocks it on this + * packet: only the window search can tell it is not live. */ + Signature *s_blk = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any any " + "(content:\"absent.example.com\"; dsize:>9000; sid:1101;)"); + FAIL_IF_NULL(s_blk); + + SigGroupBuild(de_ctx); + DetectEngineThreadCtx *det_ctx = NULL; + DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); + FAIL_IF_NULL(det_ctx); + + /* the header block stays open, so the hook is not crossed on this packet */ + const uint8_t seg[] = "GET / HTTP/1.1\r\nHost: open.example.com\r\n"; + Packet *p = UTHBuildPacket((uint8_t *)seg, (uint16_t)strlen((const char *)seg), IPPROTO_TCP); + FAIL_IF_NULL(p); + p->flow = &f; + p->flowflags |= FLOW_PKT_TOSERVER; + p->flowflags |= FLOW_PKT_ESTABLISHED; + p->flags |= PKT_HAS_FLOW | PKT_STREAM_EST; + p->app_update_direction = STREAM_TOSERVER; + FAIL_IF(AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_HTTP1, STREAM_TOSERVER, seg, + (uint16_t)strlen((const char *)seg)) != 0); + SigMatchSignatures(&th_v, de_ctx, det_ctx, p); + + /* the rule cannot be inspected on this packet: the default policy decides now */ + FAIL_IF(!(f.flags & FLOW_ACTION_DROP)); + FAIL_IF(f.flags & FLOW_ACTION_ACCEPT); + /* and it is not the rule that dropped: no alert of its own is queued */ + for (uint16_t i = 0; i < det_ctx->alert_queue_size; i++) { + FAIL_IF(det_ctx->alert_queue[i].s == s_blk); + } + + UTHFreePackets(&p, 1); + DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx); + DetectEngineCtxFree(de_ctx); + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(true); + FLOW_DESTROY(&f); + StatsThreadCleanup(&th_v.stats); + EngineModeSetIDS(); + + PASS; +} + void SigRegisterTests(void) { SigParseRegisterTests(); @@ -4813,6 +4971,8 @@ void SigRegisterTests(void) UtRegisterTest("DetectFwLteCoverageTest01", DetectFwLteCoverageTest01); UtRegisterTest("DetectFwLteCoverageTest02", DetectFwLteCoverageTest02); UtRegisterTest("DetectFwLteCoverageTest03", DetectFwLteCoverageTest03); + UtRegisterTest("DetectFwLtePendingLiveTest01", DetectFwLtePendingLiveTest01); + UtRegisterTest("DetectFwLtePendingLivePolicyTest01", DetectFwLtePendingLivePolicyTest01); UtRegisterTest("SigTest01", SigTest01); UtRegisterTest("SigTest02 -- Offset/Depth match", SigTest02); From dcccd70ebc6030ffc127202d14171da040556d75 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sat, 3 Oct 2026 22:11:34 +0000 Subject: [PATCH 06/10] detect: unit tests for the LTE pending window The candidate list of a "less than or equal" hook rule is decided by the fast pattern at the rules hook, so the walk has to find the lowest rule the pattern did not add from the window the rule is registered in: both are in id order, so a single pass over them answers it. Test that pass, including the rules of other groups that are in the list as well. Ticket: #9149. --- src/detect-engine-prefilter.c | 183 ++++++++++++++++++++++++++++++++++ src/runmode-unittests.c | 2 + 2 files changed, 185 insertions(+) diff --git a/src/detect-engine-prefilter.c b/src/detect-engine-prefilter.c index a10c5a0027..614522be6e 100644 --- a/src/detect-engine-prefilter.c +++ b/src/detect-engine-prefilter.c @@ -2146,4 +2146,187 @@ void PostRuleMatchWorkQueueAppend( } #ifdef UNITTESTS +static struct PrefilterNonPFDataTx *LteWindowTestData(const uint32_t *iids, uint32_t cnt) +{ + struct PrefilterNonPFDataTx *data = SCCalloc(1, sizeof(*data) + cnt * sizeof(data->array[0])); + if (data == NULL) + return NULL; + data->size = cnt; + for (uint32_t i = 0; i < cnt; i++) + data->array[i] = iids[i]; + return data; +} + +/** \test the pending rule of a LTE window the fast pattern did not add */ +static int PrefilterLteWindowTest01(void) +{ + const uint32_t iids[] = { 2, 5, 9, 13 }; + struct PrefilterNonPFDataTx *data = LteWindowTestData(iids, 4); + FAIL_IF(data == NULL); + + /* the pattern added nothing: the lowest rule of the window is pending */ + FAIL_IF(LteWindowFirstMissing(data, NULL, 0, 0) != 2); + + /* a rule of the window matched, but it is not the lowest one */ + RuleMatchCandidateTx cands[1] = { { .id = 5 } }; + FAIL_IF(LteWindowFirstMissing(data, cands, 1, 0) != 2); + + /* the lowest rule matched, so the miss is the next one */ + cands[0].id = 2; + FAIL_IF(LteWindowFirstMissing(data, cands, 1, 0) != 5); + + RuleMatchCandidateTx more[3] = { { .id = 2 }, { .id = 5 }, { .id = 9 } }; + FAIL_IF(LteWindowFirstMissing(data, more, 3, 0) != 13); + + /* the window is fully matched: nothing is pending */ + RuleMatchCandidateTx all[4] = { { .id = 2 }, { .id = 5 }, { .id = 9 }, { .id = 13 } }; + FAIL_IF(LteWindowFirstMissing(data, all, 4, 0) != UINT32_MAX); + + /* the candidates also hold rules of other groups, before, between and after */ + RuleMatchCandidateTx mix[5] = { { .id = 1 }, { .id = 2 }, { .id = 6 }, { .id = 9 }, + { .id = 40 } }; + FAIL_IF(LteWindowFirstMissing(data, mix, 5, 0) != 5); + + SCFree(data); + PASS; +} + +/** \test empty and single entry windows */ +static int PrefilterLteWindowTest02(void) +{ + struct PrefilterNonPFDataTx *empty = LteWindowTestData(NULL, 0); + FAIL_IF(empty == NULL); + FAIL_IF(LteWindowFirstMissing(empty, NULL, 0, 0) != UINT32_MAX); + SCFree(empty); + + const uint32_t iids[] = { 7 }; + struct PrefilterNonPFDataTx *one = LteWindowTestData(iids, 1); + FAIL_IF(one == NULL); + FAIL_IF(LteWindowFirstMissing(one, NULL, 0, 0) != 7); + FAIL_IF(LteWindowFirstMissing(one, NULL, 0, 8) != UINT32_MAX); + RuleMatchCandidateTx cand = { .id = 7 }; + FAIL_IF(LteWindowFirstMissing(one, &cand, 1, 0) != UINT32_MAX); + SCFree(one); + PASS; +} + +/** \test DetectPrefilterLtePendingIid over several windows */ +static int PrefilterLteWindowTest03(void) +{ + DetectEngineThreadCtx det_ctx = { 0 }; + Signature sigs[6] = { { 0 } }; + for (uint32_t i = 0; i < 6; i++) + sigs[i].iid = i; + + const uint32_t a[] = { 1, 3, 5 }, b[] = { 0, 2, 4 }; + struct PrefilterNonPFDataTx *wa = LteWindowTestData(a, 3); + struct PrefilterNonPFDataTx *wb = LteWindowTestData(b, 3); + FAIL_IF(wa == NULL || wb == NULL); + FAIL_IF(wa == wb); + + RuleMatchCandidateTx cands[2] = { { .id = 0 }, { .id = 3 } }; + const void *windows[2] = { wa, wb }; + det_ctx.tx_candidates = cands; + det_ctx.fw_lte_windows = windows; + det_ctx.fw_lte_window_size = 2; + det_ctx.fw_lte_window_cnt = 2; + + /* window b has rule 0, so its first miss is 2; window a misses at 1 */ + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 0) != sigs[1].iid); + /* the caller takes the next one when a rule turns out to be out of scope */ + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 2) != 2); + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 3) != 4); + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 6) != UINT32_MAX); + + /* nothing pending: every rule of every window made the list */ + det_ctx.fw_lte_window_cnt = 0; + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 0) != UINT32_MAX); + + SCFree(wa); + SCFree(wb); + PASS; +} + +/** \test more windows than the initial array holds: they all stay tracked */ +static int PrefilterLteWindowTest04(void) +{ + DetectEngineThreadCtx det_ctx = { 0 }; + Signature sigs[16] = { { 0 } }; + for (uint32_t i = 0; i < 16; i++) + sigs[i].iid = i; + + const uint32_t pending[] = { 4, 9 }; + const uint32_t matched[] = { 12 }; + struct PrefilterNonPFDataTx *w0 = LteWindowTestData(pending, 2); + struct PrefilterNonPFDataTx *w[16] = { 0 }; + FAIL_IF(w0 == NULL); + PrefilterLteWindowAdd(&det_ctx, w0); + for (uint32_t i = 0; i < 16; i++) { + /* distinct sets: the array has to grow past its initial 8 entries */ + w[i] = LteWindowTestData(matched, 1); + FAIL_IF(w[i] == NULL); + PrefilterLteWindowAdd(&det_ctx, w[i]); + } + FAIL_IF(det_ctx.fw_lte_window_cnt != 17); + FAIL_IF(det_ctx.fw_lte_window_size != 32); + /* a window already tracked is not tracked twice: the engines of a group share it */ + PrefilterLteWindowAdd(&det_ctx, w[3]); + FAIL_IF(det_ctx.fw_lte_window_cnt != 17); + + /* the windows that are fully matched give no pending rule, window 0 misses at 9 */ + RuleMatchCandidateTx cands[2] = { { .id = 4 }, { .id = 12 } }; + det_ctx.tx_candidates = cands; + FAIL_IF(DetectPrefilterLtePendingIid(&det_ctx, 2, 0) != sigs[9].iid); + + DetectPrefilterLteWindowFree(&det_ctx); + FAIL_IF(det_ctx.fw_lte_windows != NULL); + FAIL_IF(det_ctx.fw_lte_window_cnt != 0); + for (uint32_t i = 0; i < 16; i++) { + SCFree(w[i]); + } + SCFree(w0); + PASS; +} + +/** \test a large window entered at any cursor answers as a linear scan would + * (the search enters both arrays where the cursor lies, not at index 0) + */ +static int PrefilterLteWindowTest05(void) +{ + enum { WIN = 256, CAND = 128 }; + uint32_t iids[WIN]; + for (uint32_t i = 0; i < WIN; i++) + iids[i] = i * 2 + 1; + struct PrefilterNonPFDataTx *data = LteWindowTestData(iids, WIN); + FAIL_IF_NULL(data); + RuleMatchCandidateTx cands[CAND]; + for (uint32_t i = 0; i < CAND; i++) + cands[i].id = i * 4 + 1; + for (uint32_t cursor = 0; cursor <= WIN * 2; cursor += 7) { + uint32_t want = UINT32_MAX; + for (uint32_t i = 0; i < WIN; i++) { + if (iids[i] < cursor) + continue; + bool in_cands = false; + for (uint32_t j = 0; j < CAND; j++) + in_cands |= cands[j].id == iids[i]; + if (!in_cands) { + want = iids[i]; + break; + } + } + FAIL_IF(LteWindowFirstMissing(data, cands, CAND, cursor) != want); + } + SCFree(data); + PASS; +} + +void DetectPrefilterRegisterTests(void) +{ + UtRegisterTest("PrefilterLteWindowTest01", PrefilterLteWindowTest01); + UtRegisterTest("PrefilterLteWindowTest02", PrefilterLteWindowTest02); + UtRegisterTest("PrefilterLteWindowTest03", PrefilterLteWindowTest03); + UtRegisterTest("PrefilterLteWindowTest04", PrefilterLteWindowTest04); + UtRegisterTest("PrefilterLteWindowTest05", PrefilterLteWindowTest05); +} #endif /* UNITTESTS */ diff --git a/src/runmode-unittests.c b/src/runmode-unittests.c index fde9c66483..fa58dc7f21 100644 --- a/src/runmode-unittests.c +++ b/src/runmode-unittests.c @@ -28,6 +28,7 @@ #include "detect-parse.h" #include "detect-engine.h" #include "detect-engine-alert.h" +#include "detect-engine-prefilter.h" #include "detect-engine-address.h" #include "detect-engine-proto.h" #include "detect-engine-port.h" @@ -206,6 +207,7 @@ static void RegisterUnittests(void) DetectProtoTests(); DetectPortTests(); DetectEngineAlertRegisterTests(); + DetectPrefilterRegisterTests(); SCAtomicRegisterTests(); AppLayerUnittestsRegister(); StreamingBufferRegisterTests(); From 878fe2444247c7d90ddf43b873d4830f4dfce011 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Mon, 5 Oct 2026 01:44:57 +0000 Subject: [PATCH 07/10] detect: test the pending LTE accept priority The semantics of a pending `flags |= DE_QUIET; + FAIL_IF(DetectFirewallInitDefaultPolicies(de_ctx) != 0); + + /* TD rule at the hook: must still run while the accept rules are pending */ + Signature *s_td = DetectEngineAppendSig(de_ctx, + "alert http1:request_headers any any -> any any " + "(http.host.raw; content:\"early.example.com\"; sid:1103;)"); + FAIL_IF_NULL(s_td); + /* accept the packet hook for all tcp: suppresses the default drop and lets + * the app-layer rules decide */ + Signature *s_scaf1 = DetectFirewallRuleAppendNew( + de_ctx, "accept:hook tcp:all any any -> any any (flow:not_established; sid:1021;)"); + FAIL_IF_NULL(s_scaf1); + Signature *s_scaf2 = DetectFirewallRuleAppendNew( + de_ctx, "accept:hook tcp:all any any -> any any (flow:established; sid:1022;)"); + FAIL_IF_NULL(s_scaf2); + /* content present from the first segment */ + Signature *s_early = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any any " + "(http.host.raw; content:\"early.example.com\"; sid:1102;)"); + FAIL_IF_NULL(s_early); + /* the header only arrives in the second segment */ + Signature *s_late = DetectFirewallRuleAppendNew(de_ctx, + "accept:flow,alert http1: any any " + "(http.header; content:\"X-Late: a\"; sid:1101;)"); + FAIL_IF_NULL(s_late); + + SigGroupBuild(de_ctx); + DetectEngineThreadCtx *det_ctx = NULL; + DetectEngineThreadCtxInit(&th_v, (void *)de_ctx, (void *)&det_ctx); + FAIL_IF_NULL(det_ctx); + + /* segment 1: request line + host, the header block is open */ + const uint8_t *seg1 = (const uint8_t *)"GET / HTTP/1.1\r\nHost: early.example.com\r\n"; + Packet *p1 = UTHBuildPacket((uint8_t *)seg1, (uint16_t)strlen((const char *)seg1), IPPROTO_TCP); + FAIL_IF_NULL(p1); + p1->flow = &f; + p1->flowflags |= FLOW_PKT_TOSERVER; + p1->flowflags |= FLOW_PKT_ESTABLISHED; + p1->flags |= PKT_HAS_FLOW | PKT_STREAM_EST; + p1->app_update_direction = STREAM_TOSERVER; + int r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_HTTP1, STREAM_TOSERVER, seg1, + (uint16_t)strlen((const char *)seg1)); + FAIL_IF(r != 0); + SigMatchSignatures(&th_v, de_ctx, det_ctx, p1); + + /* 1101 is pending: 1102 must not have accepted the flow yet */ + FAIL_IF(f.flags & FLOW_ACTION_ACCEPT); + + /* segment 2: the late header completes the block */ + const uint8_t *seg2 = (const uint8_t *)"X-Late: a\r\n\r\n"; + Packet *p2 = UTHBuildPacket((uint8_t *)seg2, (uint16_t)strlen((const char *)seg2), IPPROTO_TCP); + FAIL_IF_NULL(p2); + p2->flow = &f; + p2->flowflags |= FLOW_PKT_TOSERVER; + p2->flowflags |= FLOW_PKT_ESTABLISHED; + p2->flags |= PKT_HAS_FLOW | PKT_STREAM_EST; + p2->app_update_direction = STREAM_TOSERVER; + r = AppLayerParserParse(NULL, alp_tctx, &f, ALPROTO_HTTP1, STREAM_TOSERVER, seg2, + (uint16_t)strlen((const char *)seg2)); + FAIL_IF(r != 0); + SigMatchSignatures(&th_v, de_ctx, det_ctx, p2); + + /* 1101 matched on completion: the flow is accepted by the pending rule */ + FAIL_IF(!(f.flags & FLOW_ACTION_ACCEPT)); + + /* the pending rule won; the early rule never matched. The TD rule ran + * once, on completion. */ + uint32_t late = 0, early = 0, td = 0; + for (uint16_t i = 0; i < det_ctx->alert_queue_size; i++) { + if (det_ctx->alert_queue[i].s == s_late) + late++; + else if (det_ctx->alert_queue[i].s == s_early) + early++; + else if (det_ctx->alert_queue[i].s == s_td) + td++; + } + FAIL_IF(late != 1); + FAIL_IF(early != 0); + FAIL_IF(td != 1); + + UTHFreePackets(&p2, 1); + UTHFreePackets(&p1, 1); + DetectEngineThreadCtxDeinit(&th_v, (void *)det_ctx); + DetectEngineCtxFree(de_ctx); + AppLayerParserThreadCtxFree(alp_tctx); + StreamTcpFreeConfig(true); + FLOW_DESTROY(&f); + StatsThreadCleanup(&th_v.stats); + EngineModeSetIDS(); + + PASS; +} + /** \test a pending LTE rule the walk would discard must not hold the hook. * Pins the filters in DetectFwLteRuleStillLive(): a rule whose header and protocol * checks pass but whose packet engines fail is no barrier, as the full walk would @@ -4971,6 +5095,7 @@ void SigRegisterTests(void) UtRegisterTest("DetectFwLteCoverageTest01", DetectFwLteCoverageTest01); UtRegisterTest("DetectFwLteCoverageTest02", DetectFwLteCoverageTest02); UtRegisterTest("DetectFwLteCoverageTest03", DetectFwLteCoverageTest03); + UtRegisterTest("DetectFwLteMpmWalkTest01", DetectFwLteMpmWalkTest01); UtRegisterTest("DetectFwLtePendingLiveTest01", DetectFwLtePendingLiveTest01); UtRegisterTest("DetectFwLtePendingLivePolicyTest01", DetectFwLtePendingLivePolicyTest01); From c3379c04b9dd3c0b120b36e76efb0fc306c53402 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sat, 10 Oct 2026 01:26:19 +0000 Subject: [PATCH 08/10] firewall: no raw stream match at an LTE hook An LTE rule of an app-ioctl protocol cannot carry a raw stream pattern: the packet scan of a lower state is not a decision at this hook, so such a rule would be matched where it must not be. Refuse it at parse time. The window unit tests move to `flow:` predicates to keep that distinction. A `flowbits` predicate no longer gates anything here: a rule which reads a variable gets no fast pattern now, so it is a plain candidate in every update and the ordinary walk settles it. `flow:` is refused by neither check and stays out of the variable list, so the packet test still proves a packet accept at a lower state decides nothing on its own, and the policy test still proves a default policy does not decide around a live rule. `DetectFwLtePendingLivePolicyTest01()` is the one whose predicate has to do real work: with the liveness filter in ` DetectFwLteRuleStillLive()` forced to `return true` it is the only LTE window test that fails. --- doc/userguide/firewall/firewall-design.rst | 61 ++++++++++++++-------- src/detect-engine-prefilter.c | 9 ++-- src/detect-parse.c | 7 +++ src/tests/detect.c | 22 ++++---- 4 files changed, 65 insertions(+), 34 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 678daee9dc..c00fbe3b2d 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -289,17 +289,14 @@ matching, but a buffer of its hook can still grow - a rule hooked at a higher state is still evaluated once the transaction has moved past the pending rule's hook; while the transaction is at or before that hook the walk stops, so no higher state's policy can decide the flow early. A rule of a higher id at the -pending rule's own hook is evaluated too: stepping over the pending one must not -stall the hook, because a rule which can never resolve would otherwise hold it -open for the whole transaction. That is a choice, and its price is that a -drop:flow rule at that hook can decide before a pending accept resolves. +pending rule's own hook is inspected in the same walk: a rule which cannot be +decided by this update must not stall the hook for the rules which can. The price +is that such a rule's action - a ``drop:flow``, say - decides the flow before the +pending accept resolves. ``ruletype-firewall-616`` pins it. A higher-hook rule inspects the buffers of every state below its own hook, so its match covers the pending rule and its action decides the flow: when it matches, the pending rule's later -no-match does not apply the default policy of its own state. Stepping over a rule -the transaction has moved past keeps that rule's accept for the packet, as a -listed partial match of it would: without it the packet default policy drops -packets of a flow which then ends up accepted. +no-match does not apply the default policy of its own state. A sub-state's buffers can also grow after the transaction moved past the rule's hook: a http2 trailer HEADERS frame updates the header lists above the @@ -307,9 +304,10 @@ hook: a http2 trailer HEADERS frame updates the header lists above the not with the transaction: a http2 stream only completes once both sides closed, and holding a rule pending that long defers the default policy past the request. The fast pattern at the hook decides there: above it a group is retired once its rules are, so a rule whose buffer is final at that -point is not run again. A rule whose pattern can still arrive later - a raw -stream ``content``, or a buffer that is not complete yet - keeps the window -filled, and with it the default policies of the states it covers. +point is not run again. A group whose buffer can still be rewritten is not: all +of its rules go into the list at that update, and with them the default policies +of the states they cover. A rule that would stay open because a *stream* match +can still arrive cannot use the notation at all, see the limitations below. The bound is the transaction's end state. For a parser with per-direction sub- states that can be later than the direction's own close - a http2 stream @@ -506,20 +504,41 @@ Two moments, for the rules of a group hooked at ``H``: wins the state and the policy decides when nothing does. A group that also holds a rule whose pattern can still grow above ``C`` is not retired there, so the retirement cannot apply a default policy through it. Tracking such a rule keeps - its hook covered while the transaction has moved past it, without ending the walk - there: a match at a higher rule still gets its inspection, and the rule keeps its - packet accept. When no candidate sits above it, the walk takes the pending rules - at its end, so they are accounted for either way. + its hook, so a rule which can never resolve does not hold it open for the whole + transaction. When no candidate takes an id above them, the walk takes the pending + rules at its end, so they are accounted for either way. ``C`` is ``H + 1`` for the app buffers of a protocol which declares no sub-states: an app hooked rule can only hold patterns of buffers whose engine sits at its hook, so such -a buffer cannot grow once the transaction has moved past it. Two cases do grow: a raw -stream pattern, which arrives with a later segment, and an app buffer of a protocol -declaring sub-states, because a frame can rewrite it above the hook -- an http2 trailer -HEADERS frame updates the header buffers. The first still leaves the fast pattern to say -which update brings it, so the group keeps its window filled there; the second has no -pattern which covers that update, so all the rules of the group are inspected. A rule +a buffer cannot grow once the transaction has moved past it. One case does grow: an +app buffer of a protocol declaring sub-states, because a frame can rewrite it above the +hook -- an http2 trailer HEADERS frame updates the header list buffers. The buffers of a +request that ended with END_STREAM do not: the parser says where each direction stops +growing, and that is what the revisit keys on. It has no pattern +which covers that update, so all the rules of the group are inspected there. A rule with no fast pattern at all has no update which brings it, so a group which holds one is inspected whole there as well. A transaction that ends at or below ``H`` never opens a window: its end state decides, and a window opens only for a tx that keeps going past ``H``. + +LIMITATIONS +----------- + +* A rule using the auto-accept notation (``init_data->mpm_sm == NULL) { diff --git a/src/detect-parse.c b/src/detect-parse.c index 9888ea5fb2..a1aa265bdb 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -2939,6 +2939,13 @@ static bool DetectFirewallRuleValidate(const DetectEngineCtx *de_ctx, const Sign } } if (s->flags & SIG_FLAG_FW_HOOK_LTE) { + if (s->init_data->smlists[DETECT_SM_LIST_PMATCH] != NULL) { + SCLogError("rule %u: the auto-accept notation ('id); + return false; + } if (!(((s->action & ACTION_ACCEPT) != 0) && (s->action_scope == ACTION_SCOPE_FLOW || s->action_scope == ACTION_SCOPE_TX || s->action_scope == ACTION_SCOPE_HOOK))) { diff --git a/src/tests/detect.c b/src/tests/detect.c index f5263996cb..6a4989d8d8 100644 --- a/src/tests/detect.c +++ b/src/tests/detect.c @@ -4949,17 +4949,19 @@ static int DetectFwLtePendingLiveTest01(void) /* live in every respect on the packet below */ Signature *s_ok = DetectFirewallRuleAppendNew(de_ctx, "accept:flow,alert http1: any any " - "(content:\"abc\"; sid:10;)"); + "(http.host; content:\"abc\"; sid:10;)"); FAIL_IF_NULL(s_ok); - /* same rule, but a packet predicate that cannot hold on a small packet */ + /* same rule, but a packet predicate that cannot hold on this packet: it is not + * established. A variable reader would not do: it never gets a prefilter, so it + * cannot be in the window (ruletype-firewall-621/622). */ Signature *s_pkt = DetectFirewallRuleAppendNew(de_ctx, "accept:flow,alert http1: any any " - "(content:\"abc\"; dsize:>9000; sid:11;)"); + "(http.host; content:\"abc\"; flow:established; sid:11;)"); FAIL_IF_NULL(s_pkt); /* port does not match, so the rule header already rejects it */ Signature *s_hdr = DetectFirewallRuleAppendNew(de_ctx, "accept:flow,alert http1: any 9999 " - "(content:\"abc\"; sid:12;)"); + "(http.host; content:\"abc\"; sid:12;)"); FAIL_IF_NULL(s_hdr); SigGroupBuild(de_ctx); @@ -5003,10 +5005,11 @@ static int DetectFwLtePendingLiveTest01(void) /** \test a pending LTE rule that the walk would drop on a packet predicate must * not defer the default policy of its hook (ticket #9149). * - * The only accept rule of the state is blocked by `dsize`, so no rule is in the - * running at the hook: the http1 app default (drop:flow) has to decide on the - * first packet. Counting the rule as pending instead defers the policy, which is a - * fail-open window opened by a rule that cannot be inspected at all. */ + * The only accept rule of the state is blocked by `flow:not_established`, so no + * rule is in the running at the hook: the http1 app default (drop:flow) has to + * decide on the first packet. Counting the rule as pending instead defers the + * policy, which is a fail-open window opened by a rule that cannot be inspected + * at all. */ static int DetectFwLtePendingLivePolicyTest01(void) { EngineModeSetFirewall(ENGINE_HOST_IS_ROUTER); @@ -5046,7 +5049,8 @@ static int DetectFwLtePendingLivePolicyTest01(void) * packet: only the window search can tell it is not live. */ Signature *s_blk = DetectFirewallRuleAppendNew(de_ctx, "accept:flow,alert http1: any any " - "(content:\"absent.example.com\"; dsize:>9000; sid:1101;)"); + "(http.host; content:\"absent.example.com\"; flow:not_established; " + "sid:1101;)"); FAIL_IF_NULL(s_blk); SigGroupBuild(de_ctx); From 75f3feab6c950c5760097ec4cca948b388f4c5a5 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 9 Oct 2026 17:02:42 +0000 Subject: [PATCH 09/10] http2: read http.host from the request header section A trailer HEADERS frame lands in the same header store as the request, so a rule on http.host could see the authority of a trailer: a request to a denied host that named an allowed one while closing itself was accepted, flow included. RFC 9113 8.1 forbids pseudo-header fields in trailers and RFC 9110 6.5.1 keeps routing fields such as Host out of them, so the value is now taken from the frames before the first DATA frame, or before the HEADERS frame that follows a completed block. The h2c upgrade path sends one HEADERS frame per field with no END_HEADERS, so those frames are all header section and keep counting: cutting at the second HEADERS frame rather than at a completed block breaks that stream, and http2-keywords2 says so. Ticket: #9149. --- doc/userguide/rules/http-keywords.rst | 6 ++ rust/src/http2/detect.rs | 125 ++++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 5 deletions(-) diff --git a/doc/userguide/rules/http-keywords.rst b/doc/userguide/rules/http-keywords.rst index 1dd0a1c91c..37f97fc6de 100644 --- a/doc/userguide/rules/http-keywords.rst +++ b/doc/userguide/rules/http-keywords.rst @@ -254,6 +254,12 @@ keywords. .. note:: The ``http.host`` keyword normalizes the host header contents. If a host name has uppercase characters, those would be changed to lowercase. +.. note:: For HTTP/2 the value comes from the ``:authority`` pseudo-header, or from + the ``Host`` header field when the request has no ``:authority``. Both are taken from + the header section of the request only: a trailer cannot change the host. Pseudo-header + fields are not allowed in trailers (RFC 9113 8.1) and routing fields such as ``Host`` + stay out of them (RFC 9110 6.5.1). + Normalization Example:: GET /index.html HTTP/1.1 diff --git a/rust/src/http2/detect.rs b/rust/src/http2/detect.rs index 689d2144c5..39f961d269 100644 --- a/rust/src/http2/detect.rs +++ b/rust/src/http2/detect.rs @@ -425,14 +425,50 @@ enum Http2Header<'a> { fn http2_frames_get_header_value<'a>( tx: &'a HTTP2Transaction, direction: Direction, name: &str, ) -> Option> { - let mut found = 0; - let mut vec = Vec::new(); - let mut single = None; let frames = if direction == Direction::ToServer { &tx.frames_ts } else { &tx.frames_tc }; + http2_frames_slice_get_header_value(frames, name) +} + +/// The header section of a message: the frames before the first DATA frame or +/// before a HEADERS frame that follows a completed header block (END_HEADERS), +/// which can only be the trailer section. The headers of an h2c upgrade arrive +/// as one HEADERS frame per field without END_HEADERS and all stay in. +fn http2_header_section(frames: &[HTTP2Frame]) -> &[HTTP2Frame] { + let mut block_done = false; + for (i, frame) in frames.iter().enumerate() { + match frame.data { + HTTP2FrameTypeData::DATA => { + return &frames[..i]; + } + HTTP2FrameTypeData::HEADERS(_) => { + if block_done { + return &frames[..i]; + } + if frame.header.flags & parser::HTTP2_FLAG_HEADER_END_HEADERS != 0 { + block_done = true; + } + } + HTTP2FrameTypeData::CONTINUATION(_) + if frame.header.flags & parser::HTTP2_FLAG_HEADER_END_HEADERS != 0 => + { + block_done = true; + } + _ => {} + } + } + frames +} + +fn http2_frames_slice_get_header_value<'a>( + frames: &'a [HTTP2Frame], name: &str, +) -> Option> { + let mut found = 0; + let mut vec = Vec::new(); + let mut single = None; for frame in frames { if let Some(blocks) = http2_header_blocks(frame) { for block in blocks.iter() { @@ -638,9 +674,13 @@ fn http2_normalize_host(ve: Http2Header) -> Http2Header { fn http2_get_host(tx: &HTTP2Transaction) -> Option> { // RFC 9113 states // The recipient of an HTTP/2 request MUST NOT use the Host header field to determine the target URI if ":authority" is present. - let r = http2_frames_get_header_value(tx, Direction::ToServer, ":authority"); + // Neither can come from a trailer: RFC 9113 8.1 forbids pseudo-header fields + // there, and RFC 9110 6.5.1 keeps routing fields like Host out of trailers. + // Reading them from every frame would let a trailer rewrite the target. + let section = http2_header_section(&tx.frames_ts); + let r = http2_frames_slice_get_header_value(section, ":authority"); if r.is_none() { - http2_frames_get_header_value(tx, Direction::ToServer, "host") + http2_frames_slice_get_header_value(section, "host") } else { r } @@ -1178,6 +1218,81 @@ mod tests { } } } + + fn hdr(name: &str, value: &str, flags: u8) -> HTTP2Frame { + let block = parser::HTTP2FrameHeaderBlock { + name: name.as_bytes().to_vec().into(), + value: value.as_bytes().to_vec().into(), + error: parser::HTTP2HeaderDecodeStatus::HTTP2HeaderDecodeSuccess, + sizeupdate: 0, + }; + HTTP2Frame { + header: parser::HTTP2FrameHeader { + length: 0, + ftype: parser::HTTP2FrameType::Headers as u8, + flags, + reserved: 0, + stream_id: 1, + }, + data: HTTP2FrameTypeData::HEADERS(parser::HTTP2FrameHeaders { + padlength: None, + priority: None, + blocks: vec![block], + }), + } + } + + fn data() -> HTTP2Frame { + HTTP2Frame { + header: parser::HTTP2FrameHeader { + length: 0, + ftype: parser::HTTP2FrameType::Data as u8, + flags: 0, + reserved: 0, + stream_id: 1, + }, + data: HTTP2FrameTypeData::DATA, + } + } + + #[test] + fn test_http2_get_host_from_header_section_only() { + let eos = parser::HTTP2_FLAG_HEADER_EOS; + let end = parser::HTTP2_FLAG_HEADER_END_HEADERS; + + // a body ends the header section, so the closing HEADERS frame is a + // trailer and cannot rewrite the target + let mut tx = HTTP2Transaction::new(); + tx.frames_ts.push(hdr(":authority", "evil.example", end)); + tx.frames_ts.push(data()); + tx.frames_ts + .push(hdr(":authority", "allowed.example", end | eos)); + match http2_get_host(&tx) { + Some(Http2Header::Single(v)) => assert_eq!(v, "evil.example".as_bytes()), + other => panic!("trailer must not reach http.host, got {other:?}"), + } + + // same without a body: the second block follows a completed one + let mut tx = HTTP2Transaction::new(); + tx.frames_ts.push(hdr("host", "evil.example", end)); + tx.frames_ts.push(hdr("host", "allowed.example", end | eos)); + match http2_get_host(&tx) { + Some(Http2Header::Single(v)) => assert_eq!(v, "evil.example".as_bytes()), + other => panic!("trailer must not reach http.host, got {other:?}"), + } + + // an h2c upgrade sends one HEADERS frame per field with no END_HEADERS: + // all of them are the header section + let mut tx = HTTP2Transaction::new(); + tx.frames_ts.push(hdr(":authority", "evil.example", 0)); + tx.frames_ts.push(hdr(":authority", "allowed.example", end)); + match http2_get_host(&tx) { + Some(Http2Header::Multiple(v)) => { + assert_eq!(v, "evil.example, allowed.example".as_bytes()) + } + other => panic!("upgrade frames must all count, got {other:?}",), + } + } } #[no_mangle] From acdb78a2293906ba5d64b586b9a2b3ac490622b2 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Sat, 10 Oct 2026 01:26:19 +0000 Subject: [PATCH 10/10] firewall: document LTE candidacy and its per-protocol limits The design note gains the reason a rule which reads a variable is given no fast pattern, and the http2 paragraphs describe a candidate group reaching its retirement at the trailer update. The limitations entry about a denied body being delivered until the request closes is dropped: measured against main on the trailer and body scenarios, the deciding packet is the one main drops at. The candidacy rule itself is in "firewall: fast pattern decides LTE candidacy". Ticket: #9149. --- doc/userguide/firewall/firewall-design.rst | 49 +++++++++++----------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index c00fbe3b2d..0224da145f 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -292,7 +292,9 @@ higher state's policy can decide the flow early. A rule of a higher id at the pending rule's own hook is inspected in the same walk: a rule which cannot be decided by this update must not stall the hook for the rules which can. The price is that such a rule's action - a ``drop:flow``, say - decides the flow before the -pending accept resolves. ``ruletype-firewall-616`` pins it. +pending accept resolves. ``ruletype-firewall-616`` pins it; ``ruletype-firewall-623`` pins +the other half, that nothing at all decides an update in which the transaction has not yet +reached the pending rule's hook. A higher-hook rule inspects the buffers of every state below its own hook, so its match covers the pending rule and its action decides the flow: when it matches, the pending rule's later @@ -300,9 +302,9 @@ no-match does not apply the default policy of its own state. A sub-state's buffers can also grow after the transaction moved past the rule's hook: a http2 trailer HEADERS frame updates the header lists above the -``request_headers`` hook. That growth stops with the direction's own END_STREAM, -not with the transaction: a http2 stream only completes once both sides closed, -and holding a rule pending that long defers the default policy past the request. The fast pattern at the hook decides there: above it a +``request_headers`` hook. Such a rule is not kept open for that update; it is +brought in as a candidate, so the growth is covered without deferring the states +below the hook. The fast pattern at the hook decides there: above it a group is retired once its rules are, so a rule whose buffer is final at that point is not run again. A group whose buffer can still be rewritten is not: all of its rules go into the list at that update, and with them the default policies @@ -312,9 +314,11 @@ can still arrive cannot use the notation at all, see the limitations below. The bound is the transaction's end state. For a parser with per-direction sub- states that can be later than the direction's own close - a http2 stream completes when both sides closed, while its request buffers are final once the -client sent END_STREAM - so a window can open where that direction's own buffers -are final already. That is deliberate: the tighter bound needs a per-direction -completion the parsers do not expose yet. +client sent END_STREAM - so the bound can lag the point where a miss really is +final. What that costs is one more update at which a group is tested before its +retirement settles; for the http2 cases measured it changes no decision and no +packet, and it keeps one bound for every protocol instead of a per-direction +one per parser. Firewall pipeline ----------------- @@ -483,6 +487,11 @@ of them are worth inspecting. The rules the pattern did not add hold the hook op they are not candidates, so the walk cannot decide them, but the default policy cannot resolve the hook either. +An LTE rule with a packet condition on a variable that a rule can set (``flowbits``, +``xbits``) gets no fast pattern. Whether it is still in the running is only known once +the rules before it in the walk have run, so it is a candidate on every update and the +walk inspects it in order. + :: progress 0 ............ H (hook) C (a miss becomes final) @@ -512,10 +521,9 @@ Two moments, for the rules of a group hooked at ``H``: app hooked rule can only hold patterns of buffers whose engine sits at its hook, so such a buffer cannot grow once the transaction has moved past it. One case does grow: an app buffer of a protocol declaring sub-states, because a frame can rewrite it above the -hook -- an http2 trailer HEADERS frame updates the header list buffers. The buffers of a -request that ended with END_STREAM do not: the parser says where each direction stops -growing, and that is what the revisit keys on. It has no pattern -which covers that update, so all the rules of the group are inspected there. A rule +hook -- an http2 trailer HEADERS frame updates the header list buffers. The rules of such +a group are not retired to their last one: no pattern covers that update, so all of them +are inspected there. A rule with no fast pattern at all has no update which brings it, so a group which holds one is inspected whole there as well. A transaction that ends at or below ``H`` never opens a window: its end state decides, @@ -529,16 +537,9 @@ LIMITATIONS buffer) makes the rule fail to load. The stream is the one buffer that keeps delivering data above any hook, so a miss at the hook would not be final, and the retirement above depends on it being so. Use the buffer keyword of the state you mean. -* A http2 request that ends with a body keeps the packets of a denied host until its - END_STREAM frame. The revisit exists because a trailer can rewrite the header lists, - so a rule hooked below them stays in the running until the request side closes, and a - pending rule appends a packet accept for the packets it holds. main decides on the - first body packet; a buffer-aware revisit - final at the hook unless one of the rule's - buffers can still change - would close the difference and needs a per-buffer flag at - registration. Pinned as it stands by - ``ruletype-firewall-620-lte-http2-body-delivered-until-request-closes``. -* HTTP/1 rewrites ``http.header`` above its hook when a message has a trailer, so a rule - at ``http1: