dns: new keywords: dns.answer.name, dns.query.name - v5 #9813

jasonish · 2023-11-16T18:11:47Z

Introduce two new DNS keywords, dns.query.name and dns.answer.name.

Tickets:

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.

Previous PR: #9795
Changes from last PR:

Some doc cleanups
Answer names are now inspected in requests (SV test updated)

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.

DNS request and response messages follow the same format so there is no reason not to use the same data structure for each. While its unlikely to see fields like answers in a request, the message format does not disallow them, so it might be interesting data to have the ability to log.

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

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.

With some other minor cleanups in the DNS keyword section.

jasonish · 2023-11-16T18:12:34Z

Open question: Should dns.answer.name and dns.query.name be dns.request.query.name, dns.response.query.name, etc? Or is specify a flow direction enough?

codecov · 2023-11-16T18:31:57Z

Codecov Report

Merging #9813 (7093d63) into master (2f4027c) will increase coverage by 0.00%.
The diff coverage is 97.68%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9813   +/-   ##
=======================================
  Coverage   82.37%   82.37%           
=======================================
  Files         968      970    +2     
  Lines      273866   273927   +61     
=======================================
+ Hits       225585   225640   +55     
- Misses      48281    48287    +6

Flag	Coverage Δ
fuzzcorpus	`64.14% <78.93%> (-0.08%)`	⬇️
suricata-verify	`60.99% <97.27%> (+0.01%)`	⬆️
unittests	`62.89% <78.61%> (-0.05%)`	⬇️

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

doc/userguide/rules/dns-keywords.rst

suricata-qa · 2023-11-16T20:55:03Z

Information: QA ran without warnings.

Pipeline 16565

src/output-json-dns.c

src/output-json-alert.c

catenacyber · 2023-11-23T15:22:43Z

Why did you put the doc in a separate commit ?

catenacyber · 2023-11-23T15:29:40Z

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.

See also #9871 especially commit detect: helper to have pure rust keywords

src/detect-dns-query-name.c

catenacyber · 2023-11-23T15:31:31Z

src/detect-dns-query-name.c

+    return DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
+}
+
+typedef struct PrefilterMpm {


This generic struct is already defined somewhere, is it not ?

In many detect, but no where for generic re-use that I can see? Might be good to make it more formal in your helpers?

Will you this as part of this PR ?
Otherwise, I can take care of it

I think its out of scope for this PR. Make sense in the helper work you are doing tho.

https://redmine.openinfosecfoundation.org/issues/6575

rust/rustfmt.toml

catenacyber · 2023-11-23T15:37:21Z

Open question: Should dns.answer.name and dns.query.name be dns.request.query.name, dns.response.query.name, etc? Or is specify a flow direction enough?

This is the same buffer, but different directions, right ? (we have http.request_header and http.response_header as something to compare with)

jasonish · 2023-11-23T15:42:11Z

Why did you put the doc in a separate commit ?

Its our standard practice, or at least once was to not mix code and docs commits.

jasonish · 2023-11-23T15:52:24Z

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.

See also #9871 especially commit detect: helper to have pure rust keywords

Yeah, I'm actually waiting on that before I re-work the templates.

jasonish · 2023-11-23T15:53:16Z

Open question: Should dns.answer.name and dns.query.name be dns.request.query.name, dns.response.query.name, etc? Or is specify a flow direction enough?

This is the same buffer, but different directions, right ? (we have http.request_header and http.response_header as something to compare with)

I guess I'd asky why? When flow can do the same thing without introducing almost duplicated code for each direction.

catenacyber · 2023-11-23T15:56:32Z

Why did you put the doc in a separate commit ?

Its our standard practice, or at least once was to not mix code and docs commits.

I did not know it and I squashed them together...

catenacyber · 2023-11-23T15:58:53Z

This is the same buffer, but different directions, right ? (we have http.request_header and http.response_header as something to compare with)

I guess I'd asky why? When flow can do the same thing without introducing almost duplicated code for each direction.

For HTTP headers, it makes sense for bidirectional rules, you will want to match certain headers in the request part, and some others in the response. Not sure if it would make sense for DNS queries...

jasonish · 2023-11-23T15:59:29Z

Why did you put the doc in a separate commit ?

Its our standard practice, or at least once was to not mix code and docs commits.

I did not know it and I squashed them together...

Here we go:

Point 5 at https://docs.suricata.io/en/latest/devguide/codebase/contributing/code-submission-process.html

jasonish · 2023-11-23T16:00:40Z

This is the same buffer, but different directions, right ? (we have http.request_header and http.response_header as something to compare with)

I guess I'd asky why? When flow can do the same thing without introducing almost duplicated code for each direction.

For HTTP headers, it makes sense for bidirectional rules, you will want to match certain headers in the request part, and some others in the response. Not sure if it would make sense for DNS queries...

I don't think it makes sense for DNS as it does for HTTP. DNS has one transaction for the request and one for the response. So you don't have the ability to look at the request and the response in one TX.

jasonish · 2023-11-29T22:15:37Z

Continued at #9920.

jasonish added 12 commits November 16, 2023 09:33

detect: rename DetectAppLayerMpmRegister2 to DetectAppLayerMpmRegister

0d120f9

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.

detect: rename DetectAppLayerInspectEngine2 to DetectAppLayerInspectE…

a65394d

…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.

detect: rename InspectEngineFuncPtr2 to InspectEngineFuncPtr

5780a9a

Version 1 of the API no longer exists.

rustfmt: replace deprecated fn_args_layout with fn_params_layout

b58744d

dns: rustfmt with latest stable

9ce80a7

dns: add dns.answer.name keyword

1e52f39

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

dns: add dns.query.name sticky buffer

614f633

This buffer is much like dns.query_name but allows for detection in both directions. Feature: OISF#6497

dns: replace usage of rs_dns_tx_get_query_name with SCDnsTxGetQueryName

f8f9b2a

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.

doc/userguide: document dns.query.name, dns.answer.name

7a8a10f

With some other minor cleanups in the DNS keyword section.

output-json-alert: remove un-needed includes

7d18b02

output-json-dns: remove un-needed includes

7093d63

jasonish requested review from jufajardini and victorjulien as code owners November 16, 2023 18:11

jasonish mentioned this pull request Nov 16, 2023

dns: new keywords: dns.answer.name, dns.query.name #9795

Closed

jasonish commented Nov 16, 2023

View reviewed changes

doc/userguide/rules/dns-keywords.rst Show resolved Hide resolved

victorjulien added this to the 8.0 milestone Nov 17, 2023

catenacyber mentioned this pull request Nov 23, 2023

[draft] dns: dns.response.answer.name sticky buffer - v1 #9686

Closed