Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect bidir 5665 v14 #11497

Closed
wants to merge 5 commits into from
Closed

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • allows bidirectional signature matching !

SV_BRANCH=OISF/suricata-verify#1922

#11323 with better doc

First version not a draft.
Should I squash all the commits together ?

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.
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

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

Project coverage is 82.65%. Comparing base (223a419) to head (7463037).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11497      +/-   ##
==========================================
+ Coverage   82.56%   82.65%   +0.09%     
==========================================
  Files         938      938              
  Lines      248247   248394     +147     
==========================================
+ Hits       204961   205322     +361     
+ Misses      43286    43072     -214     
Flag Coverage Δ
fuzzcorpus 60.82% <56.31%> (+0.15%) ⬆️
livemode 18.68% <9.22%> (-0.01%) ⬇️
pcap 43.79% <45.63%> (+0.13%) ⬆️
suricata-verify 61.62% <85.92%> (+0.05%) ⬆️
unittests 59.42% <52.42%> (-0.02%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21544

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two documentation suggestions :P

Thanks for including the explanation about how Suricata interprets <> rules!

Following up on the previous PR comment about name suggestions, what about transaction rule? (or something that brought attention to this aspect)

Comment on lines +291 to +293
- They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
the direction to client only if they do have any streaming buffers for detection
on the other side.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is still difficult to know how to read, to me. What about, maybe...

Suggested change
- They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
the direction to client only if they do have any streaming buffers for detection
on the other side.
- They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
the direction to client - only if they do have any streaming buffers for detection
on the other side.

(If that's what this means, ofc XD )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding an example of an impossible rule

@jufajardini
Copy link
Contributor

To me, it looks like all the commits could be squashed, indeed. Or maybe have one for code, and one for docs... 🤔

@catenacyber
Copy link
Contributor Author

Continued in #11528

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.

None yet

3 participants