From: Jason Ish Date: Wed, 21 Dec 2022 01:30:29 +0000 (-0600) Subject: dns: validate header on every incoming message X-Git-Tag: suricata-6.0.13~26 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0b283ef4a6943229cafb7b1b93a36c3d54f94ced;p=thirdparty%2Fsuricata.git dns: validate header on every incoming message As UDP streams getting probed, a stream that does not appear to be DNS at first, may have a single packet that does look close enough to DNS to be picked up as DNS causing every subsequent packet to result in a parser error. To mitigate this, probe every incoming DNS message header for validity before continuing onto the body. If the header doesn't validate as DNS, just ignore the packet so no parse error is registered. (cherry picked from commit 595700ab7e9dc9d12d46cf4d6833a86840decdf9) --- diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index 9403a26620..f2ffc6d704 100644 --- a/rust/src/dns/dns.rs +++ b/rust/src/dns/dns.rs @@ -500,7 +500,17 @@ impl DNSState { event as u8); } - pub fn parse_request(&mut self, input: &[u8]) -> bool { + fn validate_header(&self, input: &[u8]) -> bool { + parser::dns_parse_header(input) + .map(|(_, header)| probe_header_validity(header, input.len()).0) + .unwrap_or(false) + } + + fn parse_request(&mut self, input: &[u8], is_tcp: bool) -> bool { + if !self.validate_header(input) { + return !is_tcp; + } + match parser::dns_parse_request(input) { Ok((_, request)) => { if request.header.flags & 0x8000 != 0 { @@ -543,7 +553,19 @@ impl DNSState { } } - pub fn parse_response(&mut self, input: &[u8]) -> bool { + fn parse_request_udp(&mut self, input: &[u8]) -> bool { + self.parse_request(input, false) + } + + fn parse_response_udp(&mut self, input: &[u8]) -> bool { + self.parse_response(input, false) + } + + pub fn parse_response(&mut self, input: &[u8], is_tcp: bool) -> bool { + if !self.validate_header(input) { + return !is_tcp; + } + match parser::dns_parse_response(input) { Ok((_, response)) => { @@ -620,8 +642,8 @@ impl DNSState { SCLogDebug!("[request] Have {} bytes, need {} to parse", cur_i.len(), size + 2); if size > 0 && cur_i.len() >= size + 2 { - let msg = &cur_i[0..(size + 2)]; - if self.parse_request(&msg[2..]) { + let msg = &cur_i[2..(size + 2)]; + if self.parse_request(msg, true) { cur_i = &cur_i[(size + 2)..]; consumed += size + 2; } else { @@ -667,8 +689,8 @@ impl DNSState { SCLogDebug!("[response] Have {} bytes, need {} to parse", cur_i.len(), size + 2); if size > 0 && cur_i.len() >= size + 2 { - let msg = &cur_i[0..(size + 2)]; - if self.parse_response(&msg[2..]) { + let msg = &cur_i[2..(size + 2)]; + if self.parse_response(msg, true) { cur_i = &cur_i[(size + 2)..]; consumed += size + 2; } else { @@ -706,16 +728,19 @@ impl DNSState { const DNS_HEADER_SIZE: usize = 12; fn probe_header_validity(header: DNSHeader, rlen: usize) -> (bool, bool, bool) { - if 2 * (header.additional_rr as usize - + header.answer_rr as usize - + header.authority_rr as usize - + header.questions as usize) - + DNS_HEADER_SIZE - > rlen - { - //not enough data for such a DNS record + let min_msg_size = 2 + * (header.additional_rr as usize + + header.answer_rr as usize + + header.authority_rr as usize + + header.questions as usize) + + DNS_HEADER_SIZE; + + if min_msg_size > rlen { + // Not enough data for records defined in the header, or + // impossibly large. return (false, false, false); } + let is_request = header.flags & 0x8000 == 0; return (true, is_request, false); } @@ -812,11 +837,8 @@ pub extern "C" fn rs_dns_parse_request(_flow: *const core::Flow, -> AppLayerResult { let state = cast_pointer!(state, DNSState); let buf = unsafe{std::slice::from_raw_parts(input, input_len as usize)}; - if state.parse_request(buf) { - AppLayerResult::ok() - } else { - AppLayerResult::err() - } + state.parse_request_udp(buf); + AppLayerResult::ok() } #[no_mangle] @@ -830,11 +852,8 @@ pub extern "C" fn rs_dns_parse_response(_flow: *const core::Flow, -> AppLayerResult { let state = cast_pointer!(state, DNSState); let buf = unsafe{std::slice::from_raw_parts(input, input_len as usize)}; - if state.parse_response(buf) { - AppLayerResult::ok() - } else { - AppLayerResult::err() - } + state.parse_response_udp(buf); + AppLayerResult::ok() } /// C binding parse a DNS request. Returns 1 on success, -1 on failure. @@ -1387,7 +1406,7 @@ mod tests { 0x80, ]; let mut state = DNSState::new(); - assert!(state.parse_response(buf)); + assert!(state.parse_response(buf, false)); } // Port of the C RustDNSUDPParserTest02 unit test. @@ -1407,7 +1426,7 @@ mod tests { 0x10,0x00,0x02,0xC0,0x85,0x00,0x00,0x29,0x05,0x00,0x00,0x00,0x00,0x00,0x00,0x00, ]; let mut state = DNSState::new(); - assert!(state.parse_response(buf)); + assert!(state.parse_response(buf, false)); } // Port of the C RustDNSUDPParserTest03 unit test. @@ -1427,7 +1446,7 @@ mod tests { 0x29,0x05,0x00,0x00,0x00,0x00,0x00,0x00,0x00 ]; let mut state = DNSState::new(); - assert!(state.parse_response(buf)); + assert!(state.parse_response(buf, false)); } // Port of the C RustDNSUDPParserTest04 unit test. @@ -1451,7 +1470,7 @@ mod tests { 0x6b,0x00,0x01,0x00,0x01,0x00,0x09,0x3a,0x80,0x00,0x04,0x0a,0x1e,0x1c,0x5f ]; let mut state = DNSState::new(); - assert!(state.parse_response(buf)); + assert!(state.parse_response(buf, false)); } // Port of the C RustDNSUDPParserTest05 unit test. @@ -1475,7 +1494,7 @@ mod tests { 0x6b,0x00,0x01,0x00,0x01,0x00,0x09,0x3a,0x80,0x00,0x04,0x0a,0x1e,0x1c,0x5f ]; let mut state = DNSState::new(); - assert!(!state.parse_response(buf)); + assert!(!state.parse_response(buf, false)); } // Port of the C RustDNSTCPParserTestMultiRecord unit test.