dns: add dns.rcode keyword v6 #10233

hadiqaalamdar · 2024-01-24T12:18:57Z

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: #10203

Describe changes:

made the suggested changes from the previous PR
fixed the commit history.
modified the code to implement DetectUintData<u8> instead of simple u8
SV tests are running successfully
Added documentation for rcode in doc/userguide/rules/dns-keywords.rst file.
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.

Feature OISF#6621 It matches the rcode field in DNS It's an unsigned integer match valid ranges = [0-23] Does not support prefilter Supports flow in client direction

codecov · 2024-01-24T12:46:23Z

Codecov Report

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

Comparison is base (3cb7112) 82.18% compared to head (a122126) 82.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10233      +/-   ##
==========================================
+ Coverage   82.18%   82.22%   +0.03%     
==========================================
  Files         977      978       +1     
  Lines      271894   272014     +120     
==========================================
+ Hits       223465   223657     +192     
+ Misses      48429    48357      -72

Flag	Coverage Δ
fuzzcorpus	`63.29% <26.92%> (+0.30%)`	⬆️
suricata-verify	`61.42% <82.69%> (-0.07%)`	⬇️
unittests	`62.81% <69.16%> (+<0.01%)`	⬆️

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

catenacyber · 2024-01-24T13:49:11Z

rust/src/detect/uint.rs

@@ -39,6 +39,7 @@ pub enum DetectUintMode {

 #[derive(Debug)]
 #[repr(C)]
+#[derive(PartialEq)]


You can merge lines like #[derive(Debug, PartialEq)]

catenacyber · 2024-01-24T13:50:07Z

rust/src/dns/detect.rs

+                arg1: 4,
+                arg2: 0,
+            },
+            (0b0000_0000_0000_0100 & 0xf) as u8,


Strange way to write 4

should I remove this or change it to this (0b0000_0000_0000_0100) as u8,? The opcode code had something similar so that's why I wrote it like this.

opcode did this because there was some bits shifting.
4u8 should be more readable

catenacyber · 2024-01-24T13:51:09Z

rust/src/dns/detect.rs

+            return 0;
+        }
+    } else {
+        // Not to server or to client??


You can assume this never happens (so else instead of else if flags & Direction::ToClient as u8 != 0)

catenacyber · 2024-01-24T13:51:47Z

rust/src/dns/detect.rs

+        return 0;
+    };
+
+    let rcode = (header_flags & 0xf) as u8;


@jasonish should we use tx.rcode() here ?

And @hadiqaalamdar how can this be 23 ? cf commit message saying valid ranges = [0-23]

@jasonish should we use tx.rcode() here ?

That won't get you the rcode for a request, even if its odd to do so. Could extend it to, or just leave this as is for now.

I'm sorry but I'm a bit confused. Please let me know if I should change something here?

@jasonish is the owner of DNS knowledge and he said

just leave this as is for now.

catenacyber · 2024-01-24T13:52:40Z

doc/userguide/rules/dns-keywords.rst

+
+Match on DNS requests where the **rcode** is not between 7 and 15:
+
+  dns.rcode:!7-15;


This is not yet the case, and out of the scope of this PR I think

catenacyber · 2024-01-24T13:55:01Z

src/detect-dns-rcode.c

+    sigmatch_table[DETECT_AL_DNS_RCODE].AppLayerTxMatch = DetectDnsRcodeMatch;
+
+    DetectAppLayerInspectEngineRegister(
+            "dns.rcode", ALPROTO_DNS, SIG_FLAG_TOSERVER, 0, DetectEngineInspectGenericList, NULL);


Why does the commit message say Supports flow in client direction ?

Please correct me if I'm wrong but I think that while creating SV tests for dns rcode, someone had suggested that rcode is only sent in the client direction. If my understanding is correct should I remove "dns.rcode", ALPROTO_DNS, SIG_FLAG_TOSERVER, 0, DetectEngineInspectGenericList, NULL);?

Please correct me if I'm wrong but I think that while creating SV tests for dns rcode, someone had suggested that rcode is only sent in the client direction. If my understanding is correct should I remove "dns.rcode", ALPROTO_DNS, SIG_FLAG_TOSERVER, 0, DetectEngineInspectGenericList, NULL);?

That someone was me.
But Jason Ish, owner of DNS knowledge, said this keyword should work both ways.
To, the code is correct, but the commit message is not

jufajardini · 2024-01-24T16:02:28Z

rust/src/dns/detect.rs

-/* Copyright (C) 2019 Open Information Security Foundation
+/* Copyright (C) 2024 Open Information Security Foundation


When updating copyright years for existing files, we keep the initial year, like so, in this case:
2019-2024

hadiqaalamdar · 2024-01-25T13:58:22Z

New PR: #10249

hadiqaalamdar requested review from jasonish, jufajardini and victorjulien as code owners January 24, 2024 12:18

hadiqaalamdar mentioned this pull request Jan 24, 2024

dns: add dns.rcode keyword v5 #10203

Closed

3 tasks

hadiqaalamdar marked this pull request as draft January 24, 2024 12:25

dns: add dns.rcode keyword

a122126

Feature OISF#6621 It matches the rcode field in DNS It's an unsigned integer match valid ranges = [0-23] Does not support prefilter Supports flow in client direction

hadiqaalamdar force-pushed the detect-dns-rcode-6621-v6 branch from 97cd65b to a122126 Compare January 24, 2024 12:34

hadiqaalamdar marked this pull request as ready for review January 24, 2024 13:35

catenacyber reviewed Jan 24, 2024

View reviewed changes

jufajardini reviewed Jan 24, 2024

View reviewed changes

hadiqaalamdar mentioned this pull request Jan 25, 2024

dns: add dns.rcode keyword v7 #10249

Closed

3 tasks

hadiqaalamdar closed this Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: add dns.rcode keyword v6 #10233

dns: add dns.rcode keyword v6 #10233

hadiqaalamdar commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024

catenacyber Jan 24, 2024

catenacyber Jan 24, 2024

hadiqaalamdar Jan 25, 2024

catenacyber Jan 25, 2024

catenacyber Jan 24, 2024

catenacyber Jan 24, 2024

catenacyber Jan 24, 2024

jasonish Jan 24, 2024

hadiqaalamdar Jan 25, 2024

catenacyber Jan 25, 2024

catenacyber Jan 24, 2024

catenacyber Jan 24, 2024

hadiqaalamdar Jan 25, 2024

catenacyber Jan 25, 2024

jufajardini Jan 24, 2024

hadiqaalamdar commented Jan 25, 2024


		Match on DNS requests where the rcode is not between 7 and 15:

		dns.rcode:!7-15;

		/* Copyright (C) 2019 Open Information Security Foundation
		/* Copyright (C) 2024 Open Information Security Foundation

dns: add dns.rcode keyword v6 #10233

dns: add dns.rcode keyword v6 #10233

Conversation

hadiqaalamdar commented Jan 24, 2024 • edited Loading

codecov bot commented Jan 24, 2024

Codecov Report

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

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

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

hadiqaalamdar commented Jan 25, 2024

hadiqaalamdar commented Jan 24, 2024 •

edited

Loading