fix: reject IPFIX Sets declaring an impossible length

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

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;
+                }
             }
         }
 

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;
+                }
             }
         }
 

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

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"
         );
     }