Flow bytes pkts either support/v4 #11897

inashivb · 2024-10-08T07:16:04Z

Link to ticket: https://redmine.openinfosecfoundation.org/issues/5646

SV_BRANCH=OISF/suricata-verify#2082

Previous PR: #11889

Changes since v3:

Kept the old syntax as that is more in line w schema as per internal discussion
rebased on top of latest master

Add an extension to keywords flow.bytes.. and flow.pkts.. to allow matching on bytes or pkts in either direction. The syntax for this operation would look like the following: flow.bytes_either:1000 flow.pkts_either:20 These are implemented as generic uint types and thus allow all basic ops in the syntax like greater than, less than, etc alongwith the exact match. Feature 5646

Feature 5646

codecov · 2024-10-08T07:50:51Z

Codecov Report

Attention: Patch coverage is 69.33333% with 23 lines in your changes missing coverage. Please review.

Project coverage is 82.61%. Comparing base (6ae5ae7) to head (f4bc8c7).
Report is 120 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11897   +/-   ##
=======================================
  Coverage   82.60%   82.61%           
=======================================
  Files         912      912           
  Lines      249342   249417   +75     
=======================================
+ Hits       205968   206046   +78     
+ Misses      43374    43371    -3

Flag	Coverage Δ
fuzzcorpus	`60.66% <25.33%> (+0.02%)`	⬆️
livemode	`18.72% <25.33%> (+<0.01%)`	⬆️
pcap	`44.08% <25.33%> (+0.01%)`	⬆️
suricata-verify	`62.01% <69.33%> (-0.02%)`	⬇️
unittests	`58.93% <25.33%> (-0.01%)`	⬇️

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

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

suricata-qa · 2024-10-08T14:00:04Z

Information: QA ran without warnings.

Pipeline 23041

victorjulien · 2024-10-12T08:19:27Z

src/detect-flow-pkts.c

+    if (p->flow == NULL) {
+        return 0;
+    }
+    uint32_t nb = p->flow->tosrcpktcnt;


can avoid using nb and just use p->flow->tosrcpkt_cnt directly?

victorjulien · 2024-10-12T08:20:15Z

src/detect-flow-pkts.c

+    if (p->flow == NULL) {
+        return 0;
+    }
+    uint64_t nb = p->flow->tosrcbytecnt;


same as with pkts

victorjulien

See inline comments.

victorjulien · 2024-10-12T08:23:06Z

Still not entirely fan of the keyword itself, as I feel it is better to have it as an option somehow. But since the existing keywords map to eve, and adding the option there doesn't make sense, I don't know a better way right now.

Or we'd have change eve to do something like:

flow.pkts.toserver
flow.pkts.toclient

then keywords could be

flow.pkts:toserver
flow.pkts:toclient
flow.pkts:either

Hmmm... not a perfect match either. So, I remain undecided...

jufajardini · 2024-10-15T17:17:05Z

Still not entirely fan of the keyword itself, as I feel it is better to have it as an option somehow. But since the existing keywords map to eve, and adding the option there doesn't make sense, I don't know a better way right now.

Or we'd have change eve to do something like:
flow.pkts.toserver
flow.pkts.toclient
then keywords could be
flow.pkts:toserver
flow.pkts:toclient
flow.pkts:either
Hmmm... not a perfect match either. So, I remain undecided...

Imho, this alternative seems easier to correlate and understand.

jufajardini · 2024-10-15T16:45:30Z

doc/userguide/rules/flow-keywords.rst

+flow.pkts_either
+------------------
+
+Flow number of packets in either direction (integer)


nit:

Suggested change

Flow number of packets in either direction (integer)

Flow number of packets in either direction (integer).

I wonder if it is needed to add that it is either, but not the some of both - or if it is clear enough.

I think mathematically either is different than both or/and sum. But, if it seems like it can confuse new people, sure. Do you have suggestions on incorporating it in the sentence?

jufajardini · 2024-10-15T16:48:33Z

doc/userguide/rules/flow-keywords.rst

+ flow.pkts_either:3    # exactly 3
+ flow.pkts_either:<3   # smaller than 3
+ flow.pkts_either:>=2  # greater than or equal to 2


Is this list non-exhaustive? Should we somehow indicate the other possible operations (even if by just adding a "for example", at the end of the sentence?

Well, we do link to all the possible ops in the ref tag above. It leads to this elaborate doc: https://docs.suricata.io/en/latest/rules/integer-keywords.html#rules-integer-keywords.
Lmk if this does not seem enough

jufajardini · 2024-10-15T16:49:42Z

doc/userguide/rules/flow-keywords.rst

+
+Signature example::
+
+ alert ip any any -> any any (msg:"Flow has greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)


nit:

Suggested change

alert ip any any -> any any (msg:"Flow has greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)

alert ip any any -> any any (msg:"Flow is greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)

inashivb · 2024-10-16T05:13:48Z

Still not entirely fan of the keyword itself, as I feel it is better to have it as an option somehow. But since the existing keywords map to eve, and adding the option there doesn't make sense, I don't know a better way right now.

Or we'd have change eve to do something like:
flow.pkts.toserver
flow.pkts.toclient
then keywords could be
flow.pkts:toserver
flow.pkts:toclient
flow.pkts:either

Just adding that the previous version of this PR had exactly this syntax for rule keywords to have direction as an option. eve was unchanged though

inashivb · 2024-11-22T15:55:38Z

As discussed internally, we'll keep the syntax of previous PR

victorjulien · 2024-11-22T15:59:24Z

As discussed internally, we'll keep the syntax of previous PR

But, also keep flow.pkts_toserver and flow.pkts_toclient for now. Same for bytes.

inashivb added 2 commits October 8, 2024 12:39

doc: add entry for flow.bytes_either & flow.pkts_either

f4bc8c7

Feature 5646

inashivb requested review from jufajardini and victorjulien as code owners October 8, 2024 07:16

inashivb requested review from catenacyber and removed request for jufajardini and victorjulien October 8, 2024 07:18

inashivb changed the title ~~Flow bytes pkts syntax/v4~~ Flow bytes pkts either support/v4 Oct 8, 2024

victorjulien reviewed Oct 12, 2024

View reviewed changes

victorjulien requested changes Oct 12, 2024

View reviewed changes

jufajardini reviewed Oct 15, 2024

View reviewed changes

victorjulien added the decision-required Waiting on deliberation from the team label Oct 24, 2024

inashivb closed this Nov 22, 2024

inashivb deleted the flow-bytes-pkts-syntax/v4 branch November 25, 2024 06:25

inashivb mentioned this pull request Nov 29, 2024

Flow bytes pkts syntax/v5 #12180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flow bytes pkts either support/v4 #11897

Flow bytes pkts either support/v4 #11897

inashivb commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

suricata-qa commented Oct 8, 2024

victorjulien Oct 12, 2024

victorjulien Oct 12, 2024

victorjulien left a comment

victorjulien commented Oct 12, 2024

jufajardini commented Oct 15, 2024

jufajardini Oct 15, 2024

jufajardini Oct 15, 2024

inashivb Oct 16, 2024

jufajardini Oct 15, 2024

inashivb Oct 16, 2024

jufajardini Oct 15, 2024

inashivb commented Oct 16, 2024

inashivb commented Nov 22, 2024

victorjulien commented Nov 22, 2024

	Flow number of packets in either direction (integer)
	Flow number of packets in either direction (integer).


		Signature example::

		alert ip any any -> any any (msg:"Flow has greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)

	alert ip any any -> any any (msg:"Flow has greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)
	alert ip any any -> any any (msg:"Flow is greater than 3000 bytes in either dir"; flow.bytes_either:>3000; sid:1;)

Flow bytes pkts either support/v4 #11897

Flow bytes pkts either support/v4 #11897

Conversation

inashivb commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

suricata-qa commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien left a comment

Choose a reason for hiding this comment

victorjulien commented Oct 12, 2024

jufajardini commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inashivb commented Oct 16, 2024

inashivb commented Nov 22, 2024

victorjulien commented Nov 22, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading