dns: more keywords; plus some eve/keyword parity tooling - v6 #12652

jasonish · 2025-02-21T15:21:13Z

This PR builds on #12500 and also adds the following keywords:

dns.additionals.rrname
dns.authorities.rrname
It builds on dns: add keyword for dns.response.rrname (feat 7012) - v2 #12500 and it uses functions introduced as part of the work for dns.response.rrname.

To provide consistency and better eve parity the following keywords were renamed:

dns.query.rrname -> dns.queries.rrname
dns.answer.rrname -> dns.answers.rrname

And finally, as using DNS as a test protocol for parity, try some tooling for eve parity.

For example, ./scripts/eve-parity.py mapped-keywords:

dns.rcode -> [dns.rcode]
dns.answer.additionals.rdata -> [dns.response.rrname]
dns.answer.additionals.rrname -> [dns.additionals.rrname, dns.response.rrname]
dns.answer.authorities.rdata -> [dns.response.rrname]
dns.answer.authorities.rrname -> [dns.authorities.rrname, dns.response.rrname]
dns.queries.rrname -> [dns.queries.rrname, dns.query]
dns.queries.rrtype -> [dns.rrtype]
dns.queries.opcode -> [dns.opcode]
dns.additionals.rdata -> [dns.response.rrname]
dns.additionals.rrname -> [dns.additionals.rrname, dns.response.rrname]
dns.authorities.rdata -> [dns.response.rrname]
dns.authorities.rrname -> [dns.authorities.rrname, dns.response.rrname]
dns.answers.rdata -> [dns.response.rrname]
dns.answers.rrname -> [dns.answers.rrname, dns.response.rrname]

and ./scripts/eve-parity.py unmapped-fields | grep dns:

dhcp.dns_servers
dns.aa
dns.additionals.opt.code
dns.additionals.opt.data
dns.additionals.rrtype
dns.additionals.ttl
dns.answer.additionals.opt.code
dns.answer.additionals.opt.data
dns.answer.additionals.rrtype
dns.answer.additionals.ttl
dns.answer.authorities.rdata_truncated
dns.answer.authorities.rrname_truncated
dns.answer.authorities.rrtype
dns.answer.authorities.soa.expire

and new in this version: unmapped-keywords to show known keywords that are
not mapped in the eve.json.

Other changes:

remove unit tests, they should have SV coverage now

SV_BRANCH=OISF/suricata-verify#2311

jasonish · 2025-02-21T15:28:04Z

scripts/eve-parity.py

+def load_known_keywords():
+    keywords = set()
+    result = subprocess.check_output(["./src/suricata", "--list-keywords=csv"])
+    lines = result.decode().split("\n")
+    # Skip first line, as its a header line.
+    for line in lines[1:]:
+        parts = line.split(";")
+        if parts:
+            keywords.add(parts[0])
+    return keywords


Should skip fields that are transform in the 4th column.

codecov · 2025-02-21T16:29:59Z

Codecov Report

Attention: Patch coverage is 91.39466% with 29 lines in your changes missing coverage. Please review.

Project coverage is 80.77%. Comparing base (3bc2a14) to head (762e73e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12652      +/-   ##
==========================================
- Coverage   80.77%   80.77%   -0.01%     
==========================================
  Files         932      932              
  Lines      259517   259758     +241     
==========================================
+ Hits       209629   209820     +191     
- Misses      49888    49938      +50

Flag	Coverage Δ
fuzzcorpus	`56.90% <19.58%> (-0.09%)`	⬇️
livemode	`19.35% <19.58%> (-0.03%)`	⬇️
pcap	`43.87% <19.58%> (-0.29%)`	⬇️
suricata-verify	`63.54% <91.39%> (+0.02%)`	⬆️
unittests	`58.28% <19.58%> (-0.04%)`	⬇️

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

jasonish · 2025-02-21T18:19:05Z

rust/src/dns/dns.rs

+                if !name.value.is_empty() {
+                    *buf = name.value.as_ptr();
+                    *len = name.value.len() as u32;
+                    return true;


S-V coverage missing.

jasonish · 2025-02-21T18:19:28Z

rust/src/dns/dns.rs

+                if !name.value.is_empty() {
+                    *buf = name.value.as_ptr();
+                    *len = name.value.len() as u32;
+                    return true;


S-V coverage missing.

jasonish · 2025-02-21T18:19:40Z

rust/src/dns/dns.rs

+        DNSRData::CNAME(name)
+        | DNSRData::PTR(name)
+        | DNSRData::MX(name)


S-V coverage missing.

Feature: 7012 Add dns.response sticky buffer to match on dns response fields. Add rust functions to return dns response packet data. Unit tests verifying signature matching.

Feature: 7012

This is a better name as the keyword is looking at all rrname type fields in the response.

These arrays are manually formatted for readability.

Make the function safe by returning a reference to the DNSName object, the unsafe C wrapper can do the conversion to pointers.

Split DetectHelperKeywordRegister into 2 functions, one for acquiring a new keyword ID, and another to perform the registration. This makes it easier to do the traditional C keyword initialization with a dynamic ID.

Add keywords dns.additionals.rrname and dns.authorities.rrname. Along the way, consolidate dns.query.name and dns.answer.name into a single file and register them altogether since there is a lot of common code.

To some EVE fields and a "suricata" object that contains an array of keywords. These are the keywords that map directly to this field, or somehow cover this field. This is an attempt at tooling to help with EVE and keyword parity. Related to tickets: OISF#5642, OISF#6463, OISF#4772

Currently this script has two commands: "missing" and "having". "missing" will show eve fields that do not map to any keywords. "having" will sohw eve fields along with their keyword mappsings, while also validating that those keywords really exist. Related to tickets: OISF#6463, OISF#4772

Should have coverage by S-V now.

suricata-qa · 2025-02-22T00:05:02Z

Information: QA ran without warnings.

Pipeline 24853

suricata-qa · 2025-02-22T02:25:02Z

Information: QA ran without warnings.

Pipeline 24856

catenacyber · 2025-02-23T20:05:18Z

etc/schema.json

-                    "type": "integer"
+                    "type": "integer",
+                    "suricata": {
+                        "keywords": false


Is it good to have this suricata.keyword that can either be a boolean or an array ?

I'm ok with it, unless its going into something the hardcodes a schema based on whats first seen., so not in eve. Prevents conflict with another field as well..

"keywords": ["one", "two"], "no-keywords": "true",

catenacyber · 2025-02-23T20:10:48Z

src/detect-dns-response.c

+    SCFree(ptr);
+}
+
+static int DetectDnsResponsePrefilterMpmRegister(DetectEngineCtx *de_ctx, SigGroupHead *sgh,


Why do we need a specific stuff here ?

Do not we get prefilter for all sticky buffers from the generic engine ?

Do not we get prefilter for all sticky buffers from the generic engine ?

Are you asking? If so, I'm not sure.

catenacyber · 2025-02-23T20:13:14Z

src/detect-dns-response.c

+            InspectionBuffer *buffer =
+                    GetBuffer(det_ctx, flags, transforms, txv, &cbdata, engine->sm_list, false);
+            if (buffer == NULL || buffer->inspect == NULL) {
+                local_id++;


Do we need local_id++ if we break right after ?

jasonish requested review from jufajardini, victorjulien and a team as code owners February 21, 2025 15:21

jasonish mentioned this pull request Feb 21, 2025

dns: more keywords; plus some eve/keyword parity tooling - v3 #12637

Closed

jasonish commented Feb 21, 2025

View reviewed changes

jasonish marked this pull request as draft February 21, 2025 21:31

scrivs86 and others added 13 commits February 21, 2025 16:11

dns: add dns.response sticky buffer

15dd20d

Feature: 7012 Add dns.response sticky buffer to match on dns response fields. Add rust functions to return dns response packet data. Unit tests verifying signature matching.

doc/userguide: document dns.response

f58267e

Feature: 7012

dns: rename dns.response keyword to dns.response.rrname

364057c

This is a better name as the keyword is looking at all rrname type fields in the response.

detect-dns-response: disable clang-format around byte arrays

5702306

These arrays are manually formatted for readability.

dns: refactor function to get rrname to be safe

78e9c45

Make the function safe by returning a reference to the DNSName object, the unsafe C wrapper can do the conversion to pointers.

detect: split new keyword id from registration

b58ed81

Split DetectHelperKeywordRegister into 2 functions, one for acquiring a new keyword ID, and another to perform the registration. This makes it easier to do the traditional C keyword initialization with a dynamic ID.

dns: add keywords for additionals and authorities rrnames

3fe80bd

Add keywords dns.additionals.rrname and dns.authorities.rrname. Along the way, consolidate dns.query.name and dns.answer.name into a single file and register them altogether since there is a lot of common code.

detect-dns-response: remove unit tests

9ba7a28

Should have coverage by S-V now.

schema: mark "stats" and "drop" as having no keywords

298262b

schema: mark dns.version and dns.grouped as having no keywords

41e2534

eve-parity: skip transform keywords

762e73e

jasonish force-pushed the ish/dns/feat-7012/v6 branch from b2b35ed to 762e73e Compare February 21, 2025 22:12

victorjulien mentioned this pull request Feb 22, 2025

dns: add keyword for dns.response.rrname (feat 7012) - v2 #12500

Closed

catenacyber reviewed Feb 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: more keywords; plus some eve/keyword parity tooling - v6 #12652

dns: more keywords; plus some eve/keyword parity tooling - v6 #12652

jasonish commented Feb 21, 2025

jasonish Feb 21, 2025

codecov bot commented Feb 21, 2025 •

edited

Loading

jasonish Feb 21, 2025

jasonish Feb 21, 2025

jasonish Feb 21, 2025

suricata-qa commented Feb 22, 2025

suricata-qa commented Feb 22, 2025

catenacyber Feb 23, 2025

jasonish Feb 23, 2025

catenacyber Feb 23, 2025

jasonish Feb 23, 2025

catenacyber Feb 23, 2025

dns: more keywords; plus some eve/keyword parity tooling - v6 #12652

Are you sure you want to change the base?

dns: more keywords; plus some eve/keyword parity tooling - v6 #12652

Conversation

jasonish commented Feb 21, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suricata-qa commented Feb 22, 2025

suricata-qa commented Feb 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025 •

edited

Loading