dns: new keywords: dns.answer.name, dns.query.name#9795
dns: new keywords: dns.answer.name, dns.query.name#9795jasonish wants to merge 9 commits intoOISF:masterfrom
Conversation
The old DetectAppLayerMpmRegister has not been around since 4.1.x. Rename the v2 of this function to a versionless function as there is no documentation referring to what the 2 means.
…ngine Rename DetectAppLayerInspectEngine2 to DetectAppLayerInspectEngine as there is no other variant of this function, and the versioning with lack of supporting documentation can lead to confusion.
Version 1 of the API no longer exists.
This sticky buffer will allow content matching on the answer names. While ansers typically only occur in DNS responses, we allow the buffer to be used in request context as well as the request message format allows it. Feature: OISF#6496
This buffer is much like dns.query_name but allows for detection in both directions. Feature: OISF#6497
With some other minor cleanups in the DNS keyword section.
SCDnsTxGetQueryName was introduced to allow for getting the query name in responses as well as requests, so covers the functionality of rs_dns_tx_get_query_name.
| if (!SCDnsTxGetAnswerName(txv, index, &data, &data_len)) { | ||
| InspectionBufferSetupMultiEmpty(buffer); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Missing the direction here as we currently don't parse answers, etc in DNS requests. But the message structure fully allows for it, so I'm thinking we should parse it if its there.
There was a problem hiding this comment.
you mean answers in the request? Lets at a minimum get a test case + event for it.
wonder if we should make the keywords include directionality in the name, e.g. dns.request.query. I would be confused if answers would match on a request packet, even if from the dns record pov it would be correct.
There was a problem hiding this comment.
That is the path I first went down, dns.request.query.name, dns.response.query.name, etc. But don't we have the flow keyword to express the directionality?
There was a problem hiding this comment.
you mean answers in the request? Lets at a minimum get a test case + event for it.
Converting this to draft as I mostly did complete support for answers in requests yesterday with an SV test, so worth getting that in to avoid some confusing comments.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9795 +/- ##
==========================================
- Coverage 82.43% 82.32% -0.11%
==========================================
Files 968 970 +2
Lines 273866 273944 +78
==========================================
- Hits 225758 225529 -229
- Misses 48108 48415 +307
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 16541 |
|
|
||
| /// Get the DNS query name at index i. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn SCDnsTxGetQueryName( |
There was a problem hiding this comment.
why do we break the rust style here (and previously below)?
There was a problem hiding this comment.
Do you mean the off by one on the indent? Not sure how that happened, I rust formatted the new code, but didn't take in some new formatting.
| keyword works a bit different from the normal content modifiers. When | ||
| used in a rule all contents following it are affected by it. Example: | ||
| With **dns.query** the DNS request query names are inspected. The | ||
| dns.query keyword works a bit different from the normal content |
There was a problem hiding this comment.
"this works a bit different"-bit could be removed. It's just about sticky buffer vs content modifier
There was a problem hiding this comment.
Given we're at 8 maybe we should clean this up. Just refer to it as a sticky buffer and not much more?
|
|
||
| ``dns.query.name`` is a sticky buffer that is used to look at the name | ||
| field in DNS query (question) resource records. It is nearly identical | ||
| to ``dns.query`` but supports both DNS requests and responses. |
There was a problem hiding this comment.
Guess this makes things clear enough.
|
Replaced by #9813. |
4 participants
Introduce two new DNS keywords, dns.query.name and dns.answer.name.
Tickets:
SV_BRANCH=OISF/suricata-verify#1464
While this introduces 2 new keywords, I'm also trying to create a good example
of a sticky buffer keyword, and then migrate that into the template, so please
consider the full structure of the files implementing the sticky buffers.
I probably should have left out the "cleanups", can do in a subsequent PR.