detect: allow rule which need both directions to match #11578
Conversation
Ticket: 5665 This is done with `alert ip any any => any any` The => operator means that we will need both directions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11578 +/- ##
==========================================
+ Coverage 82.50% 82.51% +0.01%
==========================================
Files 923 923
Lines 248721 248868 +147
==========================================
+ Hits 205215 205364 +149
+ Misses 43506 43504 -2
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 21764 |
| 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 (not tested). |
There was a problem hiding this comment.
Lol : I wish I had a test for this :-)
Where can I get a Suricata transaction that is initiated by the server ?
This is untested because I do not know a test case for it...
There was a problem hiding this comment.
smtp at least has first data from the server generally, not sure if that leads to a tx
There was a problem hiding this comment.
I guess there are other limitations to consider: async oneside means no matches. Also, midstream means we may pick a tx up on response only? So that wouldn't match either.
This is true for current rules that use flowbits as well of course, but it's good to point it out I think
There was a problem hiding this comment.
smtp at least has first data from the server generally
yes
not sure if that leads to a tx
Indeed it does not :-/
async oneside means no matches
Yes, should we refuse to load ?
Also, midstream means we may pick a tx up on response only? So that wouldn't match either.
Indeed, but just like a request with the connection ending before getting an answer would not match
There was a problem hiding this comment.
Investigated HTTP2 push more closely and I created https://redmine.openinfosecfoundation.org/issues/7317
It looks like a unidirectional tx from server...
| As such, it will match only when Suricata got both request and response. | ||
|
|
||
| Bidirectional rules can use direction-ambiguous keywords, by first using | ||
| ``bidir.toclient`` or ``bidir.toserver`` keywords. |
There was a problem hiding this comment.
not a fan of this additional keyword, can we use the flow keyword for it? Also, I guess I'm not even sure what they are used for? Can you explain it?
There was a problem hiding this comment.
Also, I guess I'm not even sure what they are used for?
This is important !
These are helper to the rule parser to know for ambiguous keywords such as http.connection (which can happen on both directions) which direction to use.
Like http.uri; content: "/download"; http.stat_code; content: "200"; bidir.toclient; http.connection; content: "eep";
means that we should use the http.connection; content: "eep"; sent to the client
Do you have a better idea or a better naming ?
There was a problem hiding this comment.
Ok, I see. I think an alternative would be to only allow => rules for non-ambiguous keywords, so keywords that have explicit directionality. E.g. instead of ... http ... => ... file.data; ... force ppl to use ... http ... => ... http.request_body; ....
There was a problem hiding this comment.
Sure, v1 was like that, but you told me to remove limitations because signature writers would not want them :-p
There was a problem hiding this comment.
Maybe reusing flow: toserver instead of introducing bidir.toserver is a better idea...
| @@ -1167,6 +1167,8 @@ static void FlagDetectStateNewFile(HtpTxUserData *tx, int dir) | |||
| SCLogDebug("DETECT_ENGINE_STATE_FLAG_FILE_NEW set"); | |||
| tx->tx_data.de_state->dir_state[1].flags |= DETECT_ENGINE_STATE_FLAG_FILE_NEW; | |||
| } | |||
| tx->tx_data.de_state->dir_state[DETECT_ENGINE_STATE_DIRECTION_BOTHDIR].flags |= | |||
There was a problem hiding this comment.
side note: line 1166 } else if (STREAM_TOCLIENT) { looks highly suspicious
There was a problem hiding this comment.
Argh : -Wtype-limits Warn if a comparison is always true or always false ... but do not warn for constant expressions.
| @@ -1206,7 +1212,19 @@ static bool DetectRunTxInspectRule(ThreadVars *tv, | |||
| inspect_flags |= BIT_U32(engine->id); | |||
| } | |||
| break; | |||
| } else if (!(inspect_flags & BIT_U32(engine->id)) && s->flags & SIG_FLAG_BOTHDIR && | |||
| direction != engine->dir) { | |||
| // for bidirectional rules, the engines on the opposite direction | |||
There was a problem hiding this comment.
can this be explained more / differently? Not following what is happening.
There was a problem hiding this comment.
For bidirectional rules, we want to continue and try all the engines we can
s->app_inspect are ordered by tx progress, but tx progress is directional.
So you may have a first engine in s->app_inspect which is to_client, but you get to inspect the request first.
So, you should skip this engine not in the good direction, but still continue to try other engines
Does it make sense ?
There was a problem hiding this comment.
I would expect app engines in this list to be ordered by direction and progress, so first toserver 0-N, then toclient 0-N. Is this not how it works?
There was a problem hiding this comment.
I would expect app engines in this list to be ordered by direction and progress, so first toserver 0-N, then toclient 0-N. Is this not how it works?
No, it does not work like that, to server and toclient are intertwined by AppendAppInspectEngine
And it looks even more complex with the fast_pattern logic that forces one buffer to be first
There was a problem hiding this comment.
cf SV test rule alert http any any => any any (msg:"fast_pattern on to_client side"; sid: 7; http.uri; content: "down"; http.server; content: "Apache"; fast_pattern;) that will first try http.server
Debug print in DetectRunTxInspectRule
+ while (engine != NULL) {
+ printf("lol %d %s\n", s->id, DetectEngineBufferTypeGetNameById(de_ctx, engine->sm_list));
+ engine = engine->next;
+ }
+ engine = s->app_inspect;
|
Continued in #11900 |
Thanks :-) good questions |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5665
Describe changes:
SV_BRANCH=OISF/suricata-verify#1922
#11561 with needed rebase to get green CI