From: Jason Ish Date: Fri, 1 Nov 2024 17:39:23 +0000 (-0600) Subject: dns: provide events for recoverable parse errors X-Git-Tag: suricata-8.0.0-beta1~645 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=19cf0f81335d9f787d587450f7105ad95a648951;p=thirdparty%2Fsuricata.git dns: provide events for recoverable parse errors Add events for the following resource name parsing issues: - name truncated as its too long - maximum number of labels reached - infinite loop Currently these events are only registered when encountered, but recoverable. That is where we are able to return some of the name, usually in a truncated state. As name parsing has many code paths, we pass in a pointer to a flag field that can be updated by the name parser, this is done in addition to the flags being set on a specific name as when logging we want to designate which fields are truncated, etc. But for alerts, we just care that something happened during the parse. It also reduces errors as it won't be forgotten to check for the flags and set the event if some new parser is written that also parses names. Ticket: #7280 --- diff --git a/rules/dns-events.rules b/rules/dns-events.rules index 6f3f711f8e..a3cf982c1e 100644 --- a/rules/dns-events.rules +++ b/rules/dns-events.rules @@ -8,3 +8,12 @@ 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;) + +# A resource name was too long (over 1025 chars) +alert dns any any -> any any (msg:"SURICATA DNS Name too long"; app-layer-event:dns.name_too_long; classtype:protocol-command-decode; sid:224008; rev:1;) + +# An infinite loop was found while decoding a DNS resource name. +alert dns any any -> any any (msg:"SURICATA DNS Infinite loop"; app-layer-event:dns.infinite_loop; classtype:protocol-command-decode; sid:224009; rev:1;) + +# 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;) diff --git a/rust/src/dns/dns.rs b/rust/src/dns/dns.rs index b16516b657..ce67e577df 100644 --- a/rust/src/dns/dns.rs +++ b/rust/src/dns/dns.rs @@ -129,6 +129,12 @@ pub enum DNSEvent { NotResponse, ZFlagSet, InvalidOpcode, + /// A DNS resource name was exessively long and was truncated. + NameTooLong, + /// An infinite loop was found while parsing a name. + InfiniteLoop, + /// Too many labels were found. + TooManyLabels, } #[derive(Debug, PartialEq, Eq)] @@ -383,7 +389,7 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result { + Ok((_, (request, parse_flags))) => { if request.header.flags & 0x8000 != 0 { SCLogDebug!("DNS message is not a request"); return Err(DNSParseError::NotRequest); @@ -399,10 +405,23 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result= 7 { tx.set_event(DNSEvent::InvalidOpcode); } + if parse_flags.contains(DNSNameFlags::TRUNCATED) { + tx.set_event(DNSEvent::NameTooLong); + } + + if parse_flags.contains(DNSNameFlags::INFINITE_LOOP) { + tx.set_event(DNSEvent::InfiniteLoop); + } + + if parse_flags.contains(DNSNameFlags::LABEL_LIMIT) { + tx.set_event(DNSEvent::TooManyLabels); + } + return Ok(tx); } Err(Err::Incomplete(_)) => { @@ -426,7 +445,7 @@ pub(crate) fn dns_parse_response(input: &[u8]) -> Result { + Ok((_, (response, parse_flags))) => { SCLogDebug!("Response header flags: {}", response.header.flags); let z_flag = response.header.flags & 0x0040 != 0; let opcode = ((response.header.flags >> 11) & 0xf) as u8; @@ -444,10 +463,23 @@ pub(crate) fn dns_parse_response(input: &[u8]) -> Result= 7 { tx.set_event(DNSEvent::InvalidOpcode); } + if parse_flags.contains(DNSNameFlags::TRUNCATED) { + tx.set_event(DNSEvent::NameTooLong); + } + + if parse_flags.contains(DNSNameFlags::INFINITE_LOOP) { + tx.set_event(DNSEvent::InfiniteLoop); + } + + if parse_flags.contains(DNSNameFlags::LABEL_LIMIT) { + tx.set_event(DNSEvent::TooManyLabels); + } + return Ok(tx); } Err(Err::Incomplete(_)) => { @@ -778,7 +810,7 @@ fn probe(input: &[u8], dlen: usize) -> (bool, bool, bool) { 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, _flags))) => probe_header_validity(&request.header, dlen), Err(Err::Incomplete(_)) => (false, false, true), Err(_) => (false, false, false), }, diff --git a/rust/src/dns/parser.rs b/rust/src/dns/parser.rs index e705d8b825..7bdfe024fe 100644 --- a/rust/src/dns/parser.rs +++ b/rust/src/dns/parser.rs @@ -60,7 +60,9 @@ static MAX_NAME_LEN: usize = 1025; /// Parameters: /// start: the start of the name /// message: the complete message that start is a part of with the DNS header -fn dns_parse_name<'b>(start: &'b [u8], message: &'b [u8]) -> IResult<&'b [u8], DNSName> { +fn dns_parse_name<'b>( + start: &'b [u8], message: &'b [u8], parse_flags: &mut DNSNameFlags, +) -> IResult<&'b [u8], DNSName> { let mut pos = start; let mut pivot = start; let mut name: Vec = Vec::with_capacity(32); @@ -145,6 +147,8 @@ fn dns_parse_name<'b>(start: &'b [u8], message: &'b [u8]) -> IResult<&'b [u8], D } } + parse_flags.insert(flags); + // If we followed a pointer we return the position after the first // pointer followed. Is there a better way to see if these slices // diverged from each other? A straight up comparison would @@ -167,7 +171,7 @@ fn dns_parse_name<'b>(start: &'b [u8], message: &'b [u8]) -> IResult<&'b [u8], D /// multi-string TXT entry as a single quote string, similar to the /// output of dig. Something to consider for a future version. fn dns_parse_answer<'a>( - slice: &'a [u8], message: &'a [u8], count: usize, + slice: &'a [u8], message: &'a [u8], count: usize, flags: &mut DNSNameFlags, ) -> IResult<&'a [u8], Vec> { let mut answers = Vec::new(); let mut input = slice; @@ -180,8 +184,10 @@ fn dns_parse_answer<'a>( data: &'a [u8], } - fn subparser<'a>(i: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], Answer<'a>> { - let (i, name) = dns_parse_name(i, message)?; + fn subparser<'a>( + i: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, + ) -> IResult<&'a [u8], Answer<'a>> { + let (i, name) = dns_parse_name(i, message, flags)?; let (i, rrtype) = be_u16(i)?; let (i, rrclass) = be_u16(i)?; let (i, ttl) = be_u32(i)?; @@ -197,7 +203,7 @@ fn dns_parse_answer<'a>( } for _ in 0..count { - match subparser(input, message) { + match subparser(input, message, flags) { Ok((rem, val)) => { let n = match val.rrtype { DNS_RECORD_TYPE_TXT => { @@ -227,8 +233,11 @@ fn dns_parse_answer<'a>( input = rem; continue; } - let result: IResult<&'a [u8], Vec> = - many_m_n(1, n, complete(|b| dns_parse_rdata(b, message, val.rrtype)))(val.data); + let result: IResult<&'a [u8], Vec> = many_m_n( + 1, + n, + complete(|b| dns_parse_rdata(b, message, val.rrtype, flags)), + )(val.data); match result { Ok((_, rdatas)) => { for rdata in rdatas { @@ -261,9 +270,11 @@ fn dns_parse_answer<'a>( /// Arguments are suitable for using with call!: /// /// call!(complete_dns_message_buffer) -fn dns_parse_query<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSQueryEntry> { +fn dns_parse_query<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSQueryEntry> { let i = input; - let (i, name) = dns_parse_name(i, message)?; + let (i, name) = dns_parse_name(i, message, flags)?; let (i, rrtype) = be_u16(i)?; let (i, rrclass) = be_u16(i)?; Ok(( @@ -284,22 +295,30 @@ fn dns_parse_rdata_aaaa(input: &[u8]) -> IResult<&[u8], DNSRData> { rest(input).map(|(input, data)| (input, DNSRData::AAAA(data.to_vec()))) } -fn dns_parse_rdata_cname<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { - dns_parse_name(input, message).map(|(input, name)| (input, DNSRData::CNAME(name))) +fn dns_parse_rdata_cname<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { + dns_parse_name(input, message, flags).map(|(input, name)| (input, DNSRData::CNAME(name))) } -fn dns_parse_rdata_ns<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { - dns_parse_name(input, message).map(|(input, name)| (input, DNSRData::NS(name))) +fn dns_parse_rdata_ns<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { + dns_parse_name(input, message, flags).map(|(input, name)| (input, DNSRData::NS(name))) } -fn dns_parse_rdata_ptr<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { - dns_parse_name(input, message).map(|(input, name)| (input, DNSRData::PTR(name))) +fn dns_parse_rdata_ptr<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { + dns_parse_name(input, message, flags).map(|(input, name)| (input, DNSRData::PTR(name))) } -fn dns_parse_rdata_soa<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { +fn dns_parse_rdata_soa<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { let i = input; - let (i, mname) = dns_parse_name(i, message)?; - let (i, rname) = dns_parse_name(i, message)?; + let (i, mname) = dns_parse_name(i, message, flags)?; + let (i, rname) = dns_parse_name(i, message, flags)?; let (i, serial) = be_u32(i)?; let (i, refresh) = be_u32(i)?; let (i, retry) = be_u32(i)?; @@ -319,20 +338,24 @@ fn dns_parse_rdata_soa<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u )) } -fn dns_parse_rdata_mx<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { +fn dns_parse_rdata_mx<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { // For MX we skip over the preference field before // parsing out the name. let (i, _) = be_u16(input)?; - let (i, name) = dns_parse_name(i, message)?; + let (i, name) = dns_parse_name(i, message, flags)?; Ok((i, DNSRData::MX(name))) } -fn dns_parse_rdata_srv<'a>(input: &'a [u8], message: &'a [u8]) -> IResult<&'a [u8], DNSRData> { +fn dns_parse_rdata_srv<'a>( + input: &'a [u8], message: &'a [u8], flags: &mut DNSNameFlags, +) -> IResult<&'a [u8], DNSRData> { let i = input; let (i, priority) = be_u16(i)?; let (i, weight) = be_u16(i)?; let (i, port) = be_u16(i)?; - let (i, target) = dns_parse_name(i, message)?; + let (i, target) = dns_parse_name(i, message, flags)?; Ok(( i, DNSRData::SRV(DNSRDataSRV { @@ -389,20 +412,20 @@ fn dns_parse_rdata_unknown(input: &[u8]) -> IResult<&[u8], DNSRData> { } fn dns_parse_rdata<'a>( - input: &'a [u8], message: &'a [u8], rrtype: u16, + input: &'a [u8], message: &'a [u8], rrtype: u16, flags: &mut DNSNameFlags, ) -> IResult<&'a [u8], DNSRData> { match rrtype { DNS_RECORD_TYPE_A => dns_parse_rdata_a(input), DNS_RECORD_TYPE_AAAA => dns_parse_rdata_aaaa(input), - DNS_RECORD_TYPE_CNAME => dns_parse_rdata_cname(input, message), - DNS_RECORD_TYPE_PTR => dns_parse_rdata_ptr(input, message), - DNS_RECORD_TYPE_SOA => dns_parse_rdata_soa(input, message), - DNS_RECORD_TYPE_MX => dns_parse_rdata_mx(input, message), - DNS_RECORD_TYPE_NS => dns_parse_rdata_ns(input, message), + DNS_RECORD_TYPE_CNAME => dns_parse_rdata_cname(input, message, flags), + DNS_RECORD_TYPE_PTR => dns_parse_rdata_ptr(input, message, flags), + DNS_RECORD_TYPE_SOA => dns_parse_rdata_soa(input, message, flags), + DNS_RECORD_TYPE_MX => dns_parse_rdata_mx(input, message, flags), + DNS_RECORD_TYPE_NS => dns_parse_rdata_ns(input, message, flags), DNS_RECORD_TYPE_TXT => dns_parse_rdata_txt(input), DNS_RECORD_TYPE_NULL => dns_parse_rdata_null(input), DNS_RECORD_TYPE_SSHFP => dns_parse_rdata_sshfp(input), - DNS_RECORD_TYPE_SRV => dns_parse_rdata_srv(input, message), + DNS_RECORD_TYPE_SRV => dns_parse_rdata_srv(input, message, flags), DNS_RECORD_TYPE_OPT => dns_parse_rdata_opt(input), _ => dns_parse_rdata_unknown(input), } @@ -431,20 +454,27 @@ pub fn dns_parse_header(i: &[u8]) -> IResult<&[u8], DNSHeader> { pub fn dns_parse_body<'a>( i: &'a [u8], message: &'a [u8], header: DNSHeader, -) -> 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)?; +) -> IResult<&'a [u8], (DNSMessage, DNSNameFlags)> { + let mut flags = DNSNameFlags::default(); + let (i, queries) = count( + |b| dns_parse_query(b, message, &mut flags), + 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)?; Ok(( i, - DNSMessage { - header, - queries, - answers, - authorities, - additionals, - }, + ( + DNSMessage { + header, + queries, + answers, + authorities, + additionals, + }, + flags, + ), )) } @@ -464,7 +494,8 @@ mod tests { 0x03, 0x63, 0x6f, 0x6d, 0x00, 0x00, 0x01, 0x00, /* .com.... */ ]; let expected_remainder: &[u8] = &[0x00, 0x01, 0x00]; - let (remainder, name) = dns_parse_name(buf, buf).unwrap(); + let mut flags = DNSNameFlags::default(); + let (remainder, name) = dns_parse_name(buf, buf, &mut flags).unwrap(); assert_eq!("client-cf.dropbox.com".as_bytes(), &name.value[..]); assert_eq!(remainder, expected_remainder); } @@ -498,7 +529,8 @@ mod tests { // The name at offset 54 is the complete name. let start1 = &buf[54..]; - let res1 = dns_parse_name(start1, message); + let mut flags = DNSNameFlags::default(); + let res1 = dns_parse_name(start1, message, &mut flags); assert_eq!( res1, Ok(( @@ -513,7 +545,8 @@ mod tests { // The second name starts at offset 80, but is just a pointer // to the first. let start2 = &buf[80..]; - let res2 = dns_parse_name(start2, message); + let mut flags = DNSNameFlags::default(); + let res2 = dns_parse_name(start2, message, &mut flags); assert_eq!( res2, Ok(( @@ -528,7 +561,8 @@ mod tests { // The third name starts at offset 94, but is a pointer to a // portion of the first. let start3 = &buf[94..]; - let res3 = dns_parse_name(start3, message); + let mut flags = DNSNameFlags::default(); + let res3 = dns_parse_name(start3, message, &mut flags); assert_eq!( res3, Ok(( @@ -543,7 +577,8 @@ mod tests { // The fourth name starts at offset 110, but is a pointer to a // portion of the first. let start4 = &buf[110..]; - let res4 = dns_parse_name(start4, message); + let mut flags = DNSNameFlags::default(); + let res4 = dns_parse_name(start4, message, &mut flags); assert_eq!( res4, Ok(( @@ -584,7 +619,8 @@ mod tests { // packet). let start: &[u8] = &buf[100..]; - let res = dns_parse_name(start, message); + let mut flags = DNSNameFlags::default(); + let res = dns_parse_name(start, message, &mut flags); assert_eq!( res, Ok(( @@ -612,7 +648,7 @@ mod tests { let (body, header) = dns_parse_header(pkt).unwrap(); let res = dns_parse_body(body, pkt, header); - let (rem, request) = res.unwrap(); + let (rem, (request, _flags)) = res.unwrap(); // The request should be fully parsed. assert!(rem.is_empty()); @@ -674,7 +710,7 @@ mod tests { let (body, header) = dns_parse_header(pkt).unwrap(); let res = dns_parse_body(body, pkt, header); - let (rem, request) = res.unwrap(); + let (rem, (request, _flags)) = res.unwrap(); assert!(rem.is_empty()); assert_eq!( @@ -726,7 +762,7 @@ mod tests { } /// Parse a DNS response. - fn dns_parse_response(message: &[u8]) -> IResult<&[u8], DNSMessage> { + fn dns_parse_response(message: &[u8]) -> IResult<&[u8], (DNSMessage, DNSNameFlags)> { let i = message; let (i, header) = dns_parse_header(i)?; dns_parse_body(i, message, header) @@ -751,7 +787,7 @@ mod tests { 0x00, 0x04, 0xc0, 0x00, 0x4e, 0x19, /* ....N. */ ]; - let (rem, response) = dns_parse_response(pkt).unwrap(); + let (rem, (response, _flags)) = dns_parse_response(pkt).unwrap(); // The response should be full parsed. assert_eq!(rem.len(), 0); @@ -839,7 +875,7 @@ mod tests { 0x00, 0x00, 0x00, 0x00, /* .... */ ]; - let (rem, response) = dns_parse_response(pkt).unwrap(); + let (rem, (response, _flags)) = dns_parse_response(pkt).unwrap(); // The response should be fully parsed. assert!(rem.is_empty()); @@ -916,7 +952,7 @@ mod tests { 0x44, 0x03, 0xc5, 0xe9, 0x01, /* D.... */ ]; - let (rem, response) = dns_parse_response(pkt).unwrap(); + let (rem, (response, _flags)) = dns_parse_response(pkt).unwrap(); // The response should be fully parsed. assert_eq!(rem.len(), 0); @@ -1017,7 +1053,7 @@ mod tests { 0x67, 0x6c, 0x65, 0x03, 0x63, 0x6f, 0x6d, 0x00, ]; - let (rem, response) = dns_parse_response(pkt).unwrap(); + let (rem, (response, _flags)) = dns_parse_response(pkt).unwrap(); // The data should be fully parsed. assert_eq!(rem.len(), 0); @@ -1060,7 +1096,8 @@ mod tests { } } - let (rem, name) = dns_parse_name(&buf, &buf).unwrap(); + let mut flags = DNSNameFlags::default(); + let (rem, name) = dns_parse_name(&buf, &buf, &mut flags).unwrap(); assert_eq!(name.value.len(), MAX_NAME_LEN); assert!(name.flags.contains(DNSNameFlags::TRUNCATED)); assert!(rem.is_empty()); @@ -1077,7 +1114,8 @@ mod tests { // This should fail as we've hit the segment limit without a // pointer, we'd need to keep parsing more segments to figure // out where the next data point lies. - assert!(dns_parse_name(&buf, &buf).is_err()); + let mut flags = DNSNameFlags::default(); + assert!(dns_parse_name(&buf, &buf, &mut flags).is_err()); } #[test] @@ -1097,7 +1135,8 @@ mod tests { buf.push(0b1100_0000); buf.push(0b000_0000); - let (_rem, name) = dns_parse_name(&buf[4..], &buf).unwrap(); + let mut flags = DNSNameFlags::default(); + let (_rem, name) = dns_parse_name(&buf[4..], &buf, &mut flags).unwrap(); assert_eq!(name.value.len(), 255); assert!(name.flags.contains(DNSNameFlags::TRUNCATED)); } @@ -1107,6 +1146,7 @@ mod tests { let mut buf = vec![]; buf.push(0b1100_0000); buf.push(0b0000_0000); - assert!(dns_parse_name(&buf, &buf).is_err()); + let mut flags = DNSNameFlags::default(); + assert!(dns_parse_name(&buf, &buf, &mut flags).is_err()); } }