detect: allow bypass keyword for fw rules in firewall mode (only) - v6 - #16120
jufajardini wants to merge 15 commits into
Conversation
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
Part of Ticket: OISF#8459
Part of Ticket: OISF#8459
Related to Ticket: OISF#8943
Related to 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
|
AI-generated review posted automatically by Suricata ai-review. Verdict: request changes
|
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 33377 |
|
Please elaborate on: 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. Are you referring to the fact that the bypass could happen and no alert would be seen? |
|
Please elaborate on: 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. Are you referring to the fact that the bypass could happen and no alert would be seen? |
Thanks, hehe, I noticed that I had not done this correctly when I was in bed, trying to sleep 😓 |
Question from @jufajardini:
Yes—the missing alert is a symptom, but the larger issue is that the flow is bypassed prematurely. Thus, below the configured rate, the firewall rule does not apply or alert, yet the flow still leaves inspection and same-packet and subsequent threat detection are skipped. Bypass should occur only after |
lukashino
left a comment
lukashino
left a comment
There was a problem hiding this comment.
Looks good, just not sure about the last commit.
I approve the preceding comments.
The last commit also has a pretty chatty comments/commit message too.
| will be incremented. No ``ips.*`` counter will be updated as conceptually the TD | ||
| instance won't have seen the packet. | ||
|
|
||
| .. note:: If a firewall rule uses the :ref:`bypass keyword<bypass-keyword>`, an |
There was a problem hiding this comment.
Just putting this here but perhaps bypass keyword could also be mentioned in the "Actions and Action Scopes" section as a secondary action. I recently added the config keyword too.
There was a problem hiding this comment.
🙇🏽 It is indeed a follow-up work to have it as a secondary action, explicitly.
| * | ||
| * A threat detection rule can match a packet that a firewall rule then | ||
| * bypasses. The alert is dropped during alert finalization, but the post-match | ||
| * list has already run at match time. Flow-scoped entries are harmless -- the |
There was a problem hiding this comment.
Do we need to introduce an exemption path (deferred post match) for TD in FW mode and cannot we just defer it for all modes?
I don't see an argument why not exempt the mention race for cross-flow detection (which we neither handle nor measure anyway)
| * Independent of the rule's action: `bypass` is a keyword, not an action, and is not tied to | ||
| * `accept` | ||
| */ | ||
| static inline bool AlertQueueHasFirewallBypass( |
There was a problem hiding this comment.
Also, instead of iterating over alert queue twice for most packets (in this function + the usual TD processing for loop that follows after this function call), can we perhaps set the bypass flag directly to packet structure?
E.g.:
static int DetectBypassMatch(DetectEngineThreadCtx *det_ctx, Packet *p,
const Signature *s, const SigMatchCtx *ctx)
{
PacketBypassCallback(p);
if ((s->flags & SIG_FLAG_FIREWALL) &&
p->flow != NULL && !PKT_IS_PSEUDOPKT(p) && FlowIsBypassed(p->flow)) {
p->flags |= PKT_FW_BYPASSED;
}
return 1;
}4 participants
Previous PR: #16077
Describe changes:
detection_filterstill 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)Items in bold are the most critical changes, imho.
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/8459
SV_BRANCH=OISF/suricata-verify#3321