From dbaf63df5a8f32b946888658bcc91bab0695a9ab 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
(cherry picked from commit c98c49d4bad413dbbe4e21a48ebf37260ee5cc8e)
---
 rules/dns-events.rules |  1 +
 rust/src/dns/dns.rs    | 27 ++++++++++++++++++---------
 rust/src/dns/log.rs    |  8 ++++++--
 3 files changed, 25 insertions(+), 11 deletions(-)

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 ee00986588..9403a26620 100644
--- a/rust/src/dns/dns.rs
+++ b/rust/src/dns/dns.rs
@@ -133,10 +133,11 @@ static mut ALPROTO_DNS: AppProto = ALPROTO_UNKNOWN;
 
 #[repr(u32)]
 pub enum DNSEvent {
-    MalformedData = 0,
-    NotRequest = 1,
-    NotResponse = 2,
-    ZFlagSet = 3,
+    MalformedData,
+    NotRequest,
+    NotResponse,
+    ZFlagSet,
+    InvalidOpcode,
 }
 
 impl DNSEvent {
@@ -146,6 +147,7 @@ impl DNSEvent {
             DNSEvent::NotRequest => "NOT_A_REQUEST\0",
             DNSEvent::NotResponse => "NOT_A_RESPONSE\0",
             DNSEvent::ZFlagSet => "Z_FLAG_SET\0",
+            DNSEvent::InvalidOpcode => "INVALID_OPCODE\0",
         }
     }
 
@@ -155,6 +157,7 @@ impl DNSEvent {
             1 => Some(DNSEvent::NotRequest),
             2 => Some(DNSEvent::NotResponse),
             4 => Some(DNSEvent::ZFlagSet),
+            5 => Some(DNSEvent::InvalidOpcode),
             _ => None,
         }
     }
@@ -165,6 +168,7 @@ impl DNSEvent {
             "not_a_request" => Some(DNSEvent::NotRequest),
             "not_a_response" => Some(DNSEvent::NotRequest),
             "z_flag_set" => Some(DNSEvent::ZFlagSet),
+            "invalid_opcode" => Some(DNSEvent::InvalidOpcode),
             _ => None
         }
     }
@@ -506,6 +510,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);
@@ -517,6 +522,10 @@ impl DNSState {
                     self.set_event(DNSEvent::ZFlagSet);
                 }
 
+                if opcode >= 7 {
+                    self.set_event(DNSEvent::InvalidOpcode);
+                }
+
                 return true;
             }
             Err(nom::Err::Incomplete(_)) => {
@@ -546,6 +555,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 {
@@ -562,6 +572,10 @@ impl DNSState {
                     self.set_event(DNSEvent::ZFlagSet);
                 }
 
+                if opcode >= 7 {
+                    self.set_event(DNSEvent::InvalidOpcode);
+                }
+
                 return true;
             }
             Err(nom::Err::Incomplete(_)) => {
@@ -692,11 +706,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 bba983873e..27abab245a 100644
--- a/rust/src/dns/log.rs
+++ b/rust/src/dns/log.rs
@@ -488,10 +488,12 @@ fn dns_log_json_answer(js: &mut JsonBuilder, response: &DNSResponse, flags: u64)
         js.set_bool("z", true)?;
     }
 
-    for query in &response.queries {
+    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))?;
-        break;
     }
     js.set_string("rcode", &dns_rcode_string(header.flags))?;
 
@@ -602,6 +604,8 @@ fn dns_log_query(tx: &mut DNSTransaction,
                 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);
             }
         }
-- 
2.47.2