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

@@ -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 (``<hook``) cannot match the raw stream: a
+  ``content`` or ``pcre`` on the stream (including a bare ``content`` before any sticky
+  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:<request_headers`` whose pattern only appears in a trailer is retired at the
+  first update above the hook and the default policy of that state decides before the
+  trailer is inspected. This is a known gap, open with the design discussion of the
+  retirement bound.

src/detect-engine-prefilter.c

@@ -959,10 +959,11 @@ static void TxNonPFFree(void *data)
 /** \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. */
+ *  once the tx is past the hook and a miss there is final. One case escapes
+ *  that: a protocol which declares sub-states rewrites a buffer above its hook
+ *  - an http2 trailer HEADERS frame updates the header buffers. A raw stream
+ *  pattern cannot get here: DetectFirewallRuleValidate() refuses an LTE rule
+ *  with anything on the payload list. */
 static enum FwLteGrows FwLtePatternGrowsAboveBound(const Signature *s)
 {
     if (s->init_data->mpm_sm == NULL) {

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 ('<hook') cannot match the raw "
+                       "stream: its match must come from a buffer of that hook, so that a "
+                       "miss at the hook is final",
+                    s->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))) {

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:<request_headers any any -> 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:<request_headers any any -> 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:<request_headers any any -> 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:<request_headers any any -> 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);