Repository navigation
fix: reject IPFIX Sets declaring an impossible length - #330
Merged
Merged
Conversation
mikemiles-dev
force-pushed
the
fix/ipfix-set-framing-validation
branch
from
October 11, 2026 21:36
a5ec44f to
22d8215
Compare
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) <noreply@anthropic.com>
mikemiles-dev
force-pushed
the
fix/ipfix-set-framing-validation
branch
from
October 11, 2026 21:41
22d8215 to
95f3ea5
Compare
mikemiles-dev
added a commit
that referenced
this pull request
Oct 11, 2026
#330 retired two bundled captures as malformed and noted that the crate therefore needed a well-formed capture to restore template-field and options-template coverage. That note was wrong on both counts. IPFIX Template Sets are exercised by roughly a dozen test files and by pcaps/IPFIX.pcap, which asserts 15 cached templates and exact hit and miss counts across 6,666 packets. IPFIX Options Template Sets are exercised by tests/decoded_output_limits.rs, tests/round_trip.rs and tests/template_store.rs. Neither needed restoring. What the retired can_read_ipfix capture actually exercised is narrower and was genuinely left uncovered. It declares Set ID 1, which an IPFIX message routes to the V9-style options template path, not to anything IPFIX-native, so the capture never covered IPFIX template fields despite its name. Add a well-formed version-10 message carrying a Set ID 1 V9 options template and assert it reaches the parser's cache with its scope and option fields intact. Assertions rather than a snapshot. Both retired captures were snapshot tests over opaque hex, and their snapshots recorded an empty flowsets list for years without anyone noticing they had stopped decoding anything. Correct the RELEASES note to say what was retired and what replaced it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Oct 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 a new
NetflowError::MalformedSetFraming, carrying the offset, the declared length and the octets actually available. That is distinct fromNetflowError::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, mirroringDecodedOutputBudget::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 ignoredmax_error_sample_size.The error also carries the discarded message's
export_time,sequence_numberandobservation_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. That 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, since 4 zero octets read as a Set declaring length 0. 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 -- and this change makes rejection considerably more likely. The Message Header states where the next one begins, so
ParsedNetflow::MessageDiscardednow 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 andAutoScopedParser::parse_from_sourceall 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 for callers. 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 it 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.