From 9cac5a737e973877c89ad95a4aea3eb79cc0fce1 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 25 Apr 2023 10:22:11 +0200 Subject: [PATCH 1/6] stream: update no-flow checks (cherry picked from commit 0360cb654293c333e3be70204705fa7ec328512e) --- src/stream-tcp.c | 11 +++++------ src/stream-tcp.h | 2 -- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 2a921b31df1a..c17e42a07a87 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -5476,6 +5476,11 @@ int TcpSessionPacketSsnReuse(const Packet *p, const Flow *f, const void *tcp_ssn TmEcode StreamTcp (ThreadVars *tv, Packet *p, void *data, PacketQueueNoLock *pq) { + DEBUG_VALIDATE_BUG_ON(p->flow == NULL); + if (unlikely(p->flow == NULL)) { + return TM_ECODE_OK; + } + StreamTcpThread *stt = (StreamTcpThread *)data; SCLogDebug("p->pcap_cnt %"PRIu64, p->pcap_cnt); @@ -5487,11 +5492,6 @@ TmEcode StreamTcp (ThreadVars *tv, Packet *p, void *data, PacketQueueNoLock *pq) return TM_ECODE_OK; } - if (p->flow == NULL) { - StatsIncr(tv, stt->counter_tcp_no_flow); - return TM_ECODE_OK; - } - HandleThreadId(tv, p, stt); /* only TCP packets with a flow from here */ @@ -5531,7 +5531,6 @@ TmEcode StreamTcpThreadInit(ThreadVars *tv, void *initdata, void **data) stt->counter_tcp_pseudo = StatsRegisterCounter("tcp.pseudo", tv); stt->counter_tcp_pseudo_failed = StatsRegisterCounter("tcp.pseudo_failed", tv); stt->counter_tcp_invalid_checksum = StatsRegisterCounter("tcp.invalid_checksum", tv); - stt->counter_tcp_no_flow = StatsRegisterCounter("tcp.no_flow", tv); stt->counter_tcp_syn = StatsRegisterCounter("tcp.syn", tv); stt->counter_tcp_synack = StatsRegisterCounter("tcp.synack", tv); stt->counter_tcp_rst = StatsRegisterCounter("tcp.rst", tv); diff --git a/src/stream-tcp.h b/src/stream-tcp.h index 48b32a4fb6e3..6bea83bb060b 100644 --- a/src/stream-tcp.h +++ b/src/stream-tcp.h @@ -91,8 +91,6 @@ typedef struct StreamTcpThread_ { uint16_t counter_tcp_pseudo_failed; /** packets rejected because their csum is invalid */ uint16_t counter_tcp_invalid_checksum; - /** TCP packets with no associated flow */ - uint16_t counter_tcp_no_flow; /** sessions reused */ uint16_t counter_tcp_reused_ssn; /** syn pkts */ From cecea2ea56dbf8c6c71fc6caba12578546d71890 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 25 Apr 2023 10:09:27 +0200 Subject: [PATCH 2/6] counters: make tcp stats independent of flow, ssn Counters depended on availability of flow and tcp session, meaning that 2 memcaps could affect the counters. Bug: #5017. (cherry picked from commit 36f6e0515592812259fb327d529740a030dba98e) --- src/decode-tcp.c | 9 +++++++++ src/decode.c | 5 +++++ src/decode.h | 3 +++ src/stream-tcp.c | 13 ------------- src/stream-tcp.h | 6 ------ 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/decode-tcp.c b/src/decode-tcp.c index 84d7595cffdd..fee390fbb4a9 100644 --- a/src/decode-tcp.c +++ b/src/decode-tcp.c @@ -259,6 +259,15 @@ int DecodeTCP(ThreadVars *tv, DecodeThreadVars *dtv, Packet *p, return TM_ECODE_FAILED; } + /* update counters */ + if ((p->tcph->th_flags & (TH_SYN | TH_ACK)) == (TH_SYN | TH_ACK)) { + StatsIncr(tv, dtv->counter_tcp_synack); + } else if (p->tcph->th_flags & (TH_SYN)) { + StatsIncr(tv, dtv->counter_tcp_syn); + } + if (p->tcph->th_flags & (TH_RST)) { + StatsIncr(tv, dtv->counter_tcp_rst); + } #ifdef DEBUG SCLogDebug("TCP sp: %" PRIu32 " -> dp: %" PRIu32 " - HLEN: %" PRIu32 " LEN: %" PRIu32 " %s%s%s%s%s%s", GET_TCP_SRC_PORT(p), GET_TCP_DST_PORT(p), TCP_GET_HLEN(p), len, diff --git a/src/decode.c b/src/decode.c index 243dce422c07..574f91451a0f 100644 --- a/src/decode.c +++ b/src/decode.c @@ -537,6 +537,11 @@ void DecodeRegisterPerfCounters(DecodeThreadVars *dtv, ThreadVars *tv) dtv->counter_null = StatsRegisterCounter("decoder.null", tv); dtv->counter_sll = StatsRegisterCounter("decoder.sll", tv); dtv->counter_tcp = StatsRegisterCounter("decoder.tcp", tv); + + dtv->counter_tcp_syn = StatsRegisterCounter("tcp.syn", tv); + dtv->counter_tcp_synack = StatsRegisterCounter("tcp.synack", tv); + dtv->counter_tcp_rst = StatsRegisterCounter("tcp.rst", tv); + dtv->counter_udp = StatsRegisterCounter("decoder.udp", tv); dtv->counter_sctp = StatsRegisterCounter("decoder.sctp", tv); dtv->counter_icmpv4 = StatsRegisterCounter("decoder.icmpv4", tv); diff --git a/src/decode.h b/src/decode.h index ded72485787b..3c160c220ae7 100644 --- a/src/decode.h +++ b/src/decode.h @@ -677,6 +677,9 @@ typedef struct DecodeThreadVars_ uint16_t counter_ipv4; uint16_t counter_ipv6; uint16_t counter_tcp; + uint16_t counter_tcp_syn; + uint16_t counter_tcp_synack; + uint16_t counter_tcp_rst; uint16_t counter_udp; uint16_t counter_icmpv4; uint16_t counter_icmpv6; diff --git a/src/stream-tcp.c b/src/stream-tcp.c index c17e42a07a87..dc6d8f1e20cb 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -5085,16 +5085,6 @@ int StreamTcpPacket (ThreadVars *tv, Packet *p, StreamTcpThread *stt, } } - /* update counters */ - if ((p->tcph->th_flags & (TH_SYN|TH_ACK)) == (TH_SYN|TH_ACK)) { - StatsIncr(tv, stt->counter_tcp_synack); - } else if (p->tcph->th_flags & (TH_SYN)) { - StatsIncr(tv, stt->counter_tcp_syn); - } - if (p->tcph->th_flags & (TH_RST)) { - StatsIncr(tv, stt->counter_tcp_rst); - } - /* broken TCP http://ask.wireshark.org/questions/3183/acknowledgment-number-broken-tcp-the-acknowledge-field-is-nonzero-while-the-ack-flag-is-not-set */ if (!(p->tcph->th_flags & TH_ACK) && TCP_GET_ACK(p) != 0) { StreamTcpSetEvent(p, STREAM_PKT_BROKEN_ACK); @@ -5531,9 +5521,6 @@ TmEcode StreamTcpThreadInit(ThreadVars *tv, void *initdata, void **data) stt->counter_tcp_pseudo = StatsRegisterCounter("tcp.pseudo", tv); stt->counter_tcp_pseudo_failed = StatsRegisterCounter("tcp.pseudo_failed", tv); stt->counter_tcp_invalid_checksum = StatsRegisterCounter("tcp.invalid_checksum", tv); - stt->counter_tcp_syn = StatsRegisterCounter("tcp.syn", tv); - stt->counter_tcp_synack = StatsRegisterCounter("tcp.synack", tv); - stt->counter_tcp_rst = StatsRegisterCounter("tcp.rst", tv); stt->counter_tcp_midstream_pickups = StatsRegisterCounter("tcp.midstream_pickups", tv); stt->counter_tcp_wrong_thread = StatsRegisterCounter("tcp.pkt_on_wrong_thread", tv); diff --git a/src/stream-tcp.h b/src/stream-tcp.h index 6bea83bb060b..d071d95ec391 100644 --- a/src/stream-tcp.h +++ b/src/stream-tcp.h @@ -93,12 +93,6 @@ typedef struct StreamTcpThread_ { uint16_t counter_tcp_invalid_checksum; /** sessions reused */ uint16_t counter_tcp_reused_ssn; - /** syn pkts */ - uint16_t counter_tcp_syn; - /** syn/ack pkts */ - uint16_t counter_tcp_synack; - /** rst pkts */ - uint16_t counter_tcp_rst; /** midstream pickups */ uint16_t counter_tcp_midstream_pickups; /** wrong thread */ From 3ec1aafa252ecb0bc36830581c1ed12a3ffcf381 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 21 Dec 2022 09:01:15 -0600 Subject: [PATCH 3/6] dns: mark test buffers with rustfmt::skip (cherry picked from commit 39d2524bf6d57658b532c73ceb4def34ed9e2c8a) --- rust/src/dns/dns.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index ace6d0a39468..ee0098658861 100644 --- a/rust/src/dns/dns.rs +++ b/rust/src/dns/dns.rs @@ -1200,6 +1200,7 @@ mod tests { fn test_dns_parse_request_tcp_valid() { // A UDP DNS request with the DNS payload starting at byte 42. // From pcap: https://github.com/jasonish/suricata-verify/blob/7cc0e1bd0a5249b52e6e87d82d57c0b6aaf75fce/dns-udp-dig-a-www-suricata-ids-org/dig-a-www.suricata-ids.org.pcap + #[rustfmt::skip] let buf: &[u8] = &[ 0x00, 0x15, 0x17, 0x0d, 0x06, 0xf7, 0xd8, 0xcb, /* ........ */ 0x8a, 0xed, 0xa1, 0x46, 0x08, 0x00, 0x45, 0x00, /* ...F..E. */ @@ -1235,6 +1236,7 @@ mod tests { fn test_dns_parse_request_tcp_short_payload() { // A UDP DNS request with the DNS payload starting at byte 42. // From pcap: https://github.com/jasonish/suricata-verify/blob/7cc0e1bd0a5249b52e6e87d82d57c0b6aaf75fce/dns-udp-dig-a-www-suricata-ids-org/dig-a-www.suricata-ids.org.pcap + #[rustfmt::skip] let buf: &[u8] = &[ 0x00, 0x15, 0x17, 0x0d, 0x06, 0xf7, 0xd8, 0xcb, /* ........ */ 0x8a, 0xed, 0xa1, 0x46, 0x08, 0x00, 0x45, 0x00, /* ...F..E. */ @@ -1271,6 +1273,7 @@ mod tests { fn test_dns_parse_response_tcp_valid() { // A UDP DNS response with the DNS payload starting at byte 42. // From pcap: https://github.com/jasonish/suricata-verify/blob/7cc0e1bd0a5249b52e6e87d82d57c0b6aaf75fce/dns-udp-dig-a-www-suricata-ids-org/dig-a-www.suricata-ids.org.pcap + #[rustfmt::skip] let buf: &[u8] = &[ 0xd8, 0xcb, 0x8a, 0xed, 0xa1, 0x46, 0x00, 0x15, /* .....F.. */ 0x17, 0x0d, 0x06, 0xf7, 0x08, 0x00, 0x45, 0x00, /* ......E. */ @@ -1314,6 +1317,7 @@ mod tests { fn test_dns_parse_response_tcp_short_payload() { // A UDP DNS response with the DNS payload starting at byte 42. // From pcap: https://github.com/jasonish/suricata-verify/blob/7cc0e1bd0a5249b52e6e87d82d57c0b6aaf75fce/dns-udp-dig-a-www-suricata-ids-org/dig-a-www.suricata-ids.org.pcap + #[rustfmt::skip] let buf: &[u8] = &[ 0xd8, 0xcb, 0x8a, 0xed, 0xa1, 0x46, 0x00, 0x15, /* .....F.. */ 0x17, 0x0d, 0x06, 0xf7, 0x08, 0x00, 0x45, 0x00, /* ......E. */ @@ -1359,6 +1363,7 @@ mod tests { * TTL: 86400 * serial 20130422 refresh 28800 retry 7200 exp 604800 min ttl 86400 * ns, hostmaster */ + #[rustfmt::skip] let buf: &[u8] = &[ 0x00, 0x3c, 0x85, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x0b, 0x61, 0x62, 0x63, @@ -1379,6 +1384,7 @@ mod tests { // Port of the C RustDNSUDPParserTest02 unit test. #[test] fn test_dns_udp_parser_test_02() { + #[rustfmt::skip] let buf: &[u8] = &[ 0x6D,0x08,0x84,0x80,0x00,0x01,0x00,0x08,0x00,0x00,0x00,0x01,0x03,0x57,0x57,0x57, 0x04,0x54,0x54,0x54,0x54,0x03,0x56,0x56,0x56,0x03,0x63,0x6F,0x6D,0x02,0x79,0x79, @@ -1398,6 +1404,7 @@ mod tests { // Port of the C RustDNSUDPParserTest03 unit test. #[test] fn test_dns_udp_parser_test_03() { + #[rustfmt::skip] let buf: &[u8] = &[ 0x6F,0xB4,0x84,0x80,0x00,0x01,0x00,0x02,0x00,0x02,0x00,0x03,0x03,0x57,0x57,0x77, 0x0B,0x56,0x56,0x56,0x56,0x56,0x56,0x56,0x56,0x56,0x56,0x56,0x03,0x55,0x55,0x55, @@ -1419,6 +1426,7 @@ mod tests { // Test the TXT records in an answer. #[test] fn test_dns_udp_parser_test_04() { + #[rustfmt::skip] let buf: &[u8] = &[ 0xc2,0x2f,0x81,0x80,0x00,0x01,0x00,0x01,0x00,0x01,0x00,0x01,0x0a,0x41,0x41,0x41, 0x41,0x41,0x4f,0x31,0x6b,0x51,0x41,0x05,0x3d,0x61,0x75,0x74,0x68,0x03,0x73,0x72, @@ -1442,6 +1450,7 @@ mod tests { // Test TXT records in answer with a bad length. #[test] fn test_dns_udp_parser_test_05() { + #[rustfmt::skip] let buf: &[u8] = &[ 0xc2,0x2f,0x81,0x80,0x00,0x01,0x00,0x01,0x00,0x01,0x00,0x01,0x0a,0x41,0x41,0x41, 0x41,0x41,0x4f,0x31,0x6b,0x51,0x41,0x05,0x3d,0x61,0x75,0x74,0x68,0x03,0x73,0x72, @@ -1463,6 +1472,7 @@ mod tests { // Port of the C RustDNSTCPParserTestMultiRecord unit test. #[test] fn test_dns_tcp_parser_multi_record() { + #[rustfmt::skip] let buf: &[u8] = &[ 0x00, 0x1e, 0x00, 0x00, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x30, @@ -1557,11 +1567,13 @@ mod tests { #[test] fn test_dns_tcp_parser_split_payload() { /* incomplete payload */ + #[rustfmt::skip] let buf1: &[u8] = &[ 0x00, 0x1c, 0x10, 0x32, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 ]; /* complete payload plus the start of a new payload */ + #[rustfmt::skip] let buf2: &[u8] = &[ 0x00, 0x1c, 0x10, 0x32, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, @@ -1574,6 +1586,7 @@ mod tests { ]; /* and the complete payload again with no trailing data. */ + #[rustfmt::skip] let buf3: &[u8] = &[ 0x00, 0x1c, 0x10, 0x32, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, From 3566ec0d8eb643917e683246cf280e9b3d1d2ec9 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 20 Dec 2022 19:17:38 -0600 Subject: [PATCH 4/6] 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 0e34dae139d0..d4c02b5c2f78 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 ee0098658861..9403a26620dd 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 bba983873ed5..27abab245afc 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); } } From c2ec63e8dc47fbe690707828f0e898edf5631714 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Tue, 20 Dec 2022 19:30:29 -0600 Subject: [PATCH 5/6] 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) --- rust/src/applayer.rs | 43 ++++++++++++++++++++++++ rust/src/dns/dns.rs | 79 ++++++++++++++++++++++++++++---------------- 2 files changed, 93 insertions(+), 29 deletions(-) diff --git a/rust/src/applayer.rs b/rust/src/applayer.rs index 71d4e6e1b833..6c26ddf19f8b 100644 --- a/rust/src/applayer.rs +++ b/rust/src/applayer.rs @@ -23,6 +23,49 @@ use crate::filecontainer::FileContainer; use crate::applayer; use std::os::raw::{c_void,c_char,c_int}; +#[repr(C)] +pub struct StreamSlice { + input: *const u8, + input_len: u32, + /// STREAM_* flags + flags: u8, + offset: u64, +} + +impl StreamSlice { + + /// Create a StreamSlice from a Rust slice. Useful in unit tests. + pub fn from_slice(slice: &[u8], flags: u8, offset: u64) -> Self { + Self { + input: slice.as_ptr() as *const u8, + input_len: slice.len() as u32, + flags, + offset + } + } + + pub fn is_gap(&self) -> bool { + self.input.is_null() && self.input_len > 0 + } + pub fn gap_size(&self) -> u32 { + self.input_len + } + pub fn as_slice(&self) -> &[u8] { + unsafe { std::slice::from_raw_parts(self.input, self.input_len as usize) } + } + pub fn is_empty(&self) -> bool { + self.input_len == 0 + } + pub fn len(&self) -> u32 { + self.input_len + } + pub fn offset_from(&self, slice: &[u8]) -> u32 { + self.len() - slice.len() as u32 + } + pub fn flags(&self) -> u8 { + self.flags + } +} #[repr(C)] #[derive(Debug,PartialEq)] pub struct AppLayerTxConfig { diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index 9403a26620dd..18ffbe267ee8 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,21 @@ impl DNSState { } } - pub fn parse_response(&mut self, input: &[u8]) -> bool { + fn parse_request_udp(&mut self, stream_slice: StreamSlice) -> bool { + let input = stream_slice.as_slice(); + self.parse_request(input, false) + } + + fn parse_response_udp(&mut self, stream_slice: StreamSlice) -> bool { + let input = stream_slice.as_slice(); + 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 +644,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 +691,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 +730,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 +839,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(StreamSlice::from_slice(buf, STREAM_TOSERVER, 0)); + AppLayerResult::ok() } #[no_mangle] @@ -830,11 +854,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(StreamSlice::from_slice(buf, STREAM_TOCLIENT, 0)); + AppLayerResult::ok() } /// C binding parse a DNS request. Returns 1 on success, -1 on failure. @@ -1387,7 +1408,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 +1428,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 +1448,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 +1472,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 +1496,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. From 01c7161ee1f48facab1aa5af730dfb24b7e9162c Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 21 Dec 2022 09:35:19 -0600 Subject: [PATCH 6/6] dns: split header and body parsing As part of extra header validation, split out DNS body parsing to avoid the overhead of parsing the header twice. (cherry picked from commit d720ead470bcb5dd5a0c0ae7db302ab170205ee6) --- rust/src/dns/dns.rs | 51 ++++++++++++++++------------- rust/src/dns/parser.rs | 73 +++++++++++++++++++++--------------------- 2 files changed, 64 insertions(+), 60 deletions(-) diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index 18ffbe267ee8..aeb4943cda91 100644 --- a/rust/src/dns/dns.rs +++ b/rust/src/dns/dns.rs @@ -29,7 +29,7 @@ use crate::core::STREAM_TOSERVER; use crate::core::{self, AppProto, ALPROTO_UNKNOWN, IPPROTO_UDP, IPPROTO_TCP}; use crate::dns::parser; -use nom::IResult; +use nom::{Err, IResult}; use nom::number::streaming::be_u16; /// DNS record types. @@ -500,18 +500,23 @@ impl DNSState { event as u8); } - 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 validate_header<'a>(&self, input: &'a [u8]) -> Option<(&'a [u8], DNSHeader)> { + if let Ok((body, header)) = parser::dns_parse_header(input) { + if probe_header_validity(&header, input.len()).0 { + return Some((body, header)); + } + } + None } fn parse_request(&mut self, input: &[u8], is_tcp: bool) -> bool { - if !self.validate_header(input) { + let (body, header) = if let Some((body, header)) = self.validate_header(input) { + (body, header) + } else { return !is_tcp; - } + }; - match parser::dns_parse_request(input) { + match parser::dns_parse_request_body(body, input, header) { Ok((_, request)) => { if request.header.flags & 0x8000 != 0 { SCLogDebug!("DNS message is not a request"); @@ -564,11 +569,13 @@ impl DNSState { } pub fn parse_response(&mut self, input: &[u8], is_tcp: bool) -> bool { - if !self.validate_header(input) { + let (body, header) = if let Some((body, header)) = self.validate_header(input) { + (body, header) + } else { return !is_tcp; - } + }; - match parser::dns_parse_response(input) { + match parser::dns_parse_response_body(body, input, header) { Ok((_, response)) => { SCLogDebug!("Response header flags: {}", response.header.flags); @@ -729,7 +736,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) -> (bool, bool, bool) { let min_msg_size = 2 * (header.additional_rr as usize + header.answer_rr as usize @@ -758,7 +765,7 @@ 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); } else { return (false, false, false); } @@ -766,17 +773,15 @@ fn probe(input: &[u8], dlen: usize) -> (bool, bool, bool) { match parser::dns_parse_request(input) { Ok((_, request)) => { - return probe_header_validity(request.header, dlen); - }, - Err(nom::Err::Incomplete(_)) => { - match parser::dns_parse_header(input) { - Ok((_, header)) => { - return probe_header_validity(header, dlen); - } - Err(nom::Err::Incomplete(_)) => (false, false, true), - Err(_) => (false, false, false), - } + return probe_header_validity(&request.header, dlen); } + Err(Err::Incomplete(_)) => match parser::dns_parse_header(input) { + Ok((_, header)) => { + return probe_header_validity(&header, dlen); + } + Err(Err::Incomplete(_)) => (false, false, true), + Err(_) => (false, false, false), + }, Err(_) => (false, false, false), } } diff --git a/rust/src/dns/parser.rs b/rust/src/dns/parser.rs index d25e0ad23310..dd16c8f49b55 100644 --- a/rust/src/dns/parser.rs +++ b/rust/src/dns/parser.rs @@ -20,6 +20,7 @@ use nom::IResult; use nom::combinator::rest; use nom::error::ErrorKind; +use nom::multi::count; use nom::number::streaming::{be_u8, be_u16, be_u32}; use nom; use crate::dns::dns::*; @@ -50,10 +51,8 @@ named!(pub dns_parse_header, /// /// Parameters: /// start: the start of the name -/// message: the complete message that start is a part of -pub fn dns_parse_name<'a, 'b>(start: &'b [u8], - message: &'b [u8]) - -> IResult<&'b [u8], Vec> { +/// message: the complete message that start is a part of with the DNS header +pub fn dns_parse_name<'b>(start: &'b [u8], message: &'b [u8]) -> IResult<&'b [u8], Vec> { let mut pos = start; let mut pivot = start; let mut name: Vec = Vec::with_capacity(32); @@ -197,26 +196,27 @@ fn dns_parse_answer<'a>(slice: &'a [u8], message: &'a [u8], count: usize) /// Parse a DNS response. -pub fn dns_parse_response<'a>(slice: &'a [u8]) - -> IResult<&[u8], DNSResponse> { - do_parse!( - slice, - header: dns_parse_header - >> queries: count!( - call!(dns_parse_query, slice), header.questions as usize) - >> answers: call!( - dns_parse_answer, slice, header.answer_rr as usize) - >> authorities: call!( - dns_parse_answer, slice, header.authority_rr as usize) - >> ( - DNSResponse{ - header: header, - queries: queries, - answers: answers, - authorities: authorities, - } - ) - ) +pub fn dns_parse_response(message: &[u8]) -> IResult<&[u8], DNSResponse> { + let i = message; + let (i, header) = dns_parse_header(i)?; + dns_parse_response_body(i, message, header) +} + +pub fn dns_parse_response_body<'a>( + i: &'a [u8], message: &'a [u8], header: DNSHeader, +) -> IResult<&'a [u8], DNSResponse> { + 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)?; + Ok(( + i, + DNSResponse { + header, + queries, + answers, + authorities, + }, + )) } /// Parse a single DNS query. @@ -344,19 +344,18 @@ pub fn dns_parse_rdata<'a>(input: &'a [u8], message: &'a [u8], rrtype: u16) } /// Parse a DNS request. -pub fn dns_parse_request<'a>(input: &'a [u8]) -> IResult<&[u8], DNSRequest> { - do_parse!( - input, - header: dns_parse_header >> - queries: count!(call!(dns_parse_query, input), - header.questions as usize) >> - ( - DNSRequest{ - header: header, - queries: queries, - } - ) - ) +pub fn dns_parse_request(input: &[u8]) -> IResult<&[u8], DNSRequest> { + let i = input; + let (i, header) = dns_parse_header(i)?; + dns_parse_request_body(i, input, header) +} + +pub fn dns_parse_request_body<'a>( + input: &'a [u8], message: &'a [u8], header: DNSHeader, +) -> IResult<&'a [u8], DNSRequest> { + let i = input; + let (i, queries) = count(|b| dns_parse_query(b, message), header.questions as usize)(i)?; + Ok((i, DNSRequest { header, queries })) } #[cfg(test)]