Detect bidir 5665 v13 #11323

catenacyber · 2024-06-18T14:55:38Z

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

Describe changes:

allows bidirectional signature matching !

SV_BRANCH=OISF/suricata-verify#1922

Draft again because of second commit
General feedback expected :-)

TODO :

Give me better names
Where should I document these new keywords bidir.toclient ?

#11293 rebased with some new test in SV

Ticket: 5665 This is done with `alert ip any any => any any` The => operator means that we will need both directions

By using keywords bidir.toclient and bidir.toserver, the following keywords are forced to the direction even if they can match on both directions. Ticket: 5665

Do not require a rule to use bidir.toserver after the usage of bidir.toclient for unambiguous keywords, where the rule writer does not want to explicit the redundant direction.

codecov · 2024-06-18T15:33:09Z

Codecov Report

Attention: Patch coverage is 89.80583% with 21 lines in your changes missing coverage. Please review.

Project coverage is 82.49%. Comparing base (d59c604) to head (1c4cd67).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #11323    +/-   ##
========================================
  Coverage   82.48%   82.49%            
========================================
  Files         934      934            
  Lines      252239   252386   +147     
========================================
+ Hits       208070   208208   +138     
- Misses      44169    44178     +9

Flag	Coverage Δ
fuzzcorpus	`60.23% <56.31%> (-0.02%)`	⬇️
livemode	`18.73% <9.22%> (-0.01%)`	⬇️
pcap	`43.79% <45.63%> (+0.04%)`	⬆️
suricata-verify	`61.34% <85.92%> (+<0.01%)`	⬆️
unittests	`59.90% <52.42%> (-0.02%)`	⬇️

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

suricata-qa · 2024-06-18T19:30:03Z

Information: QA ran without warnings.

Pipeline 21126

victorjulien · 2024-06-19T06:02:06Z

Comment w/o having reviewed yet: I think we should be extremely restrictive in our parsing. Just accept only the explicitly accepted combinations, and block anything else. Then we can slowly expand support.

victorjulien · 2024-06-19T06:02:27Z

Comment w/o having reviewed yet: I think we should be extremely restrictive in our parsing. Just accept only the explicitly accepted combinations, and block anything else. Then we can slowly expand support.

What I tried, but apparently failed, to do for frames.

catenacyber · 2024-06-19T06:37:49Z

Comment w/o having reviewed yet: I think we should be extremely restrictive in our parsing. Just accept only the explicitly accepted combinations, and block anything else. Then we can slowly expand support.

What I tried, but apparently failed, to do for frames.

:-p I have been doing that since the first version see detect-bidir-impossible SV test with its signatures

Let me know if I missed some

inashivb · 2024-06-24T11:19:27Z

doc/userguide/rules/intro.rst

@@ -252,6 +257,25 @@ as the direction specifies that we do not want to evaluate the response packet.

   There is no 'reverse' style direction, i.e. there is no ``<-``.

+Here is an example of a bidirectional rule:


This is very confusing to me. bidirectional and both dir seem to be interchangeably used throughout the code.
The pre-existing <> operator uses flags like SIG_FLAG_INIT_BIDIREC.. I'm unable to decipher the difference. Could you please explain?

The existing <> is just duplicating the signature in both directions specified by the header
alert tcp 1.2.3.4 any <> 5.6.7.8 80 (sid: 1; etc...)
becomes

alert tcp 1.2.3.4 any -> 5.6.7.8 80 (sid: 1; etc...) alert tcp 5.6.7.8 80 -> 1.2.3.4 any (sid: 1; etc...)

I do not know the use case for it...

This PR with => allows to match on both sides of a transaction like
alert http any any => any any (msg:"matching both uri and status"; sid: 1; http.uri; content: "/download"; http.stat_code; content: "200";)
matching the URI from the client, and the status from the server

Let me know if you have a better wording...

Thank you very much!
After reading up what bidirectional means, I think it makes more sense for the <> operator as we use the same rule in both the directions.
Suggestions for your work:

transaction scoped rule (flags and varnames could be like *tx_scope*)

full transaction match rule (flags and varnames could be like *full_tx_match*)

transaction span rule (flags and varnames could be like *tx_span*)

Interested in hearing your and others' thoughts.
Also, we should make sure that if we make changes, the <> varnames and flags are all named alike as should be the new ones, I think.

Thanks Shivani for the proposals.
I think it is still good to use somehow the word direction to indicate that these rules do not have a direction like others...

catenacyber · 2024-06-25T12:32:55Z

@jufajardini could you please review the documentation ?
Any idea where should I document these new keywords bidir.toclient ?

jufajardini · 2024-07-09T12:57:37Z

@jufajardini could you please review the documentation ? Any idea where should I document these new keywords bidir.toclient ?

Do we see more bidirectional keywords coming? If yes, maybe we could have a separate section, like we have for Flow, Meta keywords, etc. If not, then maybe a subsection of the Direction section could do.

I think that to help illustrate the difference between <> and =>, adding an example for <>, like you did in the explanation to Shivani, would be quite helpful :)

jufajardini · 2024-07-09T12:24:23Z

doc/userguide/rules/intro.rst

+directionality for adresses and ports is used,
+but such a rule can match a bidirectional transaction, using keywords
+matching in each direction.
+However, it is also possible to have a rule match both directions (``<>``)::


Considering Shivani's comments, Philippe's answers, and just this last sentence that we have in our docs, I'm thinking that maybe changing this to:

Suggested change

However, it is also possible to have a rule match both directions (``<>``)::

However, it is also possible to have a rule match either directions (``<>``)::

I'm wondering if maybe we can't come up with a different name for this new feature, as maybe bidirectional is already too known for the interchangeability of source and destination? Or would this be too disruptive for us?

If you find a good name, let me know ;-)

jufajardini · 2024-07-09T12:46:54Z

doc/userguide/rules/intro.rst

+Bidirectional rules have some limitations :
+- They are only meant to work on transactions with first a request to the server,
+and then a response to the client, and not the other way around.
+- They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
+the direction to client.
+- They will not work with ambiguous keywords, which work for both directions.
+- They will refuse to load if a single directional rule is enough.
+


Maybe this could become a note, or a subsection, as it's something specific to bidirectional rules?

jufajardini · 2024-07-09T12:48:56Z

doc/userguide/rules/intro.rst

@@ -278,8 +278,9 @@ Bidirectional rules can use direction-abmibuous keywords, by first using
 Bidirectional rules have some limitations :
 - They are only meant to work on transactions with first a request to the server,
 and then a response to the client, and not the other way around.
- They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
-the direction to client.
+- They cann have ``fast_pattern`` or ``prefilter`` on a keyword which is on


nit: s/cann/can

jufajardini · 2024-07-09T12:50:40Z

doc/userguide/rules/intro.rst

+It is also possible to have a double arrow (``=>``) which means that the
+directionality for adresses and ports is used,
+but such a rule can match a bidirectional transaction, using keywords
+matching in each direction.


suggestion of alternative text: "There is also the double arrow (=>), which respects the directionality as ->, but allows matching on bidirectional transactions, used with keywords matching each direction."

Question: would it be possible to say that using => has a similar effect as having two rules (one for each side of the bidir transaction) and setting flowbits to only have an alert in case both match?

would it be possible to say that using => has a similar effect as having two rules (one for each side of the bidir transaction) and setting flowbits to only have an alert in case both match?

It is the goal indeed cf tests/detect-bidir-flowbits/ in OISF/suricata-verify#1922

The goal is to be better as these are txbits, not flowbits ;-)

jufajardini · 2024-07-09T12:56:03Z

doc/userguide/rules/intro.rst

+- They are only meant to work on transactions with first a request to the server,
+and then a response to the client, and not the other way around.
+- They cann have ``fast_pattern`` or ``prefilter`` on a keyword which is on
+the direction to client only if they do not have any have streaming buffers


Suggested change

the direction to client only if they do not have any have streaming buffers

the direction to client only if they do not have any streaming buffers

if they do have ;-)

catenacyber · 2024-07-15T11:42:25Z

@jufajardini could you please review the documentation ? Any idea where should I document these new keywords bidir.toclient ?

Do we see more bidirectional keywords coming? If yes, maybe we could have a separate section, like we have for Flow, Meta keywords, etc. If not, then maybe a subsection of the Direction section could do.

I do not see anymore coming.
So subsection of Direction sounds good :-)

I think that to help illustrate the difference between <> and =>, adding an example for <>, like you did in the explanation to Shivani, would be quite helpful :)

Adding something

catenacyber · 2024-07-15T11:51:43Z

Thanks for the tips, continued in #11497

catenacyber added 4 commits June 18, 2024 16:21

detect: allow rule which need both directions to match

eaa84bd

Ticket: 5665 This is done with `alert ip any any => any any` The => operator means that we will need both directions

detect: bidirectional rules can use dir-ambiguous keywords

32c7ab9

By using keywords bidir.toclient and bidir.toserver, the following keywords are forced to the direction even if they can match on both directions. Ticket: 5665

detect: bidirectional rules allow fast_pattern to client

a54eb9b

detect/bidir:be more friendly for directions and keywords

1c4cd67

Do not require a rule to use bidir.toserver after the usage of bidir.toclient for unambiguous keywords, where the rule writer does not want to explicit the redundant direction.

catenacyber mentioned this pull request Jun 18, 2024

Detect bidir 5665 v12 #11293

Closed

victorjulien self-assigned this Jun 19, 2024

inashivb reviewed Jun 24, 2024

View reviewed changes

jufajardini reviewed Jul 9, 2024

View reviewed changes

catenacyber mentioned this pull request Jul 15, 2024

Detect bidir 5665 v14 #11497

Closed

catenacyber closed this Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect bidir 5665 v13 #11323

Detect bidir 5665 v13 #11323

catenacyber commented Jun 18, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading

suricata-qa commented Jun 18, 2024

victorjulien commented Jun 19, 2024

victorjulien commented Jun 19, 2024

catenacyber commented Jun 19, 2024

inashivb Jun 24, 2024

catenacyber Jun 24, 2024

inashivb Jun 25, 2024

catenacyber Jun 25, 2024

catenacyber commented Jun 25, 2024

jufajardini commented Jul 9, 2024

jufajardini Jul 9, 2024

jufajardini Jul 9, 2024

catenacyber Jul 15, 2024

jufajardini Jul 9, 2024

catenacyber Jul 15, 2024

jufajardini Jul 9, 2024

catenacyber Jul 15, 2024

jufajardini Jul 9, 2024

catenacyber Jul 15, 2024

jufajardini Jul 9, 2024

catenacyber Jul 15, 2024

catenacyber commented Jul 15, 2024

catenacyber commented Jul 15, 2024

		@@ -252,6 +257,25 @@ as the direction specifies that we do not want to evaluate the response packet.

		There is no 'reverse' style direction, i.e. there is no ``<-``.

		Here is an example of a bidirectional rule:

	However, it is also possible to have a rule match both directions (``<>``)::
	However, it is also possible to have a rule match either directions (``<>``)::

	the direction to client only if they do not have any have streaming buffers
	the direction to client only if they do not have any streaming buffers

Detect bidir 5665 v13 #11323

Detect bidir 5665 v13 #11323

Conversation

catenacyber commented Jun 18, 2024

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

suricata-qa commented Jun 18, 2024

victorjulien commented Jun 19, 2024

victorjulien commented Jun 19, 2024

catenacyber commented Jun 19, 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

catenacyber commented Jun 25, 2024

jufajardini commented Jul 9, 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

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

catenacyber commented Jul 15, 2024

catenacyber commented Jul 15, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading