Dns invalid additionals 7228 v4

rules/dns-events.rules

@@ -8,3 +8,5 @@ alert dns any any -> any any (msg:"SURICATA DNS Not a response"; flow:to_client;
 # 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;)
+alert dns any any -> any any (msg:"SURICATA DNS invalid additionals"; app-layer-event:dns.invalid_additionals; classtype:protocol-command-decode; sid:2240008; rev:1;)
+alert dns any any -> any any (msg:"SURICATA DNS invalid authorities"; app-layer-event:dns.invalid_authorities; classtype:protocol-command-decode; sid:2240009; rev:1;)

rust/src/dns/dns.rs

@@ -129,6 +129,8 @@ pub enum DNSEvent {
     NotResponse,
     ZFlagSet,
     InvalidOpcode,
+    InvalidAdditionals,
+    InvalidAuthorities,
 }
 
 #[derive(Debug, PartialEq, Eq)]
@@ -235,7 +237,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)]
@@ -343,9 +347,9 @@ impl State<DNSTransaction> for DNSState {
     }
 }
 
-fn dns_validate_header(input: &[u8]) -> Option<(&[u8], DNSHeader)> {
+fn dns_validate_header(input: &[u8], dir: Direction) -> Option<(&[u8], DNSHeader)> {
     if let Ok((body, header)) = parser::dns_parse_header(input) {
-        if probe_header_validity(&header, input.len()).0 {
+        if probe_header_validity(&header, input.len(), dir).0 {
             return Some((body, header));
         }
     }
@@ -361,7 +365,7 @@ pub(crate) enum DNSParseError {
 }
 
 pub(crate) fn dns_parse_request(input: &[u8]) -> Result<DNSTransaction, DNSParseError> {
-    let (body, header) = if let Some((body, header)) = dns_validate_header(input) {
+    let (body, header) = if let Some((body, header)) = dns_validate_header(input, Direction::ToServer) {
         (body, header)
     } else {
         return Err(DNSParseError::HeaderValidation);
@@ -378,6 +382,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 {
@@ -404,7 +414,7 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result<DNSTransaction, DNSParse
 }
 
 pub(crate) fn dns_parse_response(input: &[u8]) -> Result<DNSTransaction, DNSParseError> {
-    let (body, header) = if let Some((body, header)) = dns_validate_header(input) {
+    let (body, header) = if let Some((body, header)) = dns_validate_header(input, Direction::ToClient) {
         (body, header)
     } else {
         return Err(DNSParseError::HeaderValidation);
@@ -418,6 +428,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 {
@@ -581,7 +597,7 @@ impl DNSState {
     ) -> AppLayerResult {
         let input = stream_slice.as_slice();
         if self.gap {
-            let (is_dns, _, is_incomplete) = probe_tcp(input);
+            let (is_dns, _, is_incomplete) = probe_tcp(input, Direction::ToServer);
             if is_dns || is_incomplete {
                 self.gap = false;
             } else {
@@ -645,7 +661,7 @@ impl DNSState {
     ) -> AppLayerResult {
         let input = stream_slice.as_slice();
         if self.gap {
-            let (is_dns, _, is_incomplete) = probe_tcp(input);
+            let (is_dns, _, is_incomplete) = probe_tcp(input, Direction::ToClient);
             if is_dns || is_incomplete {
                 self.gap = false;
             } else {
@@ -718,7 +734,7 @@ impl DNSState {
 
 const DNS_HEADER_SIZE: usize = 12;
 
-fn probe_header_validity(header: &DNSHeader, rlen: usize) -> (bool, bool, bool) {
+fn probe_header_validity(header: &DNSHeader, rlen: usize, dir: Direction) -> (bool, bool, bool) {
     let min_msg_size = 2
         * (header.additional_rr as usize
             + header.answer_rr as usize
@@ -732,14 +748,28 @@ fn probe_header_validity(header: &DNSHeader, rlen: usize) -> (bool, bool, bool)
         return (false, false, false);
     }
 
+    if (header.additional_rr as usize
+        + header.answer_rr as usize
+        + header.authority_rr as usize
+        + header.questions as usize) == 0 && rlen > DNS_HEADER_SIZE {
+            // zero fields, data size should be just DNS_HEADER_SIZE
+            // happens when DNS server return s format error
+            return (false, false, false);
+        }
     let is_request = header.flags & 0x8000 == 0;
+    if dir == Direction::ToClient && is_request && rlen > 0x1000 {
+        // if we expect a DNS reply from server
+        // but the flag indicates a request and the record length is suspiciously big
+        // this does not seem to be DNS
+        return (false, false, false);
+    }
     return (true, is_request, false);
 }
 
 /// Probe input to see if it looks like DNS.
 ///
 /// Returns a tuple of booleans: (is_dns, is_request, incomplete)
-fn probe(input: &[u8], dlen: usize) -> (bool, bool, bool) {
+fn probe(input: &[u8], dlen: usize, dir: Direction) -> (bool, bool, bool) {
     // Trim input to dlen if larger.
     let input = if input.len() <= dlen {
         input
@@ -751,15 +781,15 @@ fn probe(input: &[u8], dlen: usize) -> (bool, bool, bool) {
     // parse a complete message, so perform header validation only.
     if input.len() < dlen {
         if let Ok((_, header)) = parser::dns_parse_header(input) {
-            return probe_header_validity(&header, dlen);
+            return probe_header_validity(&header, dlen, dir);
         } else {
             return (false, false, false);
         }
     }
 
     match parser::dns_parse_header(input) {
         Ok((body, header)) => match parser::dns_parse_body(body, input, header) {
-            Ok((_, request)) => probe_header_validity(&request.header, dlen),
+            Ok((_, request)) => probe_header_validity(&request.header, dlen, dir),
             Err(Err::Incomplete(_)) => (false, false, true),
             Err(_) => (false, false, false),
         },
@@ -768,10 +798,10 @@ fn probe(input: &[u8], dlen: usize) -> (bool, bool, bool) {
 }
 
 /// Probe TCP input to see if it looks like DNS.
-fn probe_tcp(input: &[u8]) -> (bool, bool, bool) {
+fn probe_tcp(input: &[u8], dir: Direction) -> (bool, bool, bool) {
     match be_u16(input) as IResult<&[u8], u16> {
         Ok((rem, dlen)) => {
-            return probe(rem, dlen as usize);
+            return probe(rem, dlen as usize, dir);
         }
         Err(Err::Incomplete(_)) => {
             return (false, false, true);
@@ -953,13 +983,13 @@ pub extern "C" fn SCDnsTxGetResponseFlags(tx: &mut DNSTransaction) -> u16 {
 }
 
 unsafe extern "C" fn probe_udp(
-    _flow: *const core::Flow, _dir: u8, input: *const u8, len: u32, rdir: *mut u8,
+    _flow: *const core::Flow, dir: u8, input: *const u8, len: u32, rdir: *mut u8,
 ) -> AppProto {
     if input.is_null() || len < std::mem::size_of::<DNSHeader>() as u32 {
         return core::ALPROTO_UNKNOWN;
     }
     let slice: &[u8] = std::slice::from_raw_parts(input as *mut u8, len as usize);
-    let (is_dns, is_request, _) = probe(slice, slice.len());
+    let (is_dns, is_request, _) = probe(slice, slice.len(), dir.into());
     if is_dns {
         let dir = if is_request {
             Direction::ToServer
@@ -980,7 +1010,7 @@ unsafe extern "C" fn c_probe_tcp(
     }
     let slice: &[u8] = std::slice::from_raw_parts(input as *mut u8, len as usize);
     //is_incomplete is checked by caller
-    let (is_dns, is_request, _) = probe_tcp(slice);
+    let (is_dns, is_request, _) = probe_tcp(slice, direction.into());
     if is_dns {
         let dir = if is_request {
             Direction::ToServer

rust/src/dns/parser.rs

@@ -361,16 +361,39 @@ pub fn dns_parse_body<'a>(
 ) -> IResult<&'a [u8], DNSMessage> {
     let (i, queries) = count(|b| dns_parse_query(b, message), header.questions as usize)(i)?;
     let (i, answers) = dns_parse_answer(i, message, header.answer_rr as usize)?;
-    let (i, authorities) = dns_parse_answer(i, message, header.authority_rr as usize)?;
-    let (i, additionals) = dns_parse_answer(i, message, header.additional_rr as usize)?;
+
+    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);
+    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);
+        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,
         },
     ))
 }