Skip to content

Commit

Permalink
dns: provide events for recoverable parse errors
Browse files Browse the repository at this point in the history
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 point to a flag
fields 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.
  • Loading branch information
jasonish committed Nov 22, 2024
1 parent 4868d26 commit 2ee7b24
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 61 deletions.
9 changes: 9 additions & 0 deletions rules/dns-events.rules
Original file line number Diff line number Diff line change
Expand Up @@ -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;)
38 changes: 35 additions & 3 deletions rust/src/dns/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -383,7 +389,7 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result<DNSTransaction, DNSParse
};

match parser::dns_parse_body(body, input, header) {
Ok((_, request)) => {
Ok((_, (request, parse_flags))) => {
if request.header.flags & 0x8000 != 0 {
SCLogDebug!("DNS message is not a request");
return Err(DNSParseError::NotRequest);
Expand All @@ -399,10 +405,23 @@ pub(crate) fn dns_parse_request(input: &[u8]) -> Result<DNSTransaction, DNSParse
SCLogDebug!("Z-flag set on DNS request");
tx.set_event(DNSEvent::ZFlagSet);
}

if opcode >= 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(_)) => {
Expand All @@ -426,7 +445,7 @@ pub(crate) fn dns_parse_response(input: &[u8]) -> Result<DNSTransaction, DNSPars
};

match parser::dns_parse_body(body, input, header) {
Ok((_, response)) => {
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;
Expand All @@ -444,10 +463,23 @@ pub(crate) fn dns_parse_response(input: &[u8]) -> Result<DNSTransaction, DNSPars
SCLogDebug!("Z-flag set on DNS response");
tx.set_event(DNSEvent::ZFlagSet);
}

if opcode >= 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(_)) => {
Expand Down Expand Up @@ -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),
},
Expand Down
Loading

0 comments on commit 2ee7b24

Please sign in to comment.