From c98c49d4bad413dbbe4e21a48ebf37260ee5cc8e Mon Sep 17 00:00:00 2001
From: Jason Ish <jason.ish@oisf.net>
Date: Tue, 20 Dec 2022 19:17:38 -0600
Subject: [PATCH] dns: parse and alert on invalid opcodes

Accept DNS messages with an invalid opcode that are otherwise
valid. Such DNS message will create a parser event.

This is a change of behavior, previously an invalid opcode would cause
the DNS message to not be detected or parsed as DNS.

Issue: #5444
---
 etc/schema.json        | 12 ++++++++++++
 rules/dns-events.rules |  1 +
 rust/src/dns/dns.rs    | 16 +++++++++++-----
 rust/src/dns/log.rs    |  5 +++++
 4 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/etc/schema.json b/etc/schema.json
index 445f891da9..46a133f032 100644
--- a/etc/schema.json
+++ b/etc/schema.json
@@ -985,6 +985,10 @@
                 "version": {
                     "type": "integer"
                 },
+                "opcode": {
+                    "description": "DNS opcode as an integer",
+                    "type": "integer"
+                },
                 "answers": {
                     "type": "array",
                     "minItems": 1,
@@ -1102,6 +1106,10 @@
                             },
                             "z": {
                                 "type": "boolean"
+                            },
+                            "opcode": {
+                                "description": "DNS opcode as an integer",
+                                "type": "integer"
                             }
                         },
                         "additionalProperties": false
@@ -1139,6 +1147,10 @@
                         },
                         "version": {
                             "type": "integer"
+                        },
+                        "opcode": {
+                            "description": "DNS opcode as an integer",
+                            "type": "integer"
                         }
                     },
                     "additionalProperties": false
diff --git a/rules/dns-events.rules b/rules/dns-events.rules
index 0e34dae139..d4c02b5c2f 100644
--- a/rules/dns-events.rules
+++ b/rules/dns-events.rules
@@ -7,3 +7,4 @@ alert dns any any -> any any (msg:"SURICATA DNS Not a request"; flow:to_server;
 alert dns any any -> any any (msg:"SURICATA DNS Not a response"; flow:to_client; app-layer-event:dns.not_a_response; classtype:protocol-command-decode; sid:2240005; rev:2;)
 # Z flag (reserved) not 0
 alert dns any any -> any any (msg:"SURICATA DNS Z flag set"; app-layer-event:dns.z_flag_set; classtype:protocol-command-decode; sid:2240006; rev:2;)
+alert dns any any -> any any (msg:"SURICATA DNS Invalid opcode"; app-layer-event:dns.invalid_opcode; classtype:protocol-command-decode; sid:2240007; rev:1;)
diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs
index daeccade6b..1e032b0467 100644
--- a/rust/src/dns/dns.rs
+++ b/rust/src/dns/dns.rs
@@ -128,6 +128,7 @@ pub enum DNSEvent {
     NotRequest,
     NotResponse,
     ZFlagSet,
+    InvalidOpcode,
 }
 
 #[derive(Debug, PartialEq, Eq)]
@@ -392,6 +393,7 @@ impl DNSState {
                 }
 
                 let z_flag = request.header.flags & 0x0040 != 0;
+                let opcode = ((request.header.flags >> 11) & 0xf) as u8;
 
                 let mut tx = self.new_tx();
                 tx.request = Some(request);
@@ -402,6 +404,10 @@ impl DNSState {
                     self.set_event(DNSEvent::ZFlagSet);
                 }
 
+                if opcode >= 7 {
+                    self.set_event(DNSEvent::InvalidOpcode);
+                }
+
                 return true;
             }
             Err(Err::Incomplete(_)) => {
@@ -454,6 +460,7 @@ impl DNSState {
                 }
 
                 let z_flag = response.header.flags & 0x0040 != 0;
+                let opcode = ((response.header.flags >> 11) & 0xf) as u8;
 
                 let mut tx = self.new_tx();
                 if let Some(ref mut config) = &mut self.config {
@@ -469,6 +476,10 @@ impl DNSState {
                     self.set_event(DNSEvent::ZFlagSet);
                 }
 
+                if opcode >= 7 {
+                    self.set_event(DNSEvent::InvalidOpcode);
+                }
+
                 return true;
             }
             Err(Err::Incomplete(_)) => {
@@ -629,11 +640,6 @@ impl DNSState {
 const DNS_HEADER_SIZE: usize = 12;
 
 fn probe_header_validity(header: DNSHeader, rlen: usize) -> (bool, bool, bool) {
-    let opcode = ((header.flags >> 11) & 0xf) as u8;
-    if opcode >= 7 {
-        //unassigned opcode
-        return (false, false, false);
-    }
     if 2 * (header.additional_rr as usize
         + header.answer_rr as usize
         + header.authority_rr as usize
diff --git a/rust/src/dns/log.rs b/rust/src/dns/log.rs
index f0a3e7f620..2fe3e7c37d 100644
--- a/rust/src/dns/log.rs
+++ b/rust/src/dns/log.rs
@@ -506,6 +506,9 @@ fn dns_log_json_answer(
         js.set_bool("z", true)?;
     }
 
+    let opcode = ((header.flags >> 11) & 0xf) as u8;
+    js.set_uint("opcode", opcode as u64)?;
+
     if let Some(query) = response.queries.first() {
         js.set_string_from_bytes("rrname", &query.name)?;
         js.set_string("rrtype", &dns_rrtype_string(query.rrtype))?;
@@ -620,6 +623,8 @@ fn dns_log_query(
                 if request.header.flags & 0x0040 != 0 {
                     jb.set_bool("z", true)?;
                 }
+                let opcode = ((request.header.flags >> 11) & 0xf) as u8;
+                jb.set_uint("opcode", opcode as u64)?;
                 return Ok(true);
             }
         }