dns: add dns.rcode keyword v3 #10126

hadiqaalamdar · 2024-01-05T10:41:12Z

Feature #6621

I have read the contributing guide lines at https://docs.suricata.io/en/latest/devguide/codebase/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6621

Previous PR: #10097

Describe changes:

made the suggested changes from previous PR
successfully ran the following tests:
from rust dir: make check
from rust dir: cargo test
from rust dir: cargo clippy --all-features
from rust dir: rustfmt src/dns/*
from suricata dir: ./scripts/clang-format.sh check-branch
from suricata dir: ./scripts/clang-format.sh branch
from suricata dir running ./src/suricata -u showed that one unit test had failed.
from suricata dir running ./src/suricata -u -U dns showed 7 tests passed.
Running sv tests gave the following error on console (will add more details for sv test in the SV Branch):

Finished release [optimized] target(s) in 0.16s
===> dns-rcode: Sub test #1: FAIL : expected 1 matches; got 0 for filter {'count': 1, 'match': {'alert.signature_id': 1, 'direction': 'to_client', 'app_proto': 'dns', 'dns.rcode': 3}}
===> dns-rcode: Sub test #2: FAIL : expected 1 matches; got 0 for filter {'count': 1, 'match': {'alert.signature_id': 1, 'direction': 'to_server', 'app_proto': 'dns', 'dns.rcode': 3}}
===> dns-rcode: Sub test #3: FAIL : expected 1 matches; got 0 for filter {'count': 1, 'match': {'alert.signature_id': 2, 'direction': 'to_server', 'app_proto': 'dns', 'dns.rcode': 3}}
===> dns-rcode: Sub test #4: FAIL : expected 1 matches; got 0 for filter {'count': 1, 'match': {'alert.signature_id': 3, 'direction': 'to_client', 'app_proto': 'dns', 'dns.rcode': 3}}
===> dns-rcode: Sub test #5: FAIL : expected 1 matches; got 0 for filter {'count': 1, 'match': {'alert.signature_id': 4, 'direction': 'to_client', 'app_proto': 'dns', 'dns.rcode': 3, 'dns.negate': True}}
/Users/hadiqa/suricata-verify/tests/dns/dns-rcode/output/eve.json - INVALID. Errors:
1./dns/answer Additional properties are not allowed ('authorities' was unexpected)
/Users/hadiqa/suricata-verify/tests/dns/dns-rcode/output/eve.json - INVALID. Errors:
1./dns/answer Additional properties are not allowed ('authorities' was unexpected)
===> dns-rcode: FAILED: Invalid JSON schema

PASSED:  0
FAILED:  1
SKIPPED: 0

SV_BRANCH=OISF/suricata-verify#1574

Feature OISF#6621

codecov · 2024-01-05T11:33:09Z

Codecov Report

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

Comparison is base (f12e026) 82.22% compared to head (943e976) 82.16%.
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10126      +/-   ##
==========================================
- Coverage   82.22%   82.16%   -0.07%     
==========================================
  Files         974      975       +1     
  Lines      271925   272069     +144     
==========================================
- Hits       223595   223539      -56     
- Misses      48330    48530     +200

Flag	Coverage Δ
fuzzcorpus	`62.94% <17.50%> (-0.07%)`	⬇️
suricata-verify	`61.40% <17.50%> (-0.03%)`	⬇️
unittests	`62.85% <67.36%> (+<0.01%)`	⬆️

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

victorjulien · 2024-01-05T13:27:56Z

src/detect-dns-rcode.c

+
+    if (SigMatchAppendSMToList(
+                de_ctx, s, DETECT_AL_DNS_RCODE, (SigMatchCtx *)detect, dns_rcode_list_id) == NULL) {
+        goto error;


minor: since we have only one error path after detect was allocated, we can call DetectDnsRcodeFree and return here, instead of using a goto.

victorjulien · 2024-01-05T13:28:39Z

src/detect-dns-rcode.c

+    SCEnter();
+
+    if (DetectSignatureSetAppProto(s, ALPROTO_DNS) != 0) {
+        return -1;


nit: SCReturnInt(-1);

currently use of the macro is inconsistent in this function

catenacyber · 2024-01-09T12:18:20Z

Looks like SV PR was not used by CI

catenacyber · 2024-01-09T12:19:53Z

@jasonish the SV test shows that we need https://redmine.openinfosecfoundation.org/issues/6281 to be fixed ;-)

catenacyber · 2024-01-09T12:22:33Z

rust/src/dns/detect.rs

@@ -26,6 +26,12 @@ pub struct DetectDnsOpcode {
    opcode: u8,
 }

+#[derive(Debug, PartialEq, Eq)]
+pub struct DetectDnsRcode {


We should reuse DetectUintData<u8> cf #10087 (comment)

It was suggested here #10087 (comment) that I can use simple u8 till the code is in working condition. I can change it to DetectUintData<u8> in the next PR I create.

catenacyber · 2024-01-09T12:34:03Z

src/detect-dns-rcode.c

+    sigmatch_table[DETECT_AL_DNS_RCODE].Match = NULL;
+    sigmatch_table[DETECT_AL_DNS_RCODE].AppLayerTxMatch = DetectDnsRcodeMatch;
+
+    DetectAppLayerInspectEngineRegister(


Should remove this line with SIG_FLAG_TOSERVER because dns.rcode is only to client

will do, thanks!

I'd keep it. While answers should only be seen in the to client direction as well, we do allow detection on them in the to server direction as they are allowed by the message format.

Likewise, the rcode field is also there in the to server direction, and at some point may be of interest to detect on in that direction.

Likewise, the rcode field is also there in the to server direction

Wireshark does not show it :-/

Not in the break down, no, but the bits are still there. I agree its somewhat non-sensical, but Suricata is here to find the non-sensical.

Note: The major DNS servers don't seem to care if an rcode is set on a request.

Makes sense.

By the way, the pcap in SV test seems to have an extended rcode cf dns.resp.ext_rcode == 0x00 Wireshark filter

jasonish · 2024-01-09T17:39:56Z

@jasonish the SV test shows that we need https://redmine.openinfosecfoundation.org/issues/6281 to be fixed ;-)

I don't think so. We should update the schema definition to match the real schema. Changing the output is a different task with possible breaking changes. Something simple like this should get us by for the scope of this PR?

diff --git a/etc/schema.json b/etc/schema.json
index 0756acd008..f6321a1179 100644
--- a/etc/schema.json
+++ b/etc/schema.json
@@ -1156,7 +1156,10 @@
                         "opcode": {
                             "description": "DNS opcode as an integer",
                             "type": "integer"
-                        }
+                        },
+                       "authorities": {
+                           "type": "array"
+                       }
                     },
                     "additionalProperties": false
                 },

jasonish

Good work, some changes requested, however it seems to function well, at least on the rules I wrote up to test it. However, the S-V tests need to be fixed up to match what is expected, I've left some comments over there as well.

hadiqaalamdar · 2024-01-11T12:12:39Z

New PR: #10150

dns: add dns.rcode keyword

943e976

Feature OISF#6621

hadiqaalamdar requested review from jasonish and victorjulien as code owners January 5, 2024 10:41

hadiqaalamdar marked this pull request as draft January 5, 2024 10:41

This was referenced Jan 5, 2024

test: add test for dns.rcode v3 OISF/suricata-verify#1574

Closed

dns: add dns.rcode keyword v2 #10097

Closed

victorjulien reviewed Jan 5, 2024

View reviewed changes

catenacyber reviewed Jan 9, 2024

View reviewed changes

jasonish requested changes Jan 9, 2024

View reviewed changes

This was referenced Jan 9, 2024

eve/schema: allow authorities in dns.answers in alert #10142

Closed

eve/schema: allow authorities in dns.answers in alert - v2 #10144

Closed

hadiqaalamdar mentioned this pull request Jan 11, 2024

dns: add dns.rcode keyword v4 #10150

Closed

3 tasks

hadiqaalamdar closed this Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: add dns.rcode keyword v3 #10126

dns: add dns.rcode keyword v3 #10126

hadiqaalamdar commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024

victorjulien Jan 5, 2024

victorjulien Jan 5, 2024

catenacyber commented Jan 9, 2024

catenacyber commented Jan 9, 2024

catenacyber Jan 9, 2024

hadiqaalamdar Jan 9, 2024

catenacyber Jan 9, 2024

hadiqaalamdar Jan 9, 2024 •

edited

Loading

jasonish Jan 9, 2024

catenacyber Jan 9, 2024

jasonish Jan 9, 2024

catenacyber Jan 10, 2024

jasonish commented Jan 9, 2024

jasonish left a comment •

edited

Loading

hadiqaalamdar commented Jan 11, 2024

dns: add dns.rcode keyword v3 #10126

dns: add dns.rcode keyword v3 #10126

Conversation

hadiqaalamdar commented Jan 5, 2024 • edited Loading

codecov bot commented Jan 5, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Jan 9, 2024

catenacyber commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadiqaalamdar Jan 9, 2024 • edited Loading

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

jasonish commented Jan 9, 2024

jasonish left a comment • edited Loading

Choose a reason for hiding this comment

hadiqaalamdar commented Jan 11, 2024

hadiqaalamdar commented Jan 5, 2024 •

edited

Loading

hadiqaalamdar Jan 9, 2024 •

edited

Loading

jasonish left a comment •

edited

Loading