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: allow rule which need both directions to match #11530

Closed
wants to merge 1 commit into from

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

#11528 with better doc bullet list

Ticket: 5665

This is done with `alert ip any any => any any`
The => operator means that we will need both directions
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

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

Project coverage is 82.51%. Comparing base (6598a69) to head (d808e31).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11530      +/-   ##
==========================================
- Coverage   82.54%   82.51%   -0.03%     
==========================================
  Files         923      923              
  Lines      248460   248607     +147     
==========================================
+ Hits       205083   205139      +56     
- Misses      43377    43468      +91     
Flag Coverage Δ
fuzzcorpus 60.37% <56.31%> (-0.15%) ⬇️
livemode 18.63% <9.22%> (-0.01%) ⬇️
pcap 44.01% <45.63%> (+0.02%) ⬆️
suricata-verify 61.76% <85.92%> (+0.04%) ⬆️
unittests 59.09% <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 21630

@jufajardini jufajardini added the needs rebase Needs rebase to master label Jul 23, 2024
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.

Left minor comments for docs.

Managed to review up until detect-engine.c (around https://github.com/OISF/suricata/pull/11530/files#diff-f0400524fccdb926fe9697ed027b4210459222917a866616235d162b880370f2R1449). So far, seems good!

Following up on a previous PR comment about name suggestions, what about transaction rule? (or something that brought attention to this aspect) Asking also because the code itself refers to <> rules as bidir rules, so if we could avoid possible misunderstandings there, that'd be great.

One question that emerged while reviewing was: what happens if a rule tries to use negated or absent content?

doc/userguide/rules/intro.rst Show resolved Hide resolved
const DetectEngineAppInspectionEngine *app = de_ctx->app_inspect_engines;
for (; app != NULL; app = app->next) {
if (app->sm_list == buf_id && (AppProtoEquals(alproto, app->alproto) || alproto == 0)) {
if (app->dir == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use an enum here to facilitate reading and interpreting? (possibly bigger than the work done here, but if agreed upon, I can create a ticket)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can create an optimization ticket (if there is not one already), and it should be a boolean like if (app->dir_toclient)

Copy link
Contributor

Choose a reason for hiding this comment

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

src/detect-flow.c Show resolved Hide resolved
@inashivb
Copy link
Member

Following up on a previous PR comment about name suggestions, what about transaction rule? (or something that brought attention to this aspect) Asking also because the code itself refers to <> rules as bidir rules, so if we could avoid possible misunderstandings there, that'd be great.

+1

@catenacyber
Copy link
Contributor Author

One question that emerged while reviewing was: what happens if a rule tries to use negated or absent content?

This should work as unidirectional rules that use negated content...
Do you have an example ?

@catenacyber
Copy link
Contributor Author

Thanks Juliana, continued in #11561

@jufajardini
Copy link
Contributor

One question that emerged while reviewing was: what happens if a rule tries to use negated or absent content?

This should work as unidirectional rules that use negated content... Do you have an example ?

I'll try to come up with one (and see if I manage to test it using the corresponding SV branch for the most recent version of this work :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

4 participants