From 95f3ea587b3fddc6f96c91e0eb6e428c9dec080e Mon Sep 17 00:00:00 2001 From: mikemiles-dev Date: Sun, 11 Oct 2026 16:35:41 -0500 Subject: [PATCH] fix: reject IPFIX Sets declaring an impossible length IPFIX Set iteration used many0, which stops at the first Set it cannot parse, and the surrounding combinator then threw away the unparsed tail. A Set declaring a length below the 4-octet Set Header, or one overrunning the Set region, therefore blocked the walk and every valid Data Set behind it was discarded while the message still reported success. That is silent data loss. A truncated datagram decoded as a short but apparently complete message, so a collector under-reported flows with no error to act on. Message-level length was already validated; only Set-level framing was not. Validate the Set Header chain and reject the message per RFC 7011 section 9.1. The whole chain is validated before any Set is parsed. Checking up front rather than at the point many0 stopped catches a bad length wherever it sits, commits no template or pending-flow state from a message that is about to be rejected, and keeps the offending offset, which map_res would otherwise discard. Only framing is judged: a well-formed Set whose contents this parser does not decode keeps its existing lenient handling, since skipping it safely is a separate concern. Failures surface as NetflowError::MalformedSetFraming, carrying the offset, the declared length and the octets actually available. That is distinct from NetflowError::Partial, which continues to mean a datagram truncated in flight, so a collector can tell an exporter emitting illegal Set lengths from one whose datagrams are being cut short. The violation travels on the parser, mirroring DecodedOutputBudget::take_exceeded, because map_res substitutes its own error over the entire Set region; formatting that into the message turned a 65,535-octet datagram into a 262,139-octet error string and ignored max_error_sample_size. 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, and rejecting the message had been throwing that away: the snapshot for the affected capture went from a full header to an empty list. A collector needs the sequence number to account for the records it did not receive and the domain to know which exporter to chase, so the rejection reports both, alongside where the chain broke. The capture's snapshot now covers the error rather than the empty packet list, which records strictly more than it did before -- the same header fields plus the declared and available lengths that identify the fault. Trailing octets too few to begin a Set are tolerated as padding, 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. Rejecting every zero tail would cost a padded message every record it carried. A longer zero run means truncation or zero-filling and is rejected. Rejecting a message no longer discards the rest of the datagram. RFC 7011 section 9.1 discards the Message, but parse_bytes broke out of its loop on 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 Message Header states where the next one begins, so ParsedNetflow::MessageDiscarded now carries that resume point and parsing continues. A declared length below the 16-octet header, or one that leaves nothing behind it, is not a trustworthy boundary and stays a hard stop; the resume slice is always strictly shorter than the input, so no caller can stall. parse_bytes, iter_packets and AutoScopedParser::parse_from_source all resume, the last re-deriving the scope from each surviving message's header. v5, v7 and v9 cannot locate the next message after a failure and are unchanged. Two consequences. Several messages in one datagram can now fail, so the reported error is the first rather than the last, and the break arms use get_or_insert so a later hard error cannot overwrite it. And iter_packets previously stopped iterating after any error; it now yields the error and continues when the next message is locatable, which is what its Result-yielding signature already implied. Two bundled captures were affected and are now rejected. Both are internally inconsistent: can_read_ipfix declares a 48-octet message whose first Set claims 36 of its 32 available octets, and the options capture declares field_count 20 and a 176-octet Set while carrying 46 field specifications, leaving 20 octets that cannot form a Set. Neither decoded a single flowset beforehand -- their snapshots recorded an empty flowsets list -- so neither was testing what its name claimed. Both are renamed to describe the framing they actually exercise, which leaves the crate needing a well-formed capture to restore template-field and options-template coverage. Validating before the walk means a rejected message commits no template, so malformed_message_does_not_commit_an_earlier_template is no longer ignored. That 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. The remaining half of #310 stays out of scope: a Set that is well framed but whose body cannot be decoded still ends the walk and drops the Sets behind it silently. The up-front validation makes stepping over such a Set feasible, since every length is known good before the walk starts, but choosing between skipping and erroring is a separate decision. Reproducer from Costa Tsaousis (#310), extended with the case that carries the real impact -- a valid Data Set behind a malformed one -- a truncation case, a chained datagram whose second message must survive, the padding and zero-tail boundaries, iterator resumption, and a guard that the framing error stays small. Verified with both fuzz targets: 5.2M and 6.5M runs, no crashes. Co-Authored-By: Claude Opus 5 (1M context) --- RELEASES.md | 55 +++ src/lib.rs | 81 ++++- src/scoped_parser.rs | 16 +- ...er__tests__base_tests__can_read_ipfix.snap | 12 - ...ts__overrunning_ipfix_set_is_rejected.snap | 11 + src/tests.rs | 64 +++- src/variable_versions/ipfix/parser.rs | 53 ++- src/variable_versions/ipfix/types.rs | 115 +++++++ tests/ipfix_set_framing.rs | 315 ++++++++++++++++++ 9 files changed, 689 insertions(+), 33 deletions(-) delete mode 100644 src/snapshots/netflow_parser__tests__base_tests__can_read_ipfix.snap create mode 100644 src/snapshots/netflow_parser__tests__base_tests__overrunning_ipfix_set_is_rejected.snap create mode 100644 tests/ipfix_set_framing.rs 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() + ); +}