From 988a0c69faa46c793ca33c50e4ec2654991f9937 Mon Sep 17 00:00:00 2001 From: Mingwei Zhang Date: Wed, 8 Dec 2021 17:29:49 -0800 Subject: [PATCH] refactor NLRI parsing for add-path issue This refactor enables parsing of NLRI block with potentially wrong add-path type. In some cases, the BGP message with add-path might be wrapped in a non-add-path message and causing trouble parsing NLRI block in BGP message and in attributes. This patch implements a workaround that can more intelligently "guess" the add-path intention and revert back to regular parsing when guess fails. Here is a brief description of the workaround: - If add-path is not set (i.e. not the add-path BGP msgs) and the first byte of an NLRI entry is 0, it is likely an add-path msg wrapped in non-add-path msg, treat as add-path. This covers both IPv4 and IPv6 cases. - If an error happens when parsing, revert back to the original add-path setting (this only happens for `0.0.0.0/0` or `0::/0` prefixes without add-path, which is pretty rare) and re-parse the whole NLRI block. (needs to go back a few bytes to the beginning). - The unhandled cases are the ones where the first byte of path_id is not zero. In those cases, we will, unfortunately, parse the msg without path-id, and depending on the msg, we might see parsing errors later (e.g. not enough bytes, or extra bytes). Test run show that the parsing of one problematic file is resolved back to normal now. See https://github.com/bgpkit/bgpkit-parser/issues/31 for more details. --- src/parser/bgp/attributes.rs | 35 ++++------ src/parser/bgp/messages.rs | 45 +++++++++---- .../mrt/messages/table_dump_v2_message.rs | 2 +- src/parser/utils.rs | 66 ++++++++++++++++++- 4 files changed, 109 insertions(+), 39 deletions(-) diff --git a/src/parser/bgp/attributes.rs b/src/parser/bgp/attributes.rs index 9e50dc1..0c7afcf 100644 --- a/src/parser/bgp/attributes.rs +++ b/src/parser/bgp/attributes.rs @@ -4,13 +4,13 @@ use std::convert::TryFrom; use bgp_models::bgp::attributes::*; use bgp_models::bgp::community::*; use bgp_models::network::*; -use log::warn; +use log::{warn,debug}; use byteorder::{BigEndian, ReadBytesExt}; use num_traits::FromPrimitive; -use crate::parser::ReadUtils; +use crate::parser::{parse_nlri_list, ReadUtils}; use crate::error::ParserErrorKind; @@ -257,6 +257,7 @@ impl AttributeParser { } /// + /// /// The attribute is encoded as shown below: /// +---------------------------------------------------------+ /// | Address Family Identifier (2 octets) | @@ -322,9 +323,11 @@ impl AttributeParser { if first_byte_zero { if reachable { // skip reserved byte for reachable NRLI - let _should_be_zero = buf_input.read_u8()?; + if buf_input.read_u8()? !=0 { + warn!("NRLI reserved byte not 0"); + } } - self.parse_mp_prefix_list(&mut buf_input, &afi)? + parse_nlri_list(&mut buf_input, self.additional_paths, &afi)? } else { pfxs.to_owned() } @@ -332,9 +335,11 @@ impl AttributeParser { None => { if reachable { // skip reserved byte for reachable NRLI - buf_input.read_u8()?; + if buf_input.read_u8()? !=0 { + warn!("NRLI reserved byte not 0"); + } } - self.parse_mp_prefix_list(&mut buf_input, &afi)? + parse_nlri_list(&mut buf_input, self.additional_paths, &afi)? } }; @@ -367,24 +372,6 @@ impl AttributeParser { Ok(output) } - fn parse_mp_prefix_list( - &self, - input: &mut Take, - afi: &Afi, - ) -> Result, ParserErrorKind> { - let mut output = Vec::new(); - while input.limit() > 0 { - let path_id = if self.additional_paths { - input.read_u32::() - } else { - Ok(0) - }?; - let prefix = input.read_nlri_prefix(afi, path_id)?; - output.push(prefix); - } - Ok(output) - } - fn parse_large_communities( &self, input: &mut Take, diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 7e2115b..d53d66f 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -1,11 +1,13 @@ -use std::io::Read; +use std::io::{Read, Take}; use bgp_models::bgp::*; use bgp_models::network::*; +use byteorder::ReadBytesExt; use num_traits::FromPrimitive; use crate::error::ParserErrorKind; -use crate::parser::{AttributeParser, ReadUtils}; +use crate::parser::{AttributeParser, parse_nlri_list, ReadUtils}; +use log::warn; /// BGP message /// @@ -122,25 +124,46 @@ pub fn parse_bgp_open_message(input: &mut T, ) -> Result(input: &mut Take, length: usize, afi: &Afi, add_path: bool) -> Result, ParserErrorKind> { + if length==0{ + return Ok(vec![]) + } + if length==1 { + // 1 byte does not make sense + warn!("seeing strange one-byte NLRI field"); + input.read_u8().unwrap(); + return Ok(vec![]) + } + + let mut buf=Vec::with_capacity(length); + input.read_to_end(&mut buf)?; + + let mut nlri_input = buf.as_slice().take(length as u64); + + let prefixes = parse_nlri_list(&mut nlri_input, add_path, &afi)?; + + Ok(prefixes) +} + +/// read bgp update message. pub fn parse_bgp_update_message(input: &mut T, add_path:bool, afi: &Afi, asn_len: &AsnLength, bgp_msg_length: u64) -> Result { + // parse withdrawn prefixes nlri let withdrawn_length = input.read_16b()? as u64; - let mut withdarwn_input = input.take(withdrawn_length); - let mut withdrawn_prefixes = vec!(); - while withdarwn_input.limit()>0 { - withdrawn_prefixes.push(withdarwn_input.read_nlri_prefix(afi, 0)?); - } + let mut withdrawn_input = input.take(withdrawn_length); + let withdrawn_prefixes = read_nlri(&mut withdrawn_input, withdrawn_length as usize, afi, add_path)?; + // parse attributes let attribute_length = input.read_16b()? as u64; let attr_parser = AttributeParser::new(add_path); let mut attr_input = input.take(attribute_length); let attributes = attr_parser.parse_attributes(&mut attr_input, asn_len, None, None, None)?; - let mut announced_prefixes = vec!(); + // parse announced prefixes nlri let nlri_length = bgp_msg_length - 4 - withdrawn_length - attribute_length; let mut nlri_input = input.take(nlri_length); - while nlri_input.limit()>0 { - announced_prefixes.push(nlri_input.read_nlri_prefix(afi, 0)?); - } + let announced_prefixes = read_nlri(&mut nlri_input, nlri_length as usize, afi, add_path)?; + Ok( BgpUpdateMessage{ withdrawn_prefixes, diff --git a/src/parser/mrt/messages/table_dump_v2_message.rs b/src/parser/mrt/messages/table_dump_v2_message.rs index 1593caa..66f0662 100644 --- a/src/parser/mrt/messages/table_dump_v2_message.rs +++ b/src/parser/mrt/messages/table_dump_v2_message.rs @@ -123,7 +123,7 @@ pub fn parse_rib_afi_entries(input: &mut Take, rib_type: Ta let sequence_number = input.read_32b()?; - let prefix = input.read_nlri_prefix(&afi, 0)?; + let prefix = input.read_nlri_prefix(&afi, add_path)?; let prefixes = vec!(prefix.clone()); let entry_count = input.read_16b()?; diff --git a/src/parser/utils.rs b/src/parser/utils.rs index 85a7fab..a0d7274 100644 --- a/src/parser/utils.rs +++ b/src/parser/utils.rs @@ -10,8 +10,9 @@ use std::{ use num_traits::FromPrimitive; use std::net::IpAddr; -use std::io::Read; +use std::io::{Read, Take}; use bgp_models::network::{Afi, Asn, AsnLength, NetworkPrefix, Safi}; +use log::warn; use crate::error::ParserErrorKind; @@ -66,10 +67,19 @@ pub trait ReadUtils: io::Read { Ok(()) } - /// Read announced prefix. + /// Read announced/withdrawn prefix. /// /// The length in bits is 1 byte, and then based on the IP version it reads different number of bytes. - fn read_nlri_prefix(&mut self, afi: &Afi, path_id: u32) -> Result { + /// If the `add_path` is true, it will also first read a 4-byte path id first; otherwise, a path-id of 0 + /// is automatically set. + fn read_nlri_prefix(&mut self, afi: &Afi, add_path: bool) -> Result { + + let path_id = if add_path { + self.read_u32::()? + } else { + 0 + }; + // Length in bits let bit_len = self.read_8b()?; @@ -199,5 +209,55 @@ pub trait ReadUtils: io::Read { } } +pub(crate) fn parse_nlri_list( + input: &mut Take<&[u8]>, + add_path: bool, + afi: &Afi, +) -> Result, ParserErrorKind> { + + let mut bytes_copy = Vec::with_capacity(input.limit() as usize); + input.read_to_end(&mut bytes_copy)?; + + let mut is_add_path = add_path; + let mut prefixes = Vec::new(); + + let mut retry = false; + let mut guessed = false; + + let mut new_input = bytes_copy.take(bytes_copy.len() as u64); + while new_input.limit() > 0 { + if !is_add_path && new_input.get_ref()[0]==0 { + // it's likely that this is a add-path wrongfully wrapped in non-add-path msg + warn!("not add-path but with NLRI size to be 0, likely add-path msg in wrong msg type, treat as add-path now"); + is_add_path = true; + guessed = true; + } + let prefix = match new_input.read_nlri_prefix(afi, is_add_path){ + Ok(p) => {p} + Err(e) => { + if guessed { + retry = true; + break; + } else { + return Err(e); + } + } + }; + prefixes.push(prefix); + } + + if retry { + prefixes.clear(); + // try again without attempt to guess add-path + let mut new_input = bytes_copy.take(bytes_copy.len() as u64); + while new_input.limit() > 0 { + let prefix = new_input.read_nlri_prefix(afi, add_path)?; + prefixes.push(prefix); + } + } + + Ok(prefixes) +} + // All types that implement Read can now read prefixes impl ReadUtils for R {}