From 6291e220e48885f4bbaaee4238642131ef9db410 Mon Sep 17 00:00:00 2001
From: Philippe Antoine <pantoine@oisf.net>
Date: Tue, 10 Sep 2024 15:31:00 +0200
Subject: [PATCH] dns: improved handling of corrupt additionals

Ticket: 7228

That means log the rest of queries and answers, even if the
final field additionals is corrupt.
Set an event in this case.
---
 rules/dns-events.rules |  3 +++
 rust/src/dns/dns.rs    | 18 +++++++++++++++++-
 rust/src/dns/parser.rs | 29 ++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/rules/dns-events.rules b/rules/dns-events.rules
index a3cf982c1e..815d941982 100644
--- a/rules/dns-events.rules
+++ b/rules/dns-events.rules
@@ -17,3 +17,6 @@ alert dns any any -> any any (msg:"SURICATA DNS Infinite loop"; app-layer-event:
 
 # Suricata's maximum number of DNS name labels was reached while parsing a resource name.
 alert dns any any -> any any (msg:"SURICATA DNS Too many labels"; app-layer-event:dns.too_many_labels; classtype:protocol-command-decode; sid:224010; rev:1;)
+
+alert dns any any -> any any (msg:"SURICATA DNS invalid additionals"; app-layer-event:dns.invalid_additionals; classtype:protocol-command-decode; sid:2240011; rev:1;)
+alert dns any any -> any any (msg:"SURICATA DNS invalid authorities"; app-layer-event:dns.invalid_authorities; classtype:protocol-command-decode; sid:2240012; rev:1;)
diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs
index 1dad2a94d9..b96956ba0a 100644
--- a/rust/src/dns/dns.rs
+++ b/rust/src/dns/dns.rs
@@ -135,6 +135,8 @@ pub enum DNSEvent {
     InfiniteLoop,
     /// Too many labels were found.
     TooManyLabels,
+    InvalidAdditionals,
+    InvalidAuthorities,
 }
 
 #[derive(Debug, PartialEq, Eq)]
@@ -256,7 +258,9 @@ pub struct DNSMessage {
     pub queries: Vec<DNSQueryEntry>,
     pub answers: Vec<DNSAnswerEntry>,
     pub authorities: Vec<DNSAnswerEntry>,
+    pub invalid_authorities: bool,
     pub additionals: Vec<DNSAnswerEntry>,
+    pub invalid_additionals: bool,
 }
 
 #[derive(Debug, Default)]
@@ -399,6 +403,12 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result<DNSTransaction, DNSParse
             let opcode = ((request.header.flags >> 11) & 0xf) as u8;
 
             let mut tx = DNSTransaction::new(Direction::ToServer);
+            if request.invalid_additionals {
+                tx.set_event(DNSEvent::InvalidAdditionals);
+            }
+            if request.invalid_authorities {
+                tx.set_event(DNSEvent::InvalidAuthorities);
+            }
             tx.request = Some(request);
 
             if z_flag {
@@ -452,6 +462,12 @@ pub(crate) fn dns_parse_response(input: &[u8]) -> Result<DNSTransaction, DNSPars
             let flags = response.header.flags;
 
             let mut tx = DNSTransaction::new(Direction::ToClient);
+            if response.invalid_additionals {
+                tx.set_event(DNSEvent::InvalidAdditionals);
+            }
+            if response.invalid_authorities {
+                tx.set_event(DNSEvent::InvalidAuthorities);
+            }
             tx.response = Some(response);
 
             if flags & 0x8000 == 0 {
@@ -1601,7 +1617,7 @@ mod tests {
     fn test_dns_event_from_id() {
         assert_eq!(DNSEvent::from_id(0), Some(DNSEvent::MalformedData));
         assert_eq!(DNSEvent::from_id(3), Some(DNSEvent::ZFlagSet));
-        assert_eq!(DNSEvent::from_id(9), None);
+        assert_eq!(DNSEvent::from_id(99), None);
     }
 
     #[test]
diff --git a/rust/src/dns/parser.rs b/rust/src/dns/parser.rs
index 13864749aa..d94daf9804 100644
--- a/rust/src/dns/parser.rs
+++ b/rust/src/dns/parser.rs
@@ -461,17 +461,40 @@ pub fn dns_parse_body<'a>(
         header.questions as usize,
     )(i)?;
     let (i, answers) = dns_parse_answer(i, message, header.answer_rr as usize, &mut flags)?;
-    let (i, authorities) = dns_parse_answer(i, message, header.authority_rr as usize, &mut flags)?;
-    let (i, additionals) = dns_parse_answer(i, message, header.additional_rr as usize, &mut flags)?;
+
+    let mut invalid_authorities = false;
+    let mut authorities = Vec::new();
+    let mut i_next = i;
+    let authorities_parsed = dns_parse_answer(i, message, header.authority_rr as usize, &mut flags);
+    if let Ok((i, authorities_ok)) = authorities_parsed {
+        authorities = authorities_ok;
+        i_next = i;
+    } else {
+        invalid_authorities = true;
+    }
+
+    let mut invalid_additionals = false;
+    let mut additionals = Vec::new();
+    if !invalid_authorities {
+        let additionals_parsed = dns_parse_answer(i_next, message, header.additional_rr as usize, &mut flags);
+        if let Ok((i, additionals_ok)) = additionals_parsed {
+                additionals = additionals_ok;
+                i_next = i;
+            } else {
+                invalid_additionals = true;
+            }
+    }
     Ok((
-        i,
+        i_next,
         (
             DNSMessage {
                 header,
                 queries,
                 answers,
                 authorities,
+                invalid_authorities,
                 additionals,
+                invalid_additionals,
             },
             flags,
         ),