dns: add dns.rcode keyword v1 #10087

hadiqaalamdar · 2023-12-21T16:16:29Z

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

Describe changes:

added the dns.rcode field

SV_BRANCH=OISF/suricata-verify#1563

Feature OISF#6621

jasonish · 2023-12-21T16:38:46Z

rust/src/dns/detect.rs

@@ -189,3 +263,43 @@ mod test {
        ));
    }
 }
+
+#[cfg(test)]
+mod test {


You can put your tests in the existing test block, in fact this will fail to compile. From the rust/ directory you can do cargo test or make check to run the tests before CI.

And you will not need tests if you reuse DetectUintData<u16>

jasonish · 2023-12-21T16:45:19Z

rust/src/dns/detect.rs

+    let rcode = ((flags >> 11) & 0xf) as u16;
+    detect.rcode == rcode


This is looking at the opcode, not the rcode. As the rcode is the last 4 bits we can drop the shift and just mask off the first 12 bits to get our rcode. See pub fn rcode in dns.rs for an example.

Is it 4 bites or a u16 cf https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-6 ?

Oh seeing the comment Support for OPT RR from RFC6891 will be needed to parse RCODE values over 15

would it be better if I used something like this instead:
let rcode = (flags & 0x000f) as DetectUintData<u16>;

jasonish · 2023-12-21T16:51:08Z

rust/src/dns/detect.rs

+            &DetectDnsRcode {
+                rcode: 4,
+            },
+            0b0010_0000_0000_0000,


rcode still be the last 4 bits. So an rcode of 4 will be 0b0000_0000_0000_0100.

jasonish

I recommend a few small steps first.

Fixup so the tests compile locally
Update tests with the rcode in the correct place in the flags
Fix the match
Locally run cargo clippy --all-features and fix any issues found by that

As a side note, all the DNS files are rustfmt clean (not true for all our rust code though), so you can run rustfmt on the DNS files or let your editor do it if you like. No need while in-progress, but will have you do it as it looks more ready to merge.

Thanks!

catenacyber · 2023-12-21T20:50:47Z

rust/src/dns/detect.rs

@@ -25,6 +25,9 @@ pub struct DetectDnsOpcode {
    negate: bool,
    opcode: u8,
 }
+pub struct DetectDnsRcode {
+    rcode: u16,


You should reuse DetectUintData<u16>

catenacyber · 2023-12-21T20:51:35Z

rust/src/dns/detect.rs

@@ -53,6 +56,23 @@ fn parse_opcode(opcode: &str) -> Result<DetectDnsOpcode, ()> {
    Err(())
 }

+fn parse_rcode(rcode: &str) -> Result<DetectDnsRcode, ()> {


Reusing DetectUintData<u16> supplies you the parsing function rs_detect_u16_parse

(and this allows more features, like negation, lesser or greater than, etc...)

I was wondering if rcode can be negated? Should I add functions and tests to check that as well similar to opcode?

I think it makes sense to have negation on the rcode. I might want to alert on all non-zero rcodes without listing them all for example.

catenacyber · 2023-12-21T20:52:15Z

src/detect-dns-rcode.c

@@ -0,0 +1,86 @@
+/* Copyright (C) 2019 Open Information Security Foundation


nit year is 2023

catenacyber · 2023-12-21T20:52:47Z

src/detect-dns-rcode.c

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


do not need a label if only used once

catenacyber · 2023-12-21T20:53:49Z

src/detect-dns-rcode.c

+static int DetectDnsRcodeMatch(DetectEngineThreadCtx *det_ctx, Flow *f, uint8_t flags, void *state,
+        void *txv, const Signature *s, const SigMatchCtx *ctx)
+{
+    return rs_dns_rcode_match(txv, (void *)ctx, flags);


Get the u16 value from the dns tx with a call to a new SCDnsTxGetRcode and then match it DetectU16Match

catenacyber · 2023-12-21T20:54:32Z

src/detect-dns-rcode.c

+void DetectDnsRcodeRegister(void)
+{
+    sigmatch_table[DETECT_AL_DNS_RCODE].name = "dns.rcode";
+    sigmatch_table[DETECT_AL_DNS_RCODE].desc = "Match the DNS header rcode flag.";


Missing a doc entry, and by the way documentation in doc/userguide/rules/dns-keywords.rst

And related: the doc addition itself :P

catenacyber · 2023-12-21T20:58:31Z

Looking good overall.
Main change is to reuse existing DetectU16Data (and this have less new code)

catenacyber · 2023-12-21T20:58:46Z

Is code really u16, or is it u8 ?

hadiqaalamdar · 2023-12-22T15:17:25Z

Is code really u16, or is it u8 ?

according to this it's supposed to be u16: https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-6

jasonish · 2023-12-22T16:12:27Z

Is code really u16, or is it u8 ?

according to this it's supposed to be u16: https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-6

We can safely use u8 here as we only deal with 4 bit rcodes currently. Note to self: do we have to worry about EDNS with extension options for more rcodes? I don't think this has ever come up.

hadiqaalamdar · 2023-12-22T16:17:52Z

Is code really u16, or is it u8 ?

according to this it's supposed to be u16: https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-6

We can safely use u8 here as we only deal with 4 bit rcodes currently. Note to self: do we have to worry about EDNS with extension options for more rcodes? I don't think this has ever come up.

so should I use the DetectUintData<u8> here or simply u8 as done for opcode?

jasonish · 2023-12-22T16:28:45Z

so should I use the DetectUintData<u8> here or simply u8 as done for opcode?

I think the end-goal would be to get to DetectUintData<u8>, but if you want to first use u8 to be as close as possible to opcode in the short term to get CI greener sooner for a small win, that is fine as well. Then migrate to DetectUintData<u8>. This PR would be useful as a template as it converts opcode, but its not in master yet.

jufajardini · 2023-12-22T16:47:32Z

Edited the PR description to remove space before SV PR link, to ensure that our CI would pick the right branch for the tests (cf https://github.com/OISF/suricata/actions/runs/7290609564/job/19867785013#step:9:38)

jufajardini · 2023-12-22T16:53:24Z

src/detect-dns-rcode.c

+ */
+
+#include "suricata-common.h"


You can also add your info as author, if you want :)

Ooo that would be cool to see when my branch gets merged. Right now my code is all sorts of messed up lol. I'm relearning bitwise operations from back when I studied them at uni

💪🏽
Happens a lot, for these things that we don't use much, after learning.

jufajardini

Hey there, good progress!

In addition to the existing comments, when you're working on the next version of the work, do make sure to check if the errors shown in https://github.com/OISF/suricata/actions/runs/7290609583/job/19867784386?pr=10087#step:12:126 have been addressed :)

catenacyber · 2023-12-22T21:59:18Z

We can safely use u8 here as we only deal with 4 bit rcodes currently. Note to self: do we have to worry about EDNS with extension options for more rcodes? I don't think this has ever come up.

To keep track, Is there a ticket to handle more rcodes Jason ?

jasonish · 2023-12-22T22:38:52Z

We can safely use u8 here as we only deal with 4 bit rcodes currently. Note to self: do we have to worry about EDNS with extension options for more rcodes? I don't think this has ever come up.

To keep track, Is there a ticket to handle more rcodes Jason ?

I guess that would be EDNS, not specifically rcodes. Is it even a thing that saw use?

jasonish · 2023-12-22T22:49:51Z

We can safely use u8 here as we only deal with 4 bit rcodes currently. Note to self: do we have to worry about EDNS with extension options for more rcodes? I don't think this has ever come up.

To keep track, Is there a ticket to handle more rcodes Jason ?

I guess that would be EDNS, not specifically rcodes. Is it even a thing that saw use?

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

@hadiqaalamdar Please don't concern yourself with these comments. They are way out of scope for the actual work required in this PR.

hadiqaalamdar · 2023-12-26T10:39:56Z

New PR: #10097

dns: add dns.rcode keyword

8e0535d

Feature OISF#6621

hadiqaalamdar requested review from jasonish and victorjulien as code owners December 21, 2023 16:16

hadiqaalamdar mentioned this pull request Dec 21, 2023

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

Closed

jasonish reviewed Dec 21, 2023

View reviewed changes

jasonish requested changes Dec 21, 2023

View reviewed changes

catenacyber reviewed Dec 21, 2023

View reviewed changes

jufajardini reviewed Dec 22, 2023

View reviewed changes

jufajardini requested changes Dec 22, 2023

View reviewed changes

hadiqaalamdar closed this Dec 26, 2023

hadiqaalamdar mentioned this pull request Dec 26, 2023

dns: add dns.rcode keyword v2 #10097

Closed

3 tasks

hadiqaalamdar mentioned this pull request Jan 5, 2024

dns: add dns.rcode keyword v3 #10126

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: add dns.rcode keyword v1 #10087

dns: add dns.rcode keyword v1 #10087

hadiqaalamdar commented Dec 21, 2023 •

edited by jufajardini

Loading

jasonish Dec 21, 2023

catenacyber Dec 21, 2023

jasonish Dec 21, 2023 •

edited

Loading

catenacyber Dec 22, 2023

catenacyber Dec 22, 2023

hadiqaalamdar Dec 22, 2023

jasonish Dec 21, 2023

jasonish left a comment

catenacyber Dec 21, 2023

catenacyber Dec 21, 2023

catenacyber Dec 21, 2023

hadiqaalamdar Dec 22, 2023

jasonish Dec 22, 2023

catenacyber Dec 21, 2023

catenacyber Dec 21, 2023

catenacyber Dec 21, 2023

catenacyber Dec 21, 2023

jufajardini Dec 22, 2023

catenacyber commented Dec 21, 2023

catenacyber commented Dec 21, 2023

hadiqaalamdar commented Dec 22, 2023

jasonish commented Dec 22, 2023

hadiqaalamdar commented Dec 22, 2023 •

edited

Loading

jasonish commented Dec 22, 2023

jufajardini commented Dec 22, 2023

jufajardini Dec 22, 2023

hadiqaalamdar Dec 22, 2023

jufajardini Dec 22, 2023

jufajardini left a comment

catenacyber commented Dec 22, 2023

jasonish commented Dec 22, 2023

jasonish commented Dec 22, 2023

hadiqaalamdar commented Dec 26, 2023

		let rcode = ((flags >> 11) & 0xf) as u16;
		detect.rcode == rcode

		@@ -0,0 +1,86 @@
		/* Copyright (C) 2019 Open Information Security Foundation

		*/

		#include "suricata-common.h"

dns: add dns.rcode keyword v1 #10087

dns: add dns.rcode keyword v1 #10087

Conversation

hadiqaalamdar commented Dec 21, 2023 • edited by jufajardini Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonish Dec 21, 2023 • 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 left a 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

catenacyber commented Dec 21, 2023

catenacyber commented Dec 21, 2023

hadiqaalamdar commented Dec 22, 2023

jasonish commented Dec 22, 2023

hadiqaalamdar commented Dec 22, 2023 • edited Loading

jasonish commented Dec 22, 2023

jufajardini commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufajardini left a comment

Choose a reason for hiding this comment

catenacyber commented Dec 22, 2023

jasonish commented Dec 22, 2023

jasonish commented Dec 22, 2023

hadiqaalamdar commented Dec 26, 2023

hadiqaalamdar commented Dec 21, 2023 •

edited by jufajardini

Loading

jasonish Dec 21, 2023 •

edited

Loading

hadiqaalamdar commented Dec 22, 2023 •

edited

Loading