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 #10252

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=pr/1603

OISF/suricata-verify#1603

Not so much a draft anymore...
But I still expect to see next iterations...

TODO :

  • more tests !!!! Throw me rules examples ! negative and positive...
  • think about solution for ambiguous-direction keywords (like new to_client and to_server keywords that are not in flow keyword, but only apply to a previous keyword). Here, it is a documented limitation...

#10242 rebased to get green CI

@catenacyber
Copy link
Contributor Author

Fixup for the warning

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 Jan 25, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c3b3c11) 82.28% compared to head (e57e501) 82.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10252      +/-   ##
==========================================
- Coverage   82.28%   82.28%   -0.01%     
==========================================
  Files         977      977              
  Lines      271950   272011      +61     
==========================================
+ Hits       223784   223828      +44     
- Misses      48166    48183      +17     
Flag Coverage Δ
fuzzcorpus 63.38% <61.40%> (-0.02%) ⬇️
suricata-verify 61.52% <90.35%> (-0.01%) ⬇️
unittests 62.81% <60.52%> (-0.01%) ⬇️

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 17766

@zoomequipd
Copy link

I've very excited by this feature, but I noticed some limitations that I'd like to confirm my understanding

    • They cannot have fast_pattern or prefilter on a keyword which is on the direction to client.

This would mean that using values such has http.response_body, http.response_header, etc could not be used as a fast_pattern?

so a rule such this one, would not be allowed?
alert http $HOME_NET any => $EXTERNAL_NET any (msg:"EXE masquerading as PNG"; http.uri; content:".png"; endswith; http.response_body; content:"MZ"; startswith; content:"!This program cannot"; distance:0; fast_pattern; ...

Instead, the fast_pattern would have to be .png?

    • They will not work with ambiguous keywords, which work for both directions.

This would apply to keywords such as http.cookie, http.header, http.connection, http.header_names?
Because these keywords are so heavily used, the inability to use these keywords would result in a lot of "unbuffered" content matches within bi-directional rules. This almost seems like a step backwards.


After reading through the docs and attempting to write a bi directional rule, I was left wondering what values should be used for the src/dst host/port variables as well as the flow direction.

Given this limitation

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.

I imagine that the src/dst host/port vars should be reflective of the client (in a tcp sense) initiating a session with the server. and that the flow keyword directional option should always be either "to_server" or "from_client"?

If so, this might be more clearly indicated in the docs and validated within the rules.

@catenacyber
Copy link
Contributor Author

but I noticed some limitations that I'd like to confirm my understanding

Thanks for you interest @zoomequipd

As a foreword, I would like to say that these limitations are the limitations of the current PR.
I do not know if we will overcome them, but we will try...
Maybe a first version will not lift all the limitations, but still allow some bidirectional rules...
And I am not 100% confident that this PR even works in all the limited cases...

This would mean that using values such has http.response_body, http.response_header, etc could not be used as a fast_pattern?

Your understanding is correct, the fast_pattern would have to be .png...

This will be hard to tackle because of streaming buffer like http.request_body...

This would apply to keywords such as http.cookie, http.header, http.connection, http.header_names?

Yes, this is correct.
This is the next limitation I want to work on

I was left wondering what values should be used for the src/dst host/port variables as well as the flow direction.

Like a normal directional rule alert scrip srcport -> dstip dstport...
Does that answer your question ?

The limitation (linked with the fast_pattern one) is that you cannot match on HTTP2 push promise, where you get first the data from the server, then the data from the client...

flow keyword directional option should always be either "to_server" or "from_client"?

You cannot use flow: to_server or flow:to_client in a bidirectional rule, because the rule will intrinsically match both directions, not only one.

I feel there is something unclear to you.
Do you have a rewording proposal for the docs ?

@catenacyber catenacyber added the needs rebase Needs rebase to master label Feb 2, 2024
@catenacyber
Copy link
Contributor Author

Continued in #10506

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.

3 participants