From 2c3f5b8c348ab3a4a23b4452a6fcfc32591a54e2 Mon Sep 17 00:00:00 2001 From: Luuk Hendriks Date: Wed, 22 Nov 2023 19:36:23 +0100 Subject: [PATCH] More fixes after fuzzing --- src/bgp/message/nlri.rs | 82 +++++++++++++-- src/bgp/message/update.rs | 163 +++++++++++++++++++++++++++++- src/bgp/message/update_builder.rs | 16 ++- 3 files changed, 242 insertions(+), 19 deletions(-) diff --git a/src/bgp/message/nlri.rs b/src/bgp/message/nlri.rs index 702f757b..6509c718 100644 --- a/src/bgp/message/nlri.rs +++ b/src/bgp/message/nlri.rs @@ -314,12 +314,12 @@ where Octs: AsRef<[u8]>, impl> Eq for Labels { } -impl Labels { - fn len(&self) -> usize { +impl> Labels { + pub fn len(&self) -> usize { self.octets.as_ref().len() } - fn as_ref(&self) -> &[u8] { + pub fn as_ref(&self) -> &[u8] { self.octets.as_ref() } @@ -491,6 +491,20 @@ pub struct MplsVpnNlri { rd: RouteDistinguisher, } +impl MplsVpnNlri { + pub fn basic(&self) -> BasicNlri { + self.basic + } + + pub fn labels(&self) -> &Labels { + &self.labels + } + + pub fn rd(&self) -> RouteDistinguisher { + self.rd + } +} + impl PartialEq> for MplsVpnNlri where Octs: AsRef<[u8]>, Other: AsRef<[u8]> @@ -569,6 +583,12 @@ impl EvpnNlri { } } +impl> EvpnNlri { + fn compose_len(&self) -> usize { + 1 + self.raw.as_ref().len() + } +} + typeenum!( EvpnRouteType, u8, { @@ -938,8 +958,12 @@ impl> Nlri { pub fn compose_len(&self) -> usize { match self { Nlri::Unicast(b) | Nlri::Multicast(b) => b.compose_len(), + Nlri::Mpls(m) => m.compose_len(), + Nlri::MplsVpn(m) => m.compose_len(), + Nlri::Vpls(v) => v.compose_len(), Nlri::FlowSpec(f) => f.compose_len(), - _ => todo!() + Nlri::RouteTarget(r) => r.compose_len(), + Nlri::Evpn(e) => e.compose_len(), } } } @@ -1263,18 +1287,26 @@ impl MplsVpnNlri { let mut prefix_bits = parser.parse_u8()?; let labels = Labels::parse(parser)?; - // Check whether we can safely subtract the labels length from the - // prefix size. If there is an unexpected path id, we might silently - // subtract too much, because there is no 'subtract with overflow' - // warning when built in release mode. - if 8 * labels.len() as u8 > prefix_bits { + // Check whether we can safely subtract both the labels length and the + // route Distinguisher length from the prefix size. If there is an + // unexpected path id, we might silently subtract too much, because + // there is no 'subtract with overflow' warning when built in release + // mode. + + if + u8::try_from(8 * (8 + labels.len())) + .map_err(|_| ParseError::form_error( + "MplsVpnNlri labels/rd too long" + ))? > prefix_bits + { return Err(ParseError::ShortInput); } prefix_bits -= 8 * labels.len() as u8; let rd = RouteDistinguisher::parse(parser)?; - prefix_bits -= 8*std::mem::size_of::() as u8; + + prefix_bits -= 8*8; let prefix = parse_prefix_for_len(parser, prefix_bits, afi)?; @@ -1283,6 +1315,12 @@ impl MplsVpnNlri { } } +impl> MplsVpnNlri { + fn compose_len(&self) -> usize { + self.basic.compose_len() + self.labels.len() + self.rd.bytes.len() + } +} + impl MplsNlri { pub fn check( parser: &mut Parser, @@ -1334,7 +1372,10 @@ impl MplsNlri { // prefix size. If there is an unexpected path id, we might silently // subtract too much, because there is no 'subtract with overflow' // warning when built in release mode. - if 8 * labels.len() as u8 > prefix_bits { + + if u8::try_from(8 * labels.len()) + .map_err(|_| ParseError::form_error("MplsNlri labels too long"))? + > prefix_bits { return Err(ParseError::ShortInput); } @@ -1351,6 +1392,15 @@ impl MplsNlri { } } +impl> MplsNlri { + fn compose_len(&self) -> usize { + self.basic.compose_len() + self.labels.len() + } + pub fn labels(&self) -> &Labels { + &self.labels + } +} + impl VplsNlri { pub fn check(parser: &mut Parser) -> Result<(), ParseError> @@ -1384,6 +1434,10 @@ impl VplsNlri { } ) } + + fn compose_len(&self) -> usize { + 8 + 2 + 2 + 2 + 4 + } } impl FlowSpecNlri { @@ -1521,6 +1575,12 @@ impl RouteTargetNlri { } } +impl> RouteTargetNlri { + fn compose_len(&self) -> usize { + self.raw.as_ref().len() + } +} + impl EvpnNlri { pub fn parse<'a, R>(parser: &mut Parser<'a, R>) -> Result diff --git a/src/bgp/message/update.rs b/src/bgp/message/update.rs index 1729e61a..fa3957a6 100644 --- a/src/bgp/message/update.rs +++ b/src/bgp/message/update.rs @@ -1331,15 +1331,20 @@ mod tests { use super::*; use std::str::FromStr; + use std::net::Ipv6Addr; use crate::bgp::communities::{ StandardCommunity, ExtendedCommunityType, ExtendedCommunitySubType, Tag, Wellknown, }; - use crate::bgp::message::{Message, nlri::PathId}; + use crate::bgp::message::{Message, nlri::{ + Labels, PathId, RouteDistinguisher + }}; use crate::addr::Prefix; + + use bytes::Bytes; #[allow(dead_code)] @@ -1373,11 +1378,11 @@ mod tests { // x chained iter // - MP NLRI types: // announcements: - // - v4 mpls unicast + // x v4 mpls unicast // - v4 mpls unicast unreach **missing** // - v4 mpls vpn unicast // - v6 mpls unicast addpath - // - v6 mpls vpn unicast + // X v6 mpls vpn unicast // - multicast **missing // - vpls // - flowspec @@ -2380,4 +2385,156 @@ mod tests { assert_eq!(update.fmt_pcap_string(), update2.fmt_pcap_string()); } + + #[test] + fn v4_mpls_unicast() { + let raw = vec![ + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0x00, 0x5c, 0x02, 0x00, 0x00, 0x00, 0x45, 0x80, + 0x0e, 0x31, 0x00, 0x01, 0x04, 0x04, 0x0a, 0x07, + 0x08, 0x08, 0x00, 0x38, 0x01, 0xf4, 0x01, 0x0a, + 0x00, 0x00, 0x09, 0x32, 0x01, 0xf4, 0x11, 0xc6, + 0x33, 0x64, 0x00, 0x32, 0x01, 0xf4, 0x21, 0xc6, + 0x33, 0x64, 0x40, 0x32, 0x01, 0xf4, 0x31, 0xc6, + 0x33, 0x64, 0x80, 0x32, 0x01, 0xf4, 0x91, 0xc6, + 0x33, 0x64, 0xc0, 0x40, 0x01, 0x01, 0x00, 0x40, + 0x02, 0x0a, 0x02, 0x02, 0x00, 0x00, 0x01, 0x2c, + 0x00, 0x00, 0x01, 0xf4 + ]; + + let upd = UpdateMessage::from_octets( + &raw, + SessionConfig::modern() + ).unwrap(); + if let Ok(Some(NextHop::Unicast(a))) = upd.mp_next_hop() { + assert_eq!(a, Ipv4Addr::from_str("10.7.8.8").unwrap()); + } else { + panic!("wrong"); + } + let mut ann = upd.mp_announcements().unwrap().unwrap().iter(); + if let Some(Ok(Nlri::Mpls(n1))) = ann.next() { + assert_eq!( + n1.basic().prefix(), + Prefix::from_str("10.0.0.9/32").unwrap() + ); + assert_eq!( + n1.labels().as_ref(), + &[0x01, 0xf4, 0x01] // single label: [2012] + //Labels::from(..), + ); + } else { + panic!("wrong"); + } + + // and 4 more: + assert_eq!(ann.count(), 4); + } + + #[test] + fn v6_mpls_vpn_unicast() { + + // BGP UPDATE for 2/128, one single announced NLRI + let raw = vec![ + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0x00, 0x9a, 0x02, 0x00, 0x00, 0x00, 0x83, 0x80, + 0x0e, 0x39, 0x00, 0x02, 0x80, 0x18, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0xff, 0xff, 0x0a, 0x00, 0x00, 0x02, 0x00, 0xd8, + 0x00, 0x7d, 0xc1, 0x00, 0x00, 0x00, 0x64, 0x00, + 0x00, 0x00, 0x01, 0xfc, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x01, 0x40, 0x01, 0x01, 0x00, 0x40, + 0x02, 0x06, 0x02, 0x01, 0x00, 0x00, 0x00, 0x01, + 0x80, 0x04, 0x04, 0x00, 0x00, 0x00, 0x00, 0x40, + 0x05, 0x04, 0x00, 0x00, 0x00, 0x64, 0xc0, 0x10, + 0x18, 0x00, 0x02, 0x00, 0x64, 0x00, 0x00, 0x00, + 0x01, 0x00, 0x09, 0x00, 0x64, 0x00, 0x00, 0x00, + 0x00, 0x01, 0x0b, 0x0a, 0x00, 0x00, 0x02, 0x00, + 0x01, 0xc0, 0x14, 0x0e, 0x00, 0x01, 0x00, 0x00, + 0x00, 0x64, 0x00, 0x00, 0x00, 0x01, 0x0a, 0x00, + 0x00, 0x02 + ]; + + let upd = UpdateMessage::from_octets( + &raw, + SessionConfig::modern() + ).unwrap(); + if let Ok(Some(NextHop::Ipv6MplsVpnUnicast(rd, a))) = upd.mp_next_hop() { + assert_eq!(rd, RouteDistinguisher::new(&[0; 8])); + assert_eq!(a, Ipv6Addr::from_str("::ffff:10.0.0.2").unwrap()); + } else { + panic!("wrong"); + } + let mut ann = upd.mp_announcements().unwrap().unwrap().iter(); + if let Some(Ok(Nlri::MplsVpn(n1))) = ann.next() { + assert_eq!( + n1.basic().prefix(), + Prefix::from_str("fc00::1/128").unwrap() + ); + assert_eq!( + n1.labels().as_ref(), + &[0x00, 0x7d, 0xc1] // single label: [2012] + //Labels::from([2012]), + ); + assert_eq!( + n1.rd(), + //RouteDistinguisher::from_str("100:1".unwrap()) + RouteDistinguisher::new(&[0, 0, 0, 100, 0, 0, 0, 1]) + ); + } else { + panic!("wrong"); + } + + assert!(ann.next().is_none()); + } + + + + + #[test] + fn debug_fuzz_unknown_afi_safi() { + let raw = vec![ + 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, + 255, 255, 255, 0, 36, 0, 0, 0, 0, 13, 255, 255, 0, 0, 0, 14, 6, 1, + 0, 4, 0, 1, 0, 0 + ]; + + let upd = UpdateMessage::from_octets(&raw, SessionConfig::modern()) + .unwrap(); + + if let Ok(pas) = upd.path_attributes() { + for pa in pas.into_iter() { + dbg!(&pa); + if let Ok(pa) = pa { + let _ = pa.to_owned(); + } + } + } + } + + + #[test] + fn debug_fuzz_assert_flowspecnlri() { + let raw = vec![ + 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, + 255, 255, 255, 0, 36, 0, 0, 0, 0, 13, 255, 255, 0, 0, 0, 15, 6, 0, + 2, 133, 43, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 1, 0, 255, 255, 255, 254 + ]; + + let upd = UpdateMessage::from_octets(&raw, SessionConfig::modern()) + .unwrap(); + + if let Ok(pas) = upd.path_attributes() { + for pa in pas.into_iter() { + dbg!(&pa); + if let Ok(pa) = pa { + let _ = pa.to_owned(); + } + } + } + } } diff --git a/src/bgp/message/update_builder.rs b/src/bgp/message/update_builder.rs index 2221493c..57444d57 100644 --- a/src/bgp/message/update_builder.rs +++ b/src/bgp/message/update_builder.rs @@ -1172,12 +1172,18 @@ impl NextHop { NextHop::Unicast(IpAddr::V4(_)) | NextHop::Multicast(IpAddr::V4(_)) => 4, NextHop::Unicast(IpAddr::V6(_)) | NextHop::Multicast(IpAddr::V6(_)) => 16, NextHop::Ipv6LL(_, _) => 32, - NextHop::Ipv4MplsVpnUnicast(_rd, _ip4) => todo!(), - NextHop::Ipv6MplsVpnUnicast(_rd, _ip6) => todo!(), + NextHop::Ipv4MplsVpnUnicast(_rd, _ip4) => 8 + 4, + NextHop::Ipv6MplsVpnUnicast(_rd, _ip6) => 8 + 16, NextHop::Empty => 0, // FlowSpec - NextHop::Evpn(IpAddr::V4(_)) => todo!(), - NextHop::Evpn(IpAddr::V6(_)) => todo!(), - NextHop::Unimplemented(_afi, _safi) => todo!() + NextHop::Evpn(IpAddr::V4(_)) => 4, + NextHop::Evpn(IpAddr::V6(_)) => 16, + NextHop::Unimplemented(_afi, _safi) => { + warn!( + "unexpected compose_len called on NextHop::Unimplemented \ + returning usize::MAX, this will cause failure." + ); + usize::MAX + } } }