Flowspec improvements - #1083
Open
nuclearcat wants to merge 9 commits into
Open
nuclearcat wants to merge 9 commits into
nuclearcat wants to merge 9 commits into
Conversation
RFC 8955 Section 4.2.2.4 defines type 4 as matching either the source or destination TCP/UDP port. The decoder previously rejected every NLRI containing this standard component, including the RFC's basic TCP port 25 example. Represent generic ports separately from source and destination ports so packet filtering can implement the required OR semantics without weakening type 5 or type 6 matches. Gate matching on TCP or UDP so protocol-specific overloads of flow port fields, such as NetFlow's ICMP type and code encoding, cannot accidentally satisfy a generic-port rule. Decode and encode type 4 operands through this field, and include it in equality, Boost serialization, and JSON round trips so the component remains intact across FastNetMon interfaces.
Extract the repeated equality-only port encoder used by generic, destination, and source port components. Keeping operator construction and network-byte-order conversion in one place prevents the three component encoders from drifting. Likewise, route generic ports, source ports, destination ports, and packet lengths through one uint16 component decoder that accepts the target rule adder. This centralizes operand parsing, pointer advancement, and component-specific error reporting. Consolidate JSON parsing and validation for ports, destination_ports, and source_ports through one local helper, and document the new ports key in the canonical JSON example.
RFC 8955 Section 4.2.2.4 states that port, destination-port, and source-port components only match TCP or UDP packets. Generic ports already enforced this rule, but the pre-existing type 5 and type 6 filters still inspected their port fields for every protocol. Compute one TCP-or-UDP predicate and require it for all populated port component forms. This prevents NetFlow representations that overload port fields for other protocols, notably ICMP type and code, from accidentally satisfying source-port or destination-port FlowSpec rules.
nuclearcat
force-pushed
the
flowspec-improvements
branch
from
July 11, 2026 05:59
49075b8 to
c09160c
Compare
nuclearcat
marked this pull request as draft
July 11, 2026 06:06
Implement RFC 8955 type 7 (ICMP Type) and type 8 (ICMP Code) as dedicated uint8 rule fields. Encode and decode their numeric operands in component order, reject decoded or JSON values above 255, and preserve the fields through equality, Boost serialization, and JSON round trips. Apply either component only to IPv4 ICMP packets whose fragment offset is zero. Track whether packet type and code were actually supplied because zero is valid for both; absent collector metadata must not accidentally satisfy an echo-reply rule. Populate that metadata from raw packets and NetFlow v5, decode the standard combined and individual ICMP fields used by NetFlow v9 and IPFIX, and preserve values decoded from embedded NetFlow Lite packets. For exporters which overload destination port as type << 8 | code, use that fallback only when field 11 is present in the template and do not override dedicated ICMP fields.
RFC 8955 Section 4.1 encodes FlowSpec NLRI lengths below 240 in one octet and lengths from 240 through 4095 as a two-octet 0xfnnn value. The MP_REACH decoder previously rejected every extended length. Decode the low 12 bits of the extended form and pass the resulting payload length to the existing component parser. Reject a missing second length octet and non-canonical extended encodings below 240. Bound both the length field and declared NLRI payload by the parsed MP_REACH attribute value. Also make a pre-existing parser assumption explicit: RFC 8955 requires a zero-length next hop, while the fixed header layout cannot safely parse a nonzero one. Reject such malformed attributes before their next-hop bytes can be interpreted as the NLRI length.
RFC 8955 Section 4.2.2.9 permits TCP flag bitmasks encoded in either one or two octets. Accept both operand sizes, use the low octet for FIN through CWR, preserve the NS bit from octet 13, and ignore the data-offset and reserved bits as required by the RFC. Extend FlowSpec flag sets, text and JSON conversion, equality, and Boost serialization with ECE, CWR, and NS. Encode NS-bearing rules with a two-octet mask while retaining the compact one-octet form when NS is absent. Also record the missing value_length metadata for parsed two-octet operands. Retain ECE, CWR, and NS while parsing raw TCP packets so the expanded masks can be evaluated. Restrict TCP-flag matching to TCP packets with fragment offset zero, preventing other protocols and non-first fragments from satisfying the component.
RFC 8955 Section 4.2.2.4 specifies that generic, destination, and source port components never match a fragmented packet when it is not the first fragment, because the transport header cannot be located there. Replace the protocol-only TCP/UDP predicate with a shared port-component applicability check that also requires fragment offset zero. Apply it consistently to FlowSpec types 4, 5, and 6.
The FlowSpec encoder stored the generated NLRI size in a uint8_t before checking whether it required the RFC 8955 extended form. This is a pre-existing bug: sizes above 255 wrapped before the >= 240 guard, allowing a malformed one-octet length to be emitted. Encoding the newly supported port, ICMP type, and ICMP code lists in the same NLRI makes that old condition substantially easier to reach. A rule needs only roughly four bytes per encoded port, so otherwise reasonable multi-value rules can now cross the boundary. Keep the length as size_t, retain the compact encoding below 240 bytes, and emit the two-octet 0xFnnn form for larger NLRIs. Also use BGP's extended attribute-length header when the enclosing MP_REACH value exceeds 255 bytes, and size the output buffer from the payload. This makes the large-payload support symmetric with the extended-length decoder.
nuclearcat
force-pushed
the
flowspec-improvements
branch
from
July 11, 2026 06:13
c09160c to
6d501aa
Compare
The shared operator-byte reader limits each component to 100 operands, but previously returned success when that loop was exhausted without observing an end-of-list bit. The reported byte count then left the caller at operand 101, whose operator byte was misinterpreted as the next component type. This parser flaw predates the current FlowSpec work, but is exercised by more paths now that port, ICMP type, and ICMP code components use the shared reader. Preserve the 100-operand resource bound while requiring an explicit end-of-list bit for success; reject capped, unterminated components instead of continuing from a false boundary.
nuclearcat
marked this pull request as ready for review
July 11, 2026 10:44
Owner
|
Hello! Thank you very much! I'll review it carefully |
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
2 participants
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.
This commits bring improved conformance with RFC8955, including some corner cases (like validating ports without verifying if it is UDP/TCP protocol).
The new generic-port, ICMP type/code, tcp flags ECE/CWR, two-octet TCP-mask, and large component-payload cases passes now mrtgen tests.