Skip to content

detect: allow bypass keyword for fw rules in firewall mode (only) - v6 - #16120

Open
jufajardini wants to merge 15 commits into
OISF:mainfrom
jufajardini:fw-8459-allow-bypass/v6
Open

detect: allow bypass keyword for fw rules in firewall mode (only) - v6#16120
jufajardini wants to merge 15 commits into
OISF:mainfrom
jufajardini:fw-8459-allow-bypass/v6

Conversation

@jufajardini

@jufajardini jufajardini commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Previous PR: #16077

Describe changes:

  • rebased
  • banned threshold keyword and threshold.config from firewall rules. detection_filter still allowed, and it doesn't seem affected by the other issues threshold seemed to show, with fw rules. (this seemed easier than banning something that was already working, to try to bring it back later on)
  • added a doc section for banned keywords in firewall mode
  • deferring post-match state writing to after firewall verdict -- allows xbits, mark, hostbits to work, but changes when the state gets written to packet
  • rebased

Items in bold are the most critical changes, imho.

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/8459

SV_BRANCH=OISF/suricata-verify#3321

In case a keyword should work in firewall mode, with firewall rules
only. The engine errors out if threat detection rules use the given keyword.

Part of
Ticket OISF#8459
The bypass keyword should work in firewall mode, with firewall rules,
only. The engine errors out if threat detection rules use said keyword.

Ticket: OISF#8459
If we add more firewall-related rule options, we can keep this opaque to
SigParseOptions.

Part of
Ticket: OISF#8459
This allows banning variations of `action`:`scope` for specific
keywords.
Mostly having firewall rules in mind.

Done by introducing several SIGMATCH flags, to cover:
actions:
- config
- drop
- reject
action scopes:
- packet
- tx
- hook

`accept` and `flow` were left out as they would not be used for the work
at hand.

Part of
Ticket: OISF#8459
A firewall rule only accepts the `bypass` keyword with the combination
of `accept:flow`. Thus, ban: `drop`, `reject`, `config`, `hook`, `tx`
and `packet` from firewall usage for this keyword.

Part of
Ticket: OISF#8459
Especially related to firewall mode.

As part of
Ticket: OISF#8459
If a firewall rule sets a flow to be bypassed, the triggering packet
could still be inspected by a threat detection rule with a drop.

Avoid that the `accept` from the firewall rule would still allow a TD
`drop` to be applied to the first packet.

This also implies that the stats for accept in such cases will now
differ between firewall and IPS, as the firewall accepted+bypassed
packet is never seen by ips (so can't be accepted).

Related to
Ticket: OISF#8459
A packet bypassed by the firewall can't lead to ips stats counters
increments.
For a accept+bypass from the firewall, this implies that the stats for
accept in such cases will now differ between firewall and IPS, as the
firewall accepted+bypassed packet is never seen by ips (so can't be
counted as accepted).

Part of
Ticket: OISF#8459
Threshold can't be used with firewall rules, be it in a rule, or from
the threshold.config file.

Related to:
Ticket: OISF#8917

Part of
Ticket: OISF#8459
In firewall mode, a threat detection rule can match a packet that a
firewall rule then bypasses. The bypass is honored during alert queue
finalization, which withholds the alert, but the rule's post-match list
has already run at match time: packet:td rules are evaluated before the
app:filter rule that applies the bypass.

Flow-scoped post-match entries are harmless, as the flow is leaving
inspection. Host, ip-pair and packet-mark state is not: xbits with an
ip_src/ip_dst/ip_pair tracker, hostbits and nfq_set_mark outlive the
flow, and can affect later firewall decisions for a packet that should
never have reached threat detection.

Mark those cases at rule-build time, defer them at match time, and apply
them from the alert queue once the firewall verdict is known. Marking is
only done for threat detection rules in firewall mode, so IDS mode and
firewall rules are unaffected. Applying them before thresholding
preserves the existing behavior that a thresholded rule still sets its
state; only a bypass suppresses it.

Important effects:
As a consequence, an affected post-match write now lands at the end of
the packet's processing, rather than mid-match. Ordering within a flow
is unchanged. Cross-flow observers were never ordered against this
write. Under autofp they are in a different worker thread. But the later
write from this patch makes losing that race considerably more likely in
practice, so rules that track host state across flows may see it less
reliably than before. suricata-verify tests that observe this state
across flows need `--runmode single` to be deterministic.

Related to
Ticket: OISF#8459
@suricata-review-bot

Copy link
Copy Markdown

AI-generated review posted automatically by Suricata ai-review.

Verdict: request changes

  • High — src/detect-engine-alert.c:683: Bypass is honored before detection_filter is evaluated. A below-rate accept:flow,bypass rule still bypasses the flow and skips TD, enabling inspection evasion. Gate bypass on the filter result or reject this combination; test that early matches do not bypass.

  • Medium — src/detect-engine-build.c:2273: IP-only rules ignore the deferred marker and execute post-match state before finalization. Bypassed packets can retain xbits/hostbits, while non-bypassed toggle operations execute twice and cancel themselves. Honor deferral in the IP-only runner and test both cases.

  • Medium — src/detect-engine-build.c:2225: Deferral excludes dataset:set/unset, which mutate global or persisted state during buffer matching. A packet later bypassed by an app-layer firewall rule can therefore poison state used by subsequent firewall decisions. Stage or reject these mutations in this mode and add bypass coverage.

  • Medium — src/detect.c:275: Deferring host/xbits writes until finalization breaks same-packet dependencies. Later isset/isnotset rules observe stale state even without a bypass, potentially suppressing alerts or drops. Preserve intra-pass visibility and test an ordered writer/reader pair.

  • Medium — src/detect-engine-alert.c:620: A pre_stream bypass is recognized only in that hook’s alert queue. The main detection pass resets the queue and evaluates TD again despite the carried bypass state, allowing alerts and state mutations on a supposedly terminating packet. Carry the verdict into the main pass or ban bypass in special hooks; add a pre_stream regression test.

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 65.59140% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.02%. Comparing base (7c66954) to head (92c312b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16120      +/-   ##
==========================================
- Coverage   83.06%   83.02%   -0.04%     
==========================================
  Files        1004     1004              
  Lines      277376   277553     +177     
==========================================
+ Hits       230389   230444      +55     
- Misses      46987    47109     +122     
Flag Coverage Δ
fuzzcorpus 61.46% <22.58%> (-0.05%) ⬇️
livemode 18.40% <14.51%> (-0.03%) ⬇️
netns 22.86% <23.65%> (-0.02%) ⬇️
pcap 45.35% <20.43%> (-0.08%) ⬇️
suricata-verify 67.12% <63.97%> (-0.04%) ⬇️
unittests 58.50% <22.58%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa

Copy link
Copy Markdown

Information: QA ran without warnings.

Pipeline = 33377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants