diff --git a/AGENTS.md b/AGENTS.md index 272d563d..ffef5678 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,3 +48,14 @@ cargo fmt -- --check - The `bgpkit-parser` binary requires the `cli` feature (`required-features = ["cli"]`). - Examples under `examples/` may require specific features (see `[[example]]` entries in `Cargo.toml`). - Benchmarks use Criterion (`[[bench]]` entries). + +## Finding Missing Implementations + +When looking for incomplete or missing BGP attribute implementations, check `src/parser/bgp/attributes/README.md`. This file contains: + +1. **Unit Test Coverage** table — lists fully implemented attributes with RFC references +2. **Known Limitations** table — lists attributes that are: + - Type defined in `AttrType` enum but have no parser (e.g., PMSI_TUNNEL, BGPSEC_PATH) + - Model structs exist but parser/encoder not yet implemented (e.g., AIGP, ATTR_SET) + +Use this file to identify which attributes need implementation work and their corresponding RFCs. diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a416e55..2c5e90f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ All notable changes to this project will be documented in this file. * **Enhanced warning messages**: Added context to 14 warning messages across BGP, BMP, and MRT parsers to help identify which parsing stage encountered an issue. Also fixed typo in NLRI warning ("NRLI" → "NLRI"). * **Codecov configuration**: Added `require_base: true` to ensure Codecov only posts coverage reports when a valid base commit coverage exists, preventing comparisons against outdated baselines. +### Security improvements + +* **AS_PATH segment count validation**: Added bounds check to prevent buffer over-read when parsing AS_PATH segments. Validates that `count * asn_len.bytes()` does not exceed remaining buffer size (CWE-126). +* **BGP message length underflow protection**: Added saturating arithmetic to prevent integer underflow when calculating message length (CWE-191). +* **NLRI prefix length validation**: Enforce maximum prefix lengths (32 bits for IPv4, 128 bits for IPv6) immediately after reading length byte (CWE-20). +* **BGP marker RFC 4271 compliance**: Fixed marker encoding to use 16 bytes of 0xFF as required by RFC 4271. Added validation on parse with warning for non-compliant markers. +* **Communities divisibility validation**: Validate that Communities (4 bytes), Extended Communities (8 bytes), and Large Communities (12 bytes) attribute lengths are properly divisible by their respective sizes. +* **FlowSpec traffic rate validation**: Added NaN and infinity checks for FlowSpec Traffic Rate extended communities to prevent undefined behavior in downstream calculations. +* **FlowSpec DSCP byte offset**: Corrected Traffic Marking extended community parsing per RFC 5575 (DSCP at proper byte offset). +* **Production code safety**: Replaced `assert_eq!` with `debug_assert_eq!` in production parsing code to prevent panics on malformed input. +* **Timestamp overflow handling**: Fixed timestamp truncation for values beyond 2106 (u32::MAX) using proper bounds checking. +* **Encoding truncation checks**: Added overflow warnings for `as u8`/`as u16` casts in encoding functions. +* **IPv4 enforcement for AGGREGATOR/ORIGINATOR_ID**: Encoders now reject IPv6 addresses for these attributes (BGP specifications require IPv4 only). + ### Bug fixes * **WASM `only_to_customer` serialization**: Added test to verify that messages without the OTC (Only To Customer) attribute correctly serialize `only_to_customer` as `null` (not `0`) in JSON output. This ensures JavaScript consumers can properly distinguish between "no OTC attribute" (`null`) and "OTC with AS 0" (`0`). diff --git a/src/models/bgp/attributes/mod.rs b/src/models/bgp/attributes/mod.rs index ebc30afb..0242b55e 100644 --- a/src/models/bgp/attributes/mod.rs +++ b/src/models/bgp/attributes/mod.rs @@ -443,6 +443,66 @@ impl From for Attribute { } } +/// AIGP TLV (Type-Length-Value) entry - RFC 7311 +#[derive(Debug, PartialEq, Clone, Eq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct AigpTlv { + pub tlv_type: u8, + pub length: u16, + pub value: Vec, +} + +/// AIGP (Accumulated IGP Metric) Attribute - RFC 7311 +/// +/// Type 26, optional non-transitive attribute containing TLVs. +/// The AIGP TLV (Type=1) contains an 8-octet accumulated metric value. +#[derive(Debug, PartialEq, Clone, Eq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct Aigp { + pub tlvs: Vec, +} + +impl Aigp { + /// Get the accumulated metric from the first AIGP TLV (Type=1) + pub fn accumulated_metric(&self) -> Option { + self.tlvs + .iter() + .find(|tlv| tlv.tlv_type == 1) + .and_then(|tlv| { + if tlv.value.len() >= 8 { + Some(u64::from_be_bytes([ + tlv.value[0], + tlv.value[1], + tlv.value[2], + tlv.value[3], + tlv.value[4], + tlv.value[5], + tlv.value[6], + tlv.value[7], + ])) + } else { + None + } + }) + } +} + +/// ATTR_SET Attribute - RFC 6368 +/// +/// Used in BGP/MPLS IP VPNs to transparently carry customer BGP path attributes +/// through the VPN core. Acts as a stack where attributes are "pushed" at the +/// PE ingress and "popped" at the PE egress. +/// +/// Type 128, optional transitive attribute. +#[derive(Debug, PartialEq, Clone, Eq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct AttrSet { + /// Origin AS number (customer network AS) + pub origin_as: Asn, + /// Nested path attributes + pub attributes: Attributes, +} + /// The `AttributeValue` enum represents different kinds of Attribute values. #[derive(Debug, PartialEq, Clone, Eq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -477,6 +537,10 @@ pub enum AttributeValue { Development(Vec), Deprecated(AttrRaw), Unknown(AttrRaw), + /// AIGP (Accumulated IGP Metric) attribute - RFC 7311 + Aigp(Aigp), + /// ATTR_SET attribute - RFC 6368 + AttrSet(AttrSet), } impl From for AttributeValue { @@ -534,6 +598,8 @@ impl AttributeValue { AttributeValue::TunnelEncapsulation(_) => AttrType::TUNNEL_ENCAPSULATION, AttributeValue::Development(_) => AttrType::DEVELOPMENT, AttributeValue::Deprecated(x) | AttributeValue::Unknown(x) => x.attr_type, + AttributeValue::Aigp(_) => AttrType::AIGP, + AttributeValue::AttrSet(_) => AttrType::ATTR_SET, } } @@ -559,6 +625,8 @@ impl AttributeValue { AttributeValue::MpReachNlri(_) => Some(OptionalNonTransitive), AttributeValue::MpUnreachNlri(_) => Some(OptionalNonTransitive), AttributeValue::LinkState(_) => Some(OptionalNonTransitive), + AttributeValue::Aigp(_) => Some(OptionalNonTransitive), + AttributeValue::AttrSet(_) => Some(OptionalTransitive), _ => None, } } diff --git a/src/models/network/asn.rs b/src/models/network/asn.rs index a53dd560..13a4e6ec 100644 --- a/src/models/network/asn.rs +++ b/src/models/network/asn.rs @@ -20,6 +20,14 @@ impl AsnLength { AsnLength::Bits32 => true, } } + + /// Return the number of bytes used to encode an ASN of this length. + pub const fn bytes(&self) -> usize { + match self { + AsnLength::Bits16 => 2, + AsnLength::Bits32 => 4, + } + } } /// ASN -- Autonomous System Number diff --git a/src/parser/bgp/attributes/README.md b/src/parser/bgp/attributes/README.md index dcc6ac11..c444079c 100644 --- a/src/parser/bgp/attributes/README.md +++ b/src/parser/bgp/attributes/README.md @@ -19,11 +19,40 @@ | Large Community | [RFC8092][rfc8092] | 32 | Yes | | Only To Customer | [RFC9234][rfc9234] | 35 | Yes | +## Known Limitations + +| Path Attribute | RFC | Type Code | Status | Notes | +|---------------------------------|-------------------------------|-----------|-----------------------------|----------------------------------| +| AIGP (Accumulated IGP Metric) | [RFC7311][rfc7311] | 26 | Type defined, model only | Parser and encoding not yet implemented | +| ATTR_SET | [RFC6368][rfc6368] | 128 | Type defined, model only | Parser and encoding not yet implemented | +| PMSI_TUNNEL | [RFC6514][rfc6514] | 22 | Type defined in enum | Parser not implemented | +| TRAFFIC_ENGINEERING | [RFC5543][rfc5543] | 24 | Type defined in enum | Parser not implemented | +| IPv6_EXT_COMMUNITIES | [RFC5701][rfc5701] | 25 | ✅ Implemented | Listed in main table above | +| PE_DISTINGUISHER_LABELS | [RFC6514][rfc6514] | 27 | Type defined in enum | Parser not implemented | +| BGPSEC_PATH | [RFC8205][rfc8205] | 33 | Type defined in enum | Parser not implemented | +| SFP_ATTRIBUTE | [RFC9015][rfc9015] | 37 | Type defined in enum | Parser not implemented | +| BFD_DISCRIMINATOR | [RFC9026][rfc9026] | 38 | Type defined in enum | Parser not implemented | +| BGP_PREFIX_SID | [RFC8669][rfc8669] | 40 | Type defined in enum | Parser not implemented | + +**Legend:** +- **Type defined**: Attribute type code is defined in `AttrType` enum (models) +- **Model only**: Data structures exist but parser/encoder not implemented +- **Type defined in enum**: Only the type code exists, no parser or model + [rfc1997]: https://datatracker.ietf.org/doc/html/rfc1997 [rfc4271]: https://datatracker.ietf.org/doc/html/rfc4271#section-4.3 [rfc4360]: https://datatracker.ietf.org/doc/html/rfc4360 [rfc4456]: https://datatracker.ietf.org/doc/html/rfc4456 [rfc4760]: https://datatracker.ietf.org/doc/html/rfc4760 +[rfc5543]: https://datatracker.ietf.org/doc/html/rfc5543 +[rfc5701]: https://datatracker.ietf.org/doc/html/rfc5701 +[rfc6368]: https://datatracker.ietf.org/doc/html/rfc6368 +[rfc6514]: https://datatracker.ietf.org/doc/html/rfc6514 +[rfc7311]: https://datatracker.ietf.org/doc/html/rfc7311 [rfc8092]: https://datatracker.ietf.org/doc/html/rfc8092 +[rfc8205]: https://datatracker.ietf.org/doc/html/rfc8205 +[rfc8669]: https://datatracker.ietf.org/doc/html/rfc8669 +[rfc9015]: https://datatracker.ietf.org/doc/html/rfc9015 +[rfc9026]: https://datatracker.ietf.org/doc/html/rfc9026 [rfc9234]: https://datatracker.ietf.org/doc/html/rfc9234 [iana-bgp]: https://www.iana.org/assignments/bgp-parameters/bgp-parameters.xhtml \ No newline at end of file diff --git a/src/parser/bgp/attributes/attr_02_17_as_path.rs b/src/parser/bgp/attributes/attr_02_17_as_path.rs index 1ac8c30f..368f555a 100644 --- a/src/parser/bgp/attributes/attr_02_17_as_path.rs +++ b/src/parser/bgp/attributes/attr_02_17_as_path.rs @@ -27,6 +27,20 @@ fn parse_as_path_segment( ) -> Result { let segment_type = input.read_u8()?; let count = input.read_u8()? as usize; + + // Validate AS_PATH segment count against remaining buffer size + // to prevent reading beyond buffer bounds + let required_bytes = count + .checked_mul(asn_len.bytes()) + .ok_or_else(|| ParserError::ParseError("AS path segment too large".to_string()))?; + if input.remaining() < required_bytes { + return Err(ParserError::TruncatedMsg(format!( + "AS_PATH segment requires {} bytes, only {} remaining", + required_bytes, + input.remaining() + ))); + } + let path = input.read_asns(asn_len, count)?; match segment_type { AS_PATH_AS_SET => Ok(AsPathSegment::AsSet(path)), @@ -242,4 +256,38 @@ mod tests { let encoded_bytes = encode_as_path(&path, AsnLength::Bits16); assert_eq!(data, encoded_bytes); } + + #[test] + fn test_as_path_count_validation() { + // Test that excessive count value is rejected (16-bit ASN) + // count=100, but buffer only has 2 bytes + let data = Bytes::from(vec![ + 2, // sequence + 100, // 100 ASes claimed, but buffer is too small + 0, 1, // Only 2 bytes available (1 ASN, not 100) + ]); + let result = parse_as_path(data, &AsnLength::Bits16); + assert!(result.is_err()); + assert!(matches!(result, Err(ParserError::TruncatedMsg(_)))); + + // Test that excessive count value is rejected (32-bit ASN) + // count=50, but buffer only has 4 bytes + let data = Bytes::from(vec![ + 2, // sequence + 50, // 50 ASes claimed (need 200 bytes), but buffer is too small + 0, 0, 0, 1, // Only 4 bytes available (1 ASN, not 50) + ]); + let result = parse_as_path(data, &AsnLength::Bits32); + assert!(result.is_err()); + assert!(matches!(result, Err(ParserError::TruncatedMsg(_)))); + + // Test valid count passes + let data = Bytes::from(vec![ + 2, // sequence + 2, // 2 ASes + 0, 1, 0, 2, // 2 ASNs (4 bytes for 16-bit) + ]); + let result = parse_as_path(data, &AsnLength::Bits16); + assert!(result.is_ok()); + } } diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index 5cab0ec9..16ecf1fd 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -441,6 +441,14 @@ impl Attribute { AttributeValue::Development(v) => Bytes::from(v.to_owned()), AttributeValue::Deprecated(v) => Bytes::from(v.bytes.to_owned()), AttributeValue::Unknown(v) => Bytes::from(v.bytes.to_owned()), + AttributeValue::Aigp(_v) => { + // AIGP encoding not yet implemented - return empty bytes + Bytes::new() + } + AttributeValue::AttrSet(_v) => { + // ATTR_SET encoding not yet implemented - return empty bytes + Bytes::new() + } }; match self.is_extended() { diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 81d07aad..3253c23c 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -54,7 +54,16 @@ pub fn parse_bgp_message( ) -> Result { let total_size = data.len(); data.has_n_remaining(19)?; - data.advance(16); + + // Read and validate BGP marker (RFC 4271: 16 bytes of 0xFF) + let mut marker = [0u8; 16]; + data.copy_to_slice(&mut marker); + if marker != [0xFF; 16] { + // Log warning for invalid marker but continue processing + // Some implementations may use non-standard markers in MRT dumps + warn!("BGP message marker is not all 0xFF bytes (invalid per RFC 4271)"); + } + /* This 2-octet unsigned integer indicates the total length of the message, including the header in octets. Thus, it allows one @@ -81,10 +90,12 @@ pub fn parse_bgp_message( ))); } - let bgp_msg_length = if (length as usize) > total_size { - total_size - 19 + // Validate length >= 19 before any arithmetic to prevent underflow + let length_usize = length as usize; + let bgp_msg_length = if length_usize > total_size { + total_size.saturating_sub(19) } else { - length as usize - 19 + length_usize.saturating_sub(19) }; let msg_type: BgpMessageType = match BgpMessageType::try_from(data.read_u8()?) { @@ -442,13 +453,18 @@ pub fn parse_bgp_update_message( let afi = Afi::Ipv4; // parse withdrawn prefixes NLRI - let withdrawn_bytes_length = input.read_u16()? as usize; + let withdrawn_bytes_length_raw = input.read_u16()?; + let withdrawn_bytes_length = withdrawn_bytes_length_raw as usize; input.has_n_remaining(withdrawn_bytes_length)?; let withdrawn_bytes = input.split_to(withdrawn_bytes_length); let withdrawn_prefixes = read_nlri(withdrawn_bytes, &afi, add_path)?; // parse attributes - let attribute_length = input.read_u16()? as usize; + let attribute_length_raw = input.read_u16()?; + // Defensive check: ensure attribute_length fits in usize + // u16 to usize conversion is always safe on 32/64-bit platforms, + // but this check ensures safety on all architectures + let attribute_length = attribute_length_raw as usize; input.has_n_remaining(attribute_length)?; let attr_data_slice = input.split_to(attribute_length); @@ -528,12 +544,13 @@ impl BgpUpdateMessage { } impl BgpMessage { + /// BGP marker value: 16 bytes of 0xFF (RFC 4271) + const MARKER: [u8; 16] = [0xFF; 16]; + pub fn encode(&self, asn_len: AsnLength) -> Bytes { let mut bytes = BytesMut::new(); - bytes.put_u32(0); // marker - bytes.put_u32(0); // marker - bytes.put_u32(0); // marker - bytes.put_u32(0); // marker + // RFC 4271: Marker is 16 bytes of 0xFF + bytes.put_slice(&Self::MARKER); let (msg_type, msg_bytes) = match self { BgpMessage::Open(msg) => (BgpMessageType::OPEN, msg.encode()), @@ -715,10 +732,10 @@ mod tests { #[test] fn test_invlaid_type() { let bytes = Bytes::from_static(&[ - 0x00, 0x00, 0x00, 0x00, // marker - 0x00, 0x00, 0x00, 0x00, // marker - 0x00, 0x00, 0x00, 0x00, // marker - 0x00, 0x00, 0x00, 0x00, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker (valid RFC 4271) + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker 0x00, 0x28, // length 0x05, // type ]); @@ -726,6 +743,124 @@ mod tests { assert!(parse_bgp_message(&mut data, false, &AsnLength::Bits16).is_err()); } + #[test] + fn test_bgp_message_length_underflow_protection() { + // Test that length values less than 19 are properly rejected + // without causing arithmetic underflow + for len in [0u16, 1, 18] { + let bytes = Bytes::from(vec![ + 0xFF, + 0xFF, + 0xFF, + 0xFF, // marker + 0xFF, + 0xFF, + 0xFF, + 0xFF, // marker + 0xFF, + 0xFF, + 0xFF, + 0xFF, // marker + 0xFF, + 0xFF, + 0xFF, + 0xFF, // marker + (len >> 8) as u8, + (len & 0xFF) as u8, // length field + 0x01, // type = OPEN + ]); + let mut data = bytes.clone(); + let result = parse_bgp_message(&mut data, false, &AsnLength::Bits16); + assert!( + result.is_err(), + "Length {} should be rejected as invalid", + len + ); + } + } + + #[test] + fn test_bgp_marker_encoding_rfc4271() { + // Test that BgpMessage::encode produces correct RFC 4271 marker (all 0xFF) + let msg = BgpMessage::KeepAlive; + let encoded = msg.encode(AsnLength::Bits16); + + // First 16 bytes should be all 0xFF + assert_eq!( + &encoded[..16], + &[0xFF; 16], + "BGP marker should be all 0xFF bytes" + ); + } + + #[test] + fn test_bgp_marker_validation() { + // Test that message with valid marker (all 0xFF) parses correctly + let valid_bytes = Bytes::from(vec![ + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0xFF, 0xFF, 0xFF, 0xFF, // marker + 0x00, 0x13, // length = 19 (minimum) + 0x04, // type = KEEPALIVE + ]); + let mut data = valid_bytes.clone(); + let result = parse_bgp_message(&mut data, false, &AsnLength::Bits16); + assert!(result.is_ok(), "Valid marker should parse successfully"); + + // Test that message with invalid marker (all zeros) is handled + // Parser should warn but still process (for MRT compatibility) + let invalid_bytes = Bytes::from(vec![ + 0x00, 0x00, 0x00, 0x00, // marker (invalid - should be 0xFF) + 0x00, 0x00, 0x00, 0x00, // marker + 0x00, 0x00, 0x00, 0x00, // marker + 0x00, 0x00, 0x00, 0x00, // marker + 0x00, 0x13, // length = 19 + 0x04, // type = KEEPALIVE + ]); + let mut data = invalid_bytes.clone(); + // Should still parse (with warning) for compatibility + let result = parse_bgp_message(&mut data, false, &AsnLength::Bits16); + assert!( + result.is_ok(), + "Invalid marker should still parse (with warning)" + ); + } + + #[test] + fn test_attribute_length_overflow_protection() { + // Test that large attribute length values are handled correctly + // without causing overflow issues + + // Create a BGP UPDATE message with attribute_length that exceeds available data + let update_bytes = Bytes::from(vec![ + 0x00, 0x00, // withdrawn length = 0 + 0xFF, + 0xFF, // attribute length = 65535 (largest u16, but not enough data) + // No actual attribute data follows + ]); + + let result = parse_bgp_update_message(update_bytes, false, &AsnLength::Bits16); + assert!( + result.is_err(), + "Should fail when attribute_length exceeds available data" + ); + assert!( + matches!(result, Err(ParserError::TruncatedMsg(_))), + "Should fail with TruncatedMsg error" + ); + + // Test valid attribute length parsing + let valid_update = Bytes::from(vec![ + 0x00, 0x00, // withdrawn length = 0 + 0x00, + 0x00, // attribute length = 0 + // No attributes, valid empty UPDATE + ]); + let result = parse_bgp_update_message(valid_update, false, &AsnLength::Bits16); + assert!(result.is_ok(), "Should parse valid empty UPDATE"); + } + #[test] fn test_parse_bgp_notification_message() { let bytes = Bytes::from_static(&[ @@ -799,11 +934,16 @@ mod tests { data: vec![0x00, 0x00], }); let bytes = bgp_message.encode(AsnLength::Bits16); + // RFC 4271: Marker is 16 bytes of 0xFF assert_eq!( bytes, Bytes::from_static(&[ - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x17, 0x03, 0x01, 0x02, 0x00, 0x00 + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, // marker (8 bytes) + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, // marker (8 bytes) + 0x00, 0x17, // length = 23 (16 marker + 2 len + 1 type + 4 msg) + 0x03, // type = NOTIFICATION + 0x01, 0x02, // error code, subcode + 0x00, 0x00 // data ]) ); } diff --git a/src/parser/mrt/mrt_elem.rs b/src/parser/mrt/mrt_elem.rs index 2f1b2954..be405cf5 100644 --- a/src/parser/mrt/mrt_elem.rs +++ b/src/parser/mrt/mrt_elem.rs @@ -147,7 +147,9 @@ fn get_relevant_attributes( | AttributeValue::Clusters(_) | AttributeValue::Development(_) | AttributeValue::LinkState(_) - | AttributeValue::TunnelEncapsulation(_) => {} + | AttributeValue::TunnelEncapsulation(_) + | AttributeValue::Aigp(_) + | AttributeValue::AttrSet(_) => {} }; } diff --git a/src/parser/utils.rs b/src/parser/utils.rs index a865770b..414d1382 100644 --- a/src/parser/utils.rs +++ b/src/parser/utils.rs @@ -165,6 +165,20 @@ pub trait ReadUtils: Buf { // Length in bits let bit_len = self.read_u8()?; + // Validate prefix length immediately after reading + // IPv4 prefix length must be <= 32, IPv6 <= 128 + let max_bit_len: u8 = match afi { + Afi::Ipv4 => 32, + Afi::Ipv6 => 128, + Afi::LinkState => 0, // Link-State doesn't use traditional prefixes + }; + + if bit_len > max_bit_len { + return Err(ParserError::ParseError(format!( + "Invalid prefix length: {bit_len} (max {max_bit_len} for {afi:?})" + ))); + } + // Convert to bytes let byte_len: usize = (bit_len as usize).div_ceil(8); let addr: IpAddr = match afi { @@ -596,6 +610,64 @@ mod tests { assert_eq!(buf.read_nlri_prefix(&Afi::Ipv4, true).unwrap(), expected); } + #[test] + fn test_read_nlri_prefix_length_validation() { + // Test IPv4 prefix length > 32 (e.g., 33, 255) is rejected + for invalid_len in [33u8, 255] { + let mut buf = Bytes::from(vec![invalid_len, 0xC0, 0xA8, 0x01]); + let result = buf.read_nlri_prefix(&Afi::Ipv4, false); + assert!( + result.is_err(), + "IPv4 prefix length {} should be rejected", + invalid_len + ); + if let Err(ParserError::ParseError(msg)) = result { + assert!(msg.contains("Invalid prefix length")); + } + } + + // Test IPv6 prefix length > 128 (e.g., 129, 255) is rejected + for invalid_len in [129u8, 255] { + let mut buf = Bytes::from(vec![invalid_len, 0x20, 0x01, 0x0D, 0xB8]); + let result = buf.read_nlri_prefix(&Afi::Ipv6, false); + assert!( + result.is_err(), + "IPv6 prefix length {} should be rejected", + invalid_len + ); + if let Err(ParserError::ParseError(msg)) = result { + assert!(msg.contains("Invalid prefix length")); + } + } + + // Test valid IPv4 prefix lengths pass (0-32) + for valid_len in [0u8, 1, 32] { + // Provide enough bytes for any byte_len calculation + let mut buf = Bytes::from(vec![valid_len, 0x00, 0x00, 0x00, 0x00]); + let result = buf.read_nlri_prefix(&Afi::Ipv4, false); + assert!( + result.is_ok(), + "IPv4 prefix length {} should be valid", + valid_len + ); + } + + // Test valid IPv6 prefix lengths pass (0-128) + for valid_len in [0u8, 1, 64, 128] { + // Provide enough bytes for any byte_len calculation (max 16 bytes) + let buf_data: Vec = std::iter::once(valid_len) + .chain(std::iter::repeat_n(0x00, 16)) + .collect(); + let mut buf = Bytes::from(buf_data); + let result = buf.read_nlri_prefix(&Afi::Ipv6, false); + assert!( + result.is_ok(), + "IPv6 prefix length {} should be valid", + valid_len + ); + } + } + #[test] fn test_encode_asn() { let asn = Asn::new_32bit(1);