diff --git a/RELEASES.md b/RELEASES.md index 1cf836b6..42867715 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -2,6 +2,61 @@ ## Fixes +* **Reject IPFIX Sets that declare an impossible length.** Set iteration + stopped at the first Set it could not parse and then discarded the rest of + the message without reporting anything, so a Set declaring a length below the + 4-octet Set Header, or one overrunning the Set region, silently destroyed + every valid Data Set behind it while the message still reported success. A + truncated datagram therefore decoded as a short but apparently complete + message, and a collector had no way to detect the loss. Such framing is now + rejected per [RFC 7011 section 9.1](https://www.rfc-editor.org/rfc/rfc7011.html#section-9.1). + Reported by Costa Tsaousis (#310). + + The whole Set Header chain is validated before any Set is parsed, so a bad + length is caught wherever it sits rather than only where the walk stopped, + and a message rejected for framing commits no template or pending-flow + state. Failures surface as the new `NetflowError::MalformedSetFraming`, + carrying the offending Set's offset, its declared length and the octets + actually available — distinct from `NetflowError::Partial`, which continues + to mean a datagram truncated in flight. + + The error also carries the discarded message's `export_time`, + `sequence_number` and `observation_domain_id`. The Message Header parses + cleanly even when its Sets do not, so a rejection still names the exporter + that emitted the illegal length and the sequence number the lost records sat + behind, which is what a collector needs to account for the gap. + + Only Set Header framing is judged. A well-formed Set whose contents this + parser does not decode keeps its previous handling. Trailing octets too few + to begin a Set are tolerated as before, as is a trailing run of up to 7 zero + octets: Sets align to 4- or 8-octet boundaries, so that is the largest + legitimate alignment tail. A longer zero run means a truncated or zero-filled + message and is rejected. + + **Behavior change:** messages that previously parsed as successful-but-empty + now return an error. Two bundled test captures were affected, both internally + inconsistent — one declares a 48-octet message whose first Set claims 36 of + its 32 available octets, the other leaves 20 octets that cannot form a Set. + Neither decoded any flowset before this change. + +* **A malformed IPFIX message no longer discards the rest of the datagram.** + RFC 7011 section 9.1 discards the *Message*, but parsing stopped at the first + message-level error and abandoned every message chained behind it — so one + bad message cost a whole datagram's worth of flows. The IPFIX Message Header + carries its own length, so when it locates a successor inside the buffer, + that message is now reported as discarded and parsing resumes at the next + one. `parse_bytes` and `iter_packets` both resume, as does + `AutoScopedParser::parse_from_source`, which re-derives the scope from each + surviving message's header. + + Because several messages in one datagram can now fail, `ParseResult.error` + and the error yielded by `iter_packets` report the **first** error rather than + the last. `iter_packets` previously stopped iterating after any error; it now + yields the error and continues when the next message is locatable, and still + stops on errors that leave no trustworthy boundary to resume from. v5, v7 and + v9 are unaffected: none of them can locate the next message after a failure, + so they remain a hard stop. + * **Route each chained message to its own scope in `AutoScopedParser`.** `parse_from_source` derived the scope once from the first message's header and then parsed the entire buffer with that one child parser. A single call diff --git a/src/lib.rs b/src/lib.rs index fcfa3fdd..24c74a6d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1008,6 +1008,16 @@ pub(crate) enum ParsedNetflow<'a> { Error { error: NetflowError, }, + /// This message is discarded, but its header located the next one, so + /// parsing can resume instead of abandoning the rest of the datagram. + /// Only produced for protocols that carry a message length (IPFIX); v5, + /// v7 and v9 have no way to find the next message after a failure. + MessageDiscarded { + error: NetflowError, + /// Bytes starting at the next message. Always strictly shorter than + /// the input, so the caller's loop cannot stall. + remaining: &'a [u8], + }, UnallowedVersion { version: u16, }, @@ -1075,6 +1085,34 @@ pub enum NetflowError { message: String, }, + /// An IPFIX Set Header declared a length that cannot be walked past: + /// either below the 4-octet Set Header itself, or beyond the end of the + /// Set region. + /// + /// The whole Message is discarded per + /// [RFC 7011 section 9.1](https://www.rfc-editor.org/rfc/rfc7011.html#section-9.1). + /// Distinct from [`NetflowError::Partial`], which reports a datagram + /// truncated in flight: this one means the exporter emitted an illegal Set + /// length, and carries the offset so the offending Set can be located. + MalformedSetFraming { + /// Octet offset of the offending Set Header, relative to the end of + /// the 16-octet Message Header. + offset: usize, + /// Length the Set Header declared. + declared: usize, + /// Octets actually remaining from `offset` to the end of the Set region. + available: usize, + /// Export time from the discarded message's header. + export_time: u32, + /// Sequence number from the discarded message's header. The Message + /// Header parsed cleanly even though its Sets did not, so a collector + /// can still account for the records it did not receive. + sequence_number: u32, + /// Observation Domain ID from the discarded message's header, naming + /// the exporter that emitted the illegal Set length. + observation_domain_id: u32, + }, + /// A complete v9/IPFIX message would exceed a configured decoded-output limit. DecodedOutputLimitExceeded { /// Protocol of the rejected message. @@ -1129,6 +1167,19 @@ impl std::fmt::Display for NetflowError { NetflowError::Partial { message } => { write!(f, "Partial parse error: {}", message) } + NetflowError::MalformedSetFraming { + offset, + declared, + available, + export_time, + sequence_number, + observation_domain_id, + } => write!( + f, + "Malformed IPFIX Set framing: Set at offset {offset} declared length {declared}, \ + {available} octets available in the Set region (observation domain \ + {observation_domain_id}, sequence {sequence_number}, export time {export_time})" + ), NetflowError::DecodedOutputLimitExceeded { protocol, limit, @@ -1241,6 +1292,16 @@ impl<'a> Iterator for NetflowPacketIterator<'a> { self.remaining = &[]; Some(Err(error)) } + ParsedNetflow::MessageDiscarded { + error, + remaining: new_remaining, + } => { + // Recoverable: this message is discarded but the next one was + // located, so yield the error and keep iterating rather than + // abandoning the rest of the datagram. + self.remaining = new_remaining; + Some(Err(error)) + } } } } @@ -1757,14 +1818,28 @@ impl NetflowParser { remaining = new_remaining; } ParsedNetflow::UnallowedVersion { version } => { - error = Some(NetflowError::FilteredVersion { version }); + error.get_or_insert(NetflowError::FilteredVersion { version }); break; } ParsedNetflow::Error { error: e } => { - // Store error but keep successfully parsed packets - error = Some(e); + // Store error but keep successfully parsed packets. A + // discarded message earlier in the datagram may already + // have recorded one, and that first error is the one + // closest to the root cause. + error.get_or_insert(e); break; } + ParsedNetflow::MessageDiscarded { + error: e, + remaining: new_remaining, + } => { + // RFC 7011 Section 9.1 discards the Message, not the rest + // of the datagram. The header located the next message, so + // keep going; the first error is the one reported, since + // `ParseResult` carries a single error. + error.get_or_insert(e); + remaining = new_remaining; + } } } diff --git a/src/scoped_parser.rs b/src/scoped_parser.rs index 50e2869f..60c6dd86 100644 --- a/src/scoped_parser.rs +++ b/src/scoped_parser.rs @@ -850,13 +850,25 @@ impl AutoScopedParser { remaining = new_remaining; } ParsedNetflow::UnallowedVersion { version } => { - error = Some(NetflowError::FilteredVersion { version }); + error.get_or_insert(NetflowError::FilteredVersion { version }); break; } ParsedNetflow::Error { error: e } => { - error = Some(e); + // Keep the first error; a discarded message earlier in the + // chain may already have recorded one. + error.get_or_insert(e); break; } + ParsedNetflow::MessageDiscarded { + error: e, + remaining: new_remaining, + } => { + // Only this message is discarded, so keep walking the + // chain; the loop re-derives the scope from the next + // message's header on the following iteration. + error.get_or_insert(e); + remaining = new_remaining; + } } } diff --git a/src/snapshots/netflow_parser__tests__base_tests__can_read_ipfix.snap b/src/snapshots/netflow_parser__tests__base_tests__can_read_ipfix.snap deleted file mode 100644 index 684fc1b1..00000000 --- a/src/snapshots/netflow_parser__tests__base_tests__can_read_ipfix.snap +++ /dev/null @@ -1,12 +0,0 @@ ---- -source: src/tests.rs -expression: ipfix ---- -- IPFix: - header: - version: 10 - length: 48 - export_time: 1654698425 - sequence_number: 8 - observation_domain_id: 1818918417 - flowsets: [] diff --git a/src/snapshots/netflow_parser__tests__base_tests__overrunning_ipfix_set_is_rejected.snap b/src/snapshots/netflow_parser__tests__base_tests__overrunning_ipfix_set_is_rejected.snap new file mode 100644 index 00000000..cf29b4a9 --- /dev/null +++ b/src/snapshots/netflow_parser__tests__base_tests__overrunning_ipfix_set_is_rejected.snap @@ -0,0 +1,11 @@ +--- +source: src/tests.rs +expression: error +--- +MalformedSetFraming: + offset: 0 + declared: 36 + available: 32 + export_time: 1654698425 + sequence_number: 8 + observation_domain_id: 1818918417 diff --git a/src/tests.rs b/src/tests.rs index deaf0bfc..00db85b5 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -246,9 +246,19 @@ mod base_tests { })); } - // Verify that an IPFIX packet with template fields is parsed correctly - #[test] - fn can_read_ipfix() { + // This capture declares a 48-octet message, leaving a 32-octet Set region, + // but its first Set declares 36 octets and so overruns that region. It is + // now rejected as malformed. + // + // The previous snapshot recorded a successful parse whose flowsets list was + // empty: the overrunning Set was dropped silently and nothing was decoded, + // so this vector never exercised template parsing despite its name. + // + // The capture also carries 10 octets past its declared length. Parsing + // resumes there and finds no valid version, so the framing error is the + // first of two; `ParseResult` reports the first. + #[test] + fn overrunning_ipfix_set_is_rejected() { let hex = "000a003062a0b1b9000000086c6a7e110001002400030008000100040002000400090001000d00010004000400080004000c00040005000100060001"; let mut parser = NetflowParser::builder() @@ -257,13 +267,35 @@ mod base_tests { .unwrap(); let packet = hex::decode(hex).unwrap(); - let ipfix = parser.parse_bytes(&packet).packets; - assert_yaml_snapshot!(ipfix); - } - - // Verify that an IPFIX options template with many fields is parsed without error - #[test] - fn options() { + let result = parser.parse_bytes(&packet); + assert!( + result.packets.is_empty(), + "a discarded message must not yield packets" + ); + // Snapshot the error, not the empty packet list: the Message Header + // parsed cleanly even though its Sets did not, so the rejection still + // names the exporter and the sequence number the lost records sat + // behind. The capture also carries 10 octets past its declared length, + // which parsing resumes into and rejects as an unknown version; the + // framing error comes first and is the one reported. + let error = result.error; + assert_yaml_snapshot!(error); + } + + // This capture is internally inconsistent and is now rejected as malformed. + // + // Its Options Template record declares field_count 20 (a 86-octet record) + // and its Set declares 176 octets, but 46 field specifications are + // physically present. That leaves 20 octets after the Set inside a + // 196-octet Set region, and those octets declare a Set length of 2 — below + // the 4-octet Set Header, so the walk cannot advance past them. + // + // Previously the trailing octets were dropped silently and the message + // reported success. It now reports a framing error instead of pretending + // the message was fully consumed. This vector therefore no longer provides + // options-template coverage; a well-formed capture is needed for that. + #[test] + fn options_template_with_inconsistent_set_framing_is_rejected() { let hex_options_template = "000a00d46319088e0000036e8b7148d2000300b0000e001400070006000f0006001000060034000200350006008800020090000200910002009200020093000200990002009a0002009e0002009f000200a1000200a2000200a3000200a4000200a5000200aa000200ab000200ac000200ad000200b0000200b1000200b2000200b4000200b5000200b6000200b7000200b8000200b9000200bd000200be000200bf000200c0000200c1000400c2000200c3000200c4000200c5000200c6000200c7000200c8000200c9000200ca000200cb0002"; let mut parser = NetflowParser::builder() @@ -274,12 +306,16 @@ mod base_tests { let packet = hex::decode(hex_options_template).unwrap(); let result = parser.parse_bytes(&packet); assert!( - !result.packets.is_empty(), - "expected packets from options template" + matches!( + result.error, + Some(crate::NetflowError::MalformedSetFraming { .. }) + ), + "expected a framing error for the trailing 20 octets, got {:?}", + result.error ); assert!( - result.error.is_none(), - "unexpected error parsing options template" + result.packets.is_empty(), + "a malformed message must not yield packets" ); } diff --git a/src/variable_versions/ipfix/parser.rs b/src/variable_versions/ipfix/parser.rs index d3b7f527..0698ca38 100644 --- a/src/variable_versions/ipfix/parser.rs +++ b/src/variable_versions/ipfix/parser.rs @@ -147,6 +147,7 @@ impl IPFixParser { template_store: config.template_store, template_store_scope: config.template_store_scope, restored_templates: Vec::new(), + set_framing_error: None, }) } @@ -528,6 +529,8 @@ impl IPFixParser { // Reset the per-parse restored-templates buffer so the next call // sees only what was restored during *this* packet. self.restored_templates.clear(); + // Likewise for any framing violation left by a previous message. + self.set_framing_error = None; match IPFix::parse(packet, self) { Ok((remaining, mut ipfix)) => { self.process_pending_flows(&mut ipfix); @@ -537,7 +540,16 @@ impl IPFixParser { } } Err(e) => { - let error = if let Some(exceeded) = self.decoded_output_budget.take_exceeded() { + let error = if let Some(violation) = self.set_framing_error.take() { + NetflowError::MalformedSetFraming { + offset: violation.offset, + declared: violation.declared, + available: violation.available, + export_time: violation.export_time, + sequence_number: violation.sequence_number, + observation_domain_id: violation.observation_domain_id, + } + } else if let Some(exceeded) = self.decoded_output_budget.take_exceeded() { NetflowError::DecodedOutputLimitExceeded { protocol: TemplateProtocol::Ipfix, limit: exceeded.limit, @@ -549,11 +561,48 @@ impl IPFixParser { message: format!("IPFIX parse error: {}", e), } }; - ParsedNetflow::Error { error } + // RFC 7011 Section 9.1 discards the Message. The Message + // Header carries its own length, so when it points inside this + // buffer the next message can still be parsed rather than + // losing the rest of the datagram with it. + match Self::next_message(packet) { + Some(remaining) => ParsedNetflow::MessageDiscarded { error, remaining }, + None => ParsedNetflow::Error { error }, + } } } } + /// Bytes following this message, derived from the Message Header's own + /// length field, or `None` when that length does not locate a further + /// message inside `packet`. + /// + /// `packet` starts just past the 2-octet Version field, which + /// `parse_packet_by_version` has already consumed, so the declared length + /// is read from the front and the message occupies `declared - 2` octets + /// of this slice. + /// + /// Returns `None` for a length below the 16-octet Message Header or one + /// that does not leave a further message, since neither can be trusted to + /// point at a real message boundary. The returned slice is always strictly + /// shorter than `packet`, so callers resuming from it cannot stall. + fn next_message(packet: &[u8]) -> Option<&[u8]> { + /// Total octets of an IPFIX Message Header, Version field included. + const MESSAGE_HEADER_LEN: usize = 16; + /// Version octets already consumed before `packet` begins. + const VERSION_LEN: usize = 2; + + let declared = usize::from(u16::from_be_bytes([*packet.first()?, *packet.get(1)?])); + if declared < MESSAGE_HEADER_LEN { + return None; + } + let consumed = declared - VERSION_LEN; + if consumed >= packet.len() { + return None; + } + packet.get(consumed..) + } + fn process_pending_flows(&mut self, ipfix: &mut IPFix) { let Some(mut pending_cache) = self.pending_flows.take() else { return; diff --git a/src/variable_versions/ipfix/types.rs b/src/variable_versions/ipfix/types.rs index 512c2b76..b6bf890e 100644 --- a/src/variable_versions/ipfix/types.rs +++ b/src/variable_versions/ipfix/types.rs @@ -65,6 +65,94 @@ pub struct IPFixParser { /// parse. Drained after each `parse` call to fire `TemplateEvent::Restored` /// hooks and to drive pending-flow replay. pub(crate) restored_templates: Vec<(TemplateProtocol, u16)>, + /// Set Header framing violation found while validating the in-flight + /// message, if any. Taken by `IPFixParser::parse` to build a precise + /// error. Mirrors the `DecodedOutputBudget::take_exceeded` pattern, since + /// `map_res` discards the nom error we construct inside the `Parse` + /// attribute. + pub(crate) set_framing_error: Option, +} + +/// A Set Header inside an IPFIX message declared a length that cannot be +/// walked past: either below the 4-octet Set Header itself, or beyond the end +/// of the Set region. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub(crate) struct SetFramingViolation { + /// Octet offset of the offending Set Header within the Set region, i.e. + /// relative to the end of the 16-octet Message Header. + pub(crate) offset: usize, + /// Length the offending Set Header declared. + pub(crate) declared: usize, + /// Octets actually remaining from `offset` to the end of the Set region. + pub(crate) available: usize, + /// Export time from the discarded message's header. + pub(crate) export_time: u32, + /// Sequence number from the discarded message's header. + pub(crate) sequence_number: u32, + /// Observation Domain ID from the discarded message's header. + pub(crate) observation_domain_id: u32, +} + +/// Where a Set Header chain broke, independent of which message carried it. +/// [`SetFramingViolation`] pairs this with the discarded message's header. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub(crate) struct SetFramingLocation { + pub(crate) offset: usize, + pub(crate) declared: usize, + pub(crate) available: usize, +} + +/// Largest trailing run of zero octets tolerated as inter-Set alignment +/// padding. Exporters align Sets to 4- or 8-octet boundaries, so a legitimate +/// tail is at most 7 octets; a longer zero run means the message was truncated +/// or zero-filled, which must not be mistaken for padding. +const MAX_TRAILING_PADDING: usize = 7; + +/// Walks the Set Header chain across `region` and reports the first Set whose +/// declared length cannot be walked past. +/// +/// Only framing is judged — Set contents are not decoded here. Running this +/// before any Set is parsed means a message rejected for bad framing commits +/// no template or pending-flow state, and gives the offending offset, which +/// `map_res` would otherwise discard. The caller pairs the location with the +/// discarded message's header, so the rejection still identifies the exporter +/// and the sequence number the lost records sat behind. +/// +/// A trailing run of fewer than 4 octets cannot begin a Set, and a trailing +/// run of up to [`MAX_TRAILING_PADDING`] zero octets is alignment padding; +/// both are accepted. +pub(crate) fn validate_set_framing(region: &[u8]) -> Result<(), SetFramingLocation> { + let mut offset = 0usize; + loop { + let tail = ®ion[offset..]; + if tail.len() < 4 { + // Too short to begin a Set; tolerated as trailing padding. + return Ok(()); + } + let declared = usize::from(u16::from_be_bytes([tail[2], tail[3]])); + if declared < 4 { + // An all-zero tail is alignment padding rather than a Set. A zero + // run followed by more data is not, so the `all` check below is + // what keeps this from excusing a zero gap mid-message. + if tail.len() <= MAX_TRAILING_PADDING && tail.iter().all(|&b| b == 0) { + return Ok(()); + } + return Err(SetFramingLocation { + offset, + declared, + available: tail.len(), + }); + } + if declared > tail.len() { + return Err(SetFramingLocation { + offset, + declared, + available: tail.len(), + }); + } + // `declared >= 4` guarantees progress, so this terminates. + offset += declared; + } } /// A parsed IPFIX message containing a header and a list of flowsets. @@ -81,6 +169,33 @@ pub struct IPFix { #[nom( PreExec = "let length = header.length.checked_sub(16).ok_or(nom::Err::Error(nom::error::Error::new(i, nom::error::ErrorKind::Verify)))?;", Parse = "map_res(take(length), |i| { + // Validate the whole Set Header chain before walking it. many0 + // stops at the first Set it cannot parse and the tail is then + // dropped silently, so a Set declaring an impossible length + // discards every valid Data Set behind it while the message still + // reports success. Reject that framing per RFC 7011 Section 9.1. + // + // Checking up front rather than at the point many0 stopped means + // no template or pending-flow state from the message is committed + // before the rejection, and catches a bad length anywhere in the + // chain rather than only at the stop point. Only framing is judged: + // a well-framed Set whose contents this parser does not decode + // keeps its existing lenient handling, since skipping it safely is + // a separate concern. + // + // map_res discards the nom error constructed here, so the precise + // violation is stashed on the parser for IPFixParser::parse. + if let Err(location) = validate_set_framing(i) { + parser.set_framing_error = Some(SetFramingViolation { + offset: location.offset, + declared: location.declared, + available: location.available, + export_time: header.export_time, + sequence_number: header.sequence_number, + observation_domain_id: header.observation_domain_id, + }); + return Err(nom::Err::Error(nom::error::Error::new(i, nom::error::ErrorKind::Verify))); + } many0(complete(|i| FlowSet::parse(i, parser) .map(|(i, flow_set)| (i, flow_set)) ))(i) diff --git a/tests/ipfix_set_framing.rs b/tests/ipfix_set_framing.rs new file mode 100644 index 00000000..cd916b82 --- /dev/null +++ b/tests/ipfix_set_framing.rs @@ -0,0 +1,315 @@ +//! A Set declaring an impossible length must be reported, not dropped silently. +//! +//! The reproducer in `malformed_message_does_not_commit_an_earlier_template` +//! comes from Costa Tsaousis (netflow_parser#310). + +use netflow_parser::{NetflowError, NetflowParser}; + +fn message(body: &[u8]) -> Vec { + let mut m = Vec::new(); + m.extend_from_slice(&10u16.to_be_bytes()); + m.extend_from_slice(&0u16.to_be_bytes()); // patched below + m.extend_from_slice(&1u32.to_be_bytes()); + m.extend_from_slice(&2u32.to_be_bytes()); + m.extend_from_slice(&3u32.to_be_bytes()); + m.extend_from_slice(body); + let len = u16::try_from(m.len()).unwrap(); + m[2..4].copy_from_slice(&len.to_be_bytes()); + m +} + +/// Template Set defining Template 256 with one 4-octet sourceIPv4Address. +fn template_set() -> Vec { + let mut s = Vec::new(); + s.extend_from_slice(&2u16.to_be_bytes()); + s.extend_from_slice(&12u16.to_be_bytes()); + s.extend_from_slice(&256u16.to_be_bytes()); + s.extend_from_slice(&1u16.to_be_bytes()); + s.extend_from_slice(&8u16.to_be_bytes()); + s.extend_from_slice(&4u16.to_be_bytes()); + s +} + +/// Data Set for Template 256 carrying one record. +fn data_set(payload: [u8; 4]) -> Vec { + let mut s = Vec::new(); + s.extend_from_slice(&256u16.to_be_bytes()); + s.extend_from_slice(&8u16.to_be_bytes()); + s.extend_from_slice(&payload); + s +} + +fn data_record_count(packets: &[netflow_parser::NetflowPacket]) -> usize { + packets + .iter() + .filter_map(|p| match p { + netflow_parser::NetflowPacket::IPFix(i) => Some(i), + _ => None, + }) + .flat_map(|i| i.flowsets.iter()) + .map(|fs| match &fs.body { + netflow_parser::variable_versions::ipfix::FlowSetBody::Data(d) => d.fields.len(), + _ => 0, + }) + .sum() +} + +/// A Set length below the 4-octet Set Header cannot be walked past. +#[test] +fn malformed_set_length_is_reported() { + let mut body = template_set(); + body.extend_from_slice(&2u16.to_be_bytes()); + body.extend_from_slice(&3u16.to_be_bytes()); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + + assert!(result.error.is_some(), "malformed framing must be reported"); + assert!(result.packets.is_empty()); +} + +/// The remaining half of netflow_parser#310: RFC 7011 Section 9.1 requires +/// discarding the whole message, so no state from its valid prefix should +/// survive. +/// +/// Framing is validated across the whole Set chain before any Set is parsed, +/// so a framing rejection commits no template. This is not full transactional +/// discard — a message rejected for some other reason mid-walk can still leave +/// a template behind — but it covers every message this check rejects. +#[test] +fn malformed_message_does_not_commit_an_earlier_template() { + let mut body = template_set(); + body.extend_from_slice(&2u16.to_be_bytes()); + body.extend_from_slice(&3u16.to_be_bytes()); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + + assert!(result.error.is_some()); + assert!(result.packets.is_empty()); + assert!(!parser.has_ipfix_template(256)); +} + +/// The case with teeth: a valid Data Set behind a malformed one was silently +/// discarded while the message still reported success. +#[test] +fn data_behind_a_malformed_set_is_not_silently_discarded() { + let mut body = template_set(); + body.extend_from_slice(&2u16.to_be_bytes()); + body.extend_from_slice(&3u16.to_be_bytes()); + body.extend_from_slice(&data_set([192, 0, 2, 2])); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + + assert!( + matches!( + result.error, + Some(NetflowError::MalformedSetFraming { + offset: 12, + declared: 3, + available: 12, + observation_domain_id: 3, + sequence_number: 2, + export_time: 1, + .. + }) + ), + "losing a valid Data Set must not be reported as success, and the \ + offending Set must be locatable: {:?}", + result.error + ); +} + +/// Truncation: a Set header claiming more octets than the message carries. +#[test] +fn set_overrunning_the_message_is_rejected() { + let mut body = template_set(); + body.extend_from_slice(&256u16.to_be_bytes()); + body.extend_from_slice(&64u16.to_be_bytes()); // claims 64, only 4 follow + body.extend_from_slice(&[1, 2, 3, 4]); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + + assert!(result.error.is_some(), "truncated Set must be reported"); +} + +/// Guard against over-correction: well-formed messages must still parse. +/// Trailing padding is covered by `trailing_zero_padding_is_tolerated`. +#[test] +fn well_formed_messages_are_unaffected() { + let mut body = template_set(); + body.extend_from_slice(&data_set([192, 0, 2, 1])); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + assert!(result.error.is_none(), "{:#?}", result.error); + assert_eq!(data_record_count(&result.packets), 1); +} + +/// Trailing zero octets are alignment padding, not a Set declaring length 0. +/// Sets align to 4- or 8-octet boundaries, so a legitimate tail is at most 7 +/// octets; rejecting all of them cost every record in the message. +#[test] +fn trailing_zero_padding_is_tolerated() { + for padding in 1..=7usize { + let mut body = template_set(); + body.extend_from_slice(&data_set([192, 0, 2, 1])); + body.extend_from_slice(&vec![0u8; padding]); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + assert!( + result.error.is_none(), + "{padding} octets of trailing padding must be tolerated: {:?}", + result.error + ); + assert_eq!( + data_record_count(&result.packets), + 1, + "{padding} octets of padding must not cost the message's records" + ); + } +} + +/// A zero run too long to be alignment padding is a truncated or zero-filled +/// message, and must not be excused as padding. +#[test] +fn an_over_long_zero_tail_is_still_rejected() { + let mut body = template_set(); + body.extend_from_slice(&data_set([192, 0, 2, 1])); + body.extend_from_slice(&[0u8; 8]); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + assert!( + matches!(result.error, Some(NetflowError::MalformedSetFraming { .. })), + "8 zero octets cannot be alignment padding: {:?}", + result.error + ); +} + +/// A zero run in the middle of a message is not padding either, even though +/// its last 4 octets would read as a Set declaring length 0 on their own. +#[test] +fn a_zero_gap_mid_message_is_rejected() { + let mut body = template_set(); + body.extend_from_slice(&[0u8; 4]); + body.extend_from_slice(&data_set([192, 0, 2, 1])); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + assert!( + matches!(result.error, Some(NetflowError::MalformedSetFraming { .. })), + "a zero gap mid-message is not padding: {:?}", + result.error + ); +} + +/// RFC 7011 Section 9.1 discards the *Message*. A datagram carrying several +/// chained messages must not lose the ones behind a malformed sibling, since +/// each Message Header declares its own length. +#[test] +fn a_malformed_message_does_not_discard_the_rest_of_the_datagram() { + let mut bad = template_set(); + bad.extend_from_slice(&2u16.to_be_bytes()); + bad.extend_from_slice(&3u16.to_be_bytes()); + + let mut good = template_set(); + good.extend_from_slice(&data_set([192, 0, 2, 9])); + + let mut datagram = message(&bad); + datagram.extend_from_slice(&message(&good)); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&datagram); + + assert!( + matches!(result.error, Some(NetflowError::MalformedSetFraming { .. })), + "the malformed message must still be reported: {:?}", + result.error + ); + assert_eq!( + data_record_count(&result.packets), + 1, + "the following message's Data Set must survive" + ); +} + +/// `iter_packets` resumes after a discarded message too, so the two APIs agree +/// about which errors are recoverable. +#[test] +fn the_iterator_resumes_after_a_discarded_message() { + let mut bad = template_set(); + bad.extend_from_slice(&2u16.to_be_bytes()); + bad.extend_from_slice(&3u16.to_be_bytes()); + + let mut good = template_set(); + good.extend_from_slice(&data_set([192, 0, 2, 9])); + + let mut datagram = message(&bad); + datagram.extend_from_slice(&message(&good)); + + let mut parser = NetflowParser::default(); + let items: Vec<_> = parser.iter_packets(&datagram).collect(); + + assert_eq!( + items.len(), + 2, + "expected one error then one packet: {items:#?}" + ); + assert!(matches!( + items[0], + Err(NetflowError::MalformedSetFraming { .. }) + )); + let packets: Vec<_> = items.into_iter().filter_map(Result::ok).collect(); + assert_eq!(data_record_count(&packets), 1); +} + +/// A message whose header length does not locate a successor is a hard stop: +/// there is no trustworthy boundary to resume from, and resuming must never +/// stall the caller's loop. +#[test] +fn a_message_with_no_locatable_successor_stops_parsing() { + let mut body = template_set(); + body.extend_from_slice(&2u16.to_be_bytes()); + body.extend_from_slice(&3u16.to_be_bytes()); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&message(&body)); + assert!(matches!( + result.error, + Some(NetflowError::MalformedSetFraming { .. }) + )); + assert!(result.packets.is_empty()); +} + +/// The framing error must stay small. It previously formatted the whole Set +/// region into a string, amplifying an attacker-chosen datagram several times +/// over while ignoring `max_error_sample_size`. +#[test] +fn the_framing_error_does_not_echo_the_message() { + let mut body = template_set(); + body.extend_from_slice(&data_set([1, 2, 3, 4])); + + // One well-framed but undecodable Set filling the message, then 4 octets + // declaring a Set length of 0. + let filler_len = 65535 - 16 - body.len() - 4; + body.extend_from_slice(&999u16.to_be_bytes()); + body.extend_from_slice(&u16::try_from(filler_len).unwrap().to_be_bytes()); + body.extend_from_slice(&vec![7u8; filler_len - 4]); + body.extend_from_slice(&[0u8, 2, 0, 0]); + + let datagram = message(&body); + assert_eq!(datagram.len(), 65535); + + let mut parser = NetflowParser::default(); + let result = parser.parse_bytes(&datagram); + let rendered = result.error.as_ref().map(ToString::to_string).unwrap(); + assert!( + rendered.len() < 256, + "error must not echo the message ({} bytes): {rendered}", + rendered.len() + ); +}