From 39632e3c3801cf21e7d98e01ab3912770b9f9edf Mon Sep 17 00:00:00 2001 From: Rayhaan Jaufeerally Date: Thu, 1 Oct 2026 22:54:37 +0200 Subject: [PATCH] Property tests and fuzz targets for the parsers, and the bugs they found Property tests (run by `cargo test`): - crates/packet/tests/properties.rs: every kind of message, generated, must be written, be as long as wire_len says, and read back equal; and noise, noise behind a valid header, and valid messages with bytes damaged must never do more than return an error. Anything the decoder accepts must be showable and survive being written and read again. - IpPrefix: text round trip, noise as text, `contains` against a bitwise model. - The session's framing: a stream of messages yields the same messages however it is cut into reads. Fuzz targets (fuzz/, cargo-fuzz on nightly, a minute each in CI): raw messages, message bodies behind a valid header, prefix text, the configuration file. Bugs found, each with a named regression test: - A prefix longer than its address family (e.g. an IPv4 NLRI of length 99) was accepted, and showing it panicked. The session logs every route, so one UPDATE from a peer ended the session task. Such a prefix is now malformed, and showing a prefix cannot fail. - Bits past a prefix's length were kept, so the same prefix sent with stray bits was a different key in the RIB. Prefixes now have one form. - An opaque extended community was read as seven octets instead of eight, putting every community after it out of step. - AGGREGATOR could be neither read from nor written to a peer with two-octet AS numbers. - The Outbound Route Filter and Long-Lived Graceful Restart capabilities were written differently from how they are read. - A capability value or unknown attribute too long for its length field was written with the length wrapped around; it is now an error, or for the attribute the two-octet length. Also: ROUTE-REFRESH carries its AFI and SAFI instead of being modelled as an empty message, and one with a body of the wrong length is refused. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/ci.yml | 22 ++ Cargo.lock | 150 +++++++- Cargo.toml | 3 + README.md | 11 + crates/packet/Cargo.toml | 1 + crates/packet/src/message.rs | 205 +++++++++-- crates/packet/src/open.rs | 98 ++++- crates/packet/tests/properties.rs | 568 +++++++++++++++++++++++++++++ crates/primitives/Cargo.toml | 3 + crates/primitives/src/constants.rs | 10 + crates/primitives/src/ip_prefix.rs | 148 +++++--- crates/server/Cargo.toml | 3 + crates/server/src/connection.rs | 60 +++ crates/server/src/fsm.rs | 7 +- crates/server/src/metrics.rs | 2 +- fuzz/.gitignore | 6 + fuzz/Cargo.toml | 54 +++ fuzz/README.md | 47 +++ fuzz/fuzz_targets/config.rs | 13 + fuzz/fuzz_targets/message.rs | 12 + fuzz/fuzz_targets/message_body.rs | 24 ++ fuzz/fuzz_targets/prefix_text.rs | 12 + fuzz/seeds/config/00 | 38 ++ fuzz/seeds/message/00 | Bin 0 -> 56 bytes fuzz/seeds/message/01 | Bin 0 -> 56 bytes fuzz/seeds/message/02 | Bin 0 -> 86 bytes fuzz/seeds/message/03 | Bin 0 -> 86 bytes fuzz/seeds/message/04 | Bin 0 -> 95 bytes fuzz/seeds/message/05 | Bin 0 -> 95 bytes fuzz/seeds/message/06 | Bin 0 -> 80 bytes fuzz/seeds/message/07 | Bin 0 -> 80 bytes fuzz/seeds/message/08 | Bin 0 -> 24 bytes fuzz/seeds/message/09 | Bin 0 -> 24 bytes fuzz/seeds/message/10 | Bin 0 -> 25 bytes fuzz/seeds/message/11 | Bin 0 -> 25 bytes fuzz/seeds/message/12 | Bin 0 -> 20 bytes fuzz/seeds/message/13 | Bin 0 -> 20 bytes fuzz/seeds/message/14 | Bin 0 -> 24 bytes fuzz/seeds/message/15 | Bin 0 -> 24 bytes fuzz/seeds/message_body/00 | Bin 0 -> 37 bytes fuzz/seeds/message_body/01 | Bin 0 -> 37 bytes fuzz/seeds/message_body/02 | Bin 0 -> 67 bytes fuzz/seeds/message_body/03 | Bin 0 -> 67 bytes fuzz/seeds/message_body/04 | Bin 0 -> 76 bytes fuzz/seeds/message_body/05 | Bin 0 -> 76 bytes fuzz/seeds/message_body/06 | Bin 0 -> 61 bytes fuzz/seeds/message_body/07 | Bin 0 -> 61 bytes fuzz/seeds/message_body/08 | Bin 0 -> 5 bytes fuzz/seeds/message_body/09 | Bin 0 -> 5 bytes fuzz/seeds/message_body/10 | 1 + fuzz/seeds/message_body/11 | 1 + fuzz/seeds/message_body/12 | 1 + fuzz/seeds/message_body/13 | 1 + fuzz/seeds/message_body/14 | Bin 0 -> 5 bytes fuzz/seeds/message_body/15 | Bin 0 -> 5 bytes fuzz/seeds/prefix_text/00 | 1 + fuzz/seeds/prefix_text/01 | 1 + fuzz/seeds/prefix_text/02 | 1 + fuzz/seeds/prefix_text/03 | 1 + fuzz/seeds/prefix_text/04 | 1 + fuzz/seeds/prefix_text/05 | 1 + fuzz/src/lib.rs | 33 ++ 62 files changed, 1437 insertions(+), 103 deletions(-) create mode 100644 crates/packet/tests/properties.rs create mode 100644 fuzz/.gitignore create mode 100644 fuzz/Cargo.toml create mode 100644 fuzz/README.md create mode 100644 fuzz/fuzz_targets/config.rs create mode 100644 fuzz/fuzz_targets/message.rs create mode 100644 fuzz/fuzz_targets/message_body.rs create mode 100644 fuzz/fuzz_targets/prefix_text.rs create mode 100644 fuzz/seeds/config/00 create mode 100644 fuzz/seeds/message/00 create mode 100644 fuzz/seeds/message/01 create mode 100644 fuzz/seeds/message/02 create mode 100644 fuzz/seeds/message/03 create mode 100644 fuzz/seeds/message/04 create mode 100644 fuzz/seeds/message/05 create mode 100644 fuzz/seeds/message/06 create mode 100644 fuzz/seeds/message/07 create mode 100644 fuzz/seeds/message/08 create mode 100644 fuzz/seeds/message/09 create mode 100644 fuzz/seeds/message/10 create mode 100644 fuzz/seeds/message/11 create mode 100644 fuzz/seeds/message/12 create mode 100644 fuzz/seeds/message/13 create mode 100644 fuzz/seeds/message/14 create mode 100644 fuzz/seeds/message/15 create mode 100644 fuzz/seeds/message_body/00 create mode 100644 fuzz/seeds/message_body/01 create mode 100644 fuzz/seeds/message_body/02 create mode 100644 fuzz/seeds/message_body/03 create mode 100644 fuzz/seeds/message_body/04 create mode 100644 fuzz/seeds/message_body/05 create mode 100644 fuzz/seeds/message_body/06 create mode 100644 fuzz/seeds/message_body/07 create mode 100644 fuzz/seeds/message_body/08 create mode 100644 fuzz/seeds/message_body/09 create mode 100644 fuzz/seeds/message_body/10 create mode 100644 fuzz/seeds/message_body/11 create mode 100644 fuzz/seeds/message_body/12 create mode 100644 fuzz/seeds/message_body/13 create mode 100644 fuzz/seeds/message_body/14 create mode 100644 fuzz/seeds/message_body/15 create mode 100644 fuzz/seeds/prefix_text/00 create mode 100644 fuzz/seeds/prefix_text/01 create mode 100644 fuzz/seeds/prefix_text/02 create mode 100644 fuzz/seeds/prefix_text/03 create mode 100644 fuzz/seeds/prefix_text/04 create mode 100644 fuzz/seeds/prefix_text/05 create mode 100644 fuzz/src/lib.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0cb85dd..145573d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,3 +26,25 @@ jobs: run: cargo clippy --workspace --all-targets -- -D warnings - name: Test run: cargo test --workspace + + # A short run of each fuzz target: enough to notice a parser change that + # breaks on input the seeds are close to. Long runs are done by hand; see + # fuzz/README.md. + fuzz: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@nightly + - name: Install cargo-fuzz + run: cargo install cargo-fuzz --locked + - name: Fuzz + working-directory: fuzz + # cargo-fuzz sets its own flags; the workspace's -D warnings would + # replace them. + env: + RUSTFLAGS: "" + run: | + for target in message message_body prefix_text config; do + mkdir -p "corpus/$target" + cargo fuzz run "$target" "corpus/$target" "seeds/$target" -- -max_total_time=60 + done diff --git a/Cargo.lock b/Cargo.lock index 9605e44..6e6368d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -182,6 +182,7 @@ dependencies = [ "bytes", "eyre", "nom", + "proptest", "serde", "serde_repr", "strum", @@ -195,6 +196,7 @@ dependencies = [ "bitfield", "bytes", "nom", + "proptest", "serde", "serde_repr", "strum", @@ -224,6 +226,7 @@ dependencies = [ "clap", "futures", "prometheus", + "proptest", "protobuf", "protobuf-codegen", "thiserror 2.0.12", @@ -248,6 +251,21 @@ dependencies = [ "tracing", ] +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitfield" version = "0.19.0" @@ -270,9 +288,9 @@ dependencies = [ [[package]] name = "bitflags" -version = "2.9.0" +version = "2.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c8214115b7bf84099f1309324e63141d4c5d7cc26862f97a0a857dbefe165bd" +checksum = "3ded4057c258ba199e2d26386d3af3780957ecaee6c4ef4041c6b4b8b97c0b06" [[package]] name = "bumpalo" @@ -863,6 +881,15 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "num-traits" +version = "0.2.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" +dependencies = [ + "autocfg", +] + [[package]] name = "once_cell" version = "1.21.1" @@ -919,6 +946,15 @@ dependencies = [ "zerovec", ] +[[package]] +name = "ppv-lite86" +version = "0.2.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85eae3c4ed2f50dcfe72643da4befc30deadb458a9b590d720cde2f2b1e97da9" +dependencies = [ + "zerocopy", +] + [[package]] name = "proc-macro2" version = "1.0.94" @@ -943,6 +979,25 @@ dependencies = [ "thiserror 2.0.12", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bit-set", + "bit-vec", + "bitflags", + "num-traits", + "rand", + "rand_chacha", + "rand_xorshift", + "regex-syntax", + "rusty-fork", + "tempfile", + "unarray", +] + [[package]] name = "protobuf" version = "3.7.2" @@ -994,6 +1049,12 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quote" version = "1.0.47" @@ -1009,6 +1070,44 @@ version = "5.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" +[[package]] +name = "rand" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +dependencies = [ + "rand_chacha", + "rand_core", +] + +[[package]] +name = "rand_chacha" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" +dependencies = [ + "ppv-lite86", + "rand_core", +] + +[[package]] +name = "rand_core" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76afc826de14238e6e8c374ddcc1fa19e374fd8dd986b0d2af0d02377261d83c" +dependencies = [ + "getrandom", +] + +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core", +] + [[package]] name = "redox_syscall" version = "0.5.10" @@ -1110,6 +1209,18 @@ version = "1.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eded382c5f5f786b989652c49544c4877d9f015cc22e145a5ea8ea66c2921cd2" +[[package]] +name = "rusty-fork" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc6bf79ff24e648f6da1f8d1f011e9cac26491b619e6b9280f2b47f1774e6ee2" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.23" @@ -1554,6 +1665,12 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicode-ident" version = "1.0.18" @@ -1590,6 +1707,15 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "want" version = "0.3.1" @@ -1814,6 +1940,26 @@ dependencies = [ "synstructure", ] +[[package]] +name = "zerocopy" +version = "0.8.59" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6df92bf3d9227be3d53173901ddbffac2babc27ae50f397776ffd6dc33f800cb" +dependencies = [ + "zerocopy-derive", +] + +[[package]] +name = "zerocopy-derive" +version = "0.8.59" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac4f328cf2f05d084e496c3e9c3f33ed0a183656a16e1fcec4d464d8373aec82" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.100", +] + [[package]] name = "zerofrom" version = "0.1.8" diff --git a/Cargo.toml b/Cargo.toml index 9feec9f..ebb418f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,8 @@ [workspace] default-members = ["bin/bgp"] members = ["bin/*", "crates/*"] +# Fuzz targets need a nightly toolchain; see fuzz/README.md. +exclude = ["fuzz"] resolver = "3" [workspace.package] @@ -46,6 +48,7 @@ strum = { version = "0.27.1", features = ["derive"] } tracing = "0.1.44" tracing-subscriber = { version = "0.3.22", features = ["env-filter"] } prometheus = "0.14.0" +proptest = "1.7" protobuf = { package = "protobuf", version = "3.7.2" } protobuf-codegen = { package = "protobuf-codegen", version = "3.7.2" } clap = { version = "4.5.53", features = ["derive"] } diff --git a/README.md b/README.md index 1cbea8b..38d3d7a 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,17 @@ cargo test --workspace Lints are configured once, under `[workspace.lints]` in the root `Cargo.toml`. `unsafe` code is forbidden throughout. +`cargo test` includes property tests: generated messages must survive being +written and read back, and no input, however damaged, may make a parser +panic. They take about a second; for a longer hunt: + +``` +PROPTEST_CASES=1000000 cargo test --release -p bgp-packet --test properties +``` + +Coverage-guided fuzz targets for the same parsers are in `fuzz/`; see +`fuzz/README.md`. CI runs each of them for a minute. + ## License Apache License 2.0; see `LICENSE`. diff --git a/crates/packet/Cargo.toml b/crates/packet/Cargo.toml index fb4e60b..dfa1727 100644 --- a/crates/packet/Cargo.toml +++ b/crates/packet/Cargo.toml @@ -20,6 +20,7 @@ bgp-primitives.workspace = true [dev-dependencies] eyre.workspace = true +proptest.workspace = true [lints] workspace = true diff --git a/crates/packet/src/message.rs b/crates/packet/src/message.rs index 08eaa51..3083cbb 100644 --- a/crates/packet/src/message.rs +++ b/crates/packet/src/message.rs @@ -45,7 +45,7 @@ pub const BGP_MAX_MSG_LEN: u16 = 4096; const BGP_MARKER: [u8; 16] = [0xFF; 16]; /// Message represents the top-level messages in the BGP protocol. -#[derive(Debug, Serialize, Deserialize, EnumDiscriminants)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, EnumDiscriminants)] #[repr(u8)] pub enum Message { Open(OpenMessage) = 1, @@ -53,7 +53,7 @@ pub enum Message { Notification(NotificationMessage) = 3, KeepAlive = 4, /// BGP Route Refresh message (RFC 2918). - RouteRefresh = 5, + RouteRefresh(RouteRefreshMessage) = 5, } impl Message { @@ -148,7 +148,14 @@ impl Message { .map_err(|e| bad_length(&format!("malformed NOTIFICATION ({})", describe(&e)))), 4 if body.is_empty() => Ok(Self::KeepAlive), 4 => Err(bad_length("KEEPALIVE with a body")), - 5 => Ok(Self::RouteRefresh), + 5 => match body { + [afi_hi, afi_lo, subtype, safi] => Ok(Self::RouteRefresh(RouteRefreshMessage { + afi: Afi(u16::from_be_bytes([*afi_hi, *afi_lo])), + subtype: *subtype, + safi: Safi(*safi), + })), + _ => Err(bad_length("ROUTE-REFRESH")), + }, other => Err(header_error( MsgHeaderSubcode::BadMessageType, vec![other], @@ -181,7 +188,12 @@ impl Message { Self::Open(m) => m.to_wire(ctx, &mut body)?, Self::Update(m) => m.to_wire(ctx, &mut body)?, Self::Notification(m) => m.to_wire(ctx, &mut body)?, - Self::KeepAlive | Self::RouteRefresh => {} + Self::RouteRefresh(m) => { + body.put_u16(m.afi.0); + body.put_u8(m.subtype); + body.put_u8(m.safi.0); + } + Self::KeepAlive => {} } let total_len = usize::from(BGP_HEADER_LEN) + body.len(); @@ -206,7 +218,8 @@ impl Message { Self::Open(m) => m.wire_len(ctx)?, Self::Update(m) => m.wire_len(ctx)?, Self::Notification(m) => m.wire_len(), - Self::KeepAlive | Self::RouteRefresh => 0, + Self::RouteRefresh(_) => 4, + Self::KeepAlive => 0, }; Ok(BGP_HEADER_LEN + body_len) } @@ -217,11 +230,36 @@ impl Message { Self::Update(_) => 2, Self::Notification(_) => 3, Self::KeepAlive => 4, - Self::RouteRefresh => 5, + Self::RouteRefresh(_) => 5, } } } +/// A request to send again every route of one address family (RFC 2918). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub struct RouteRefreshMessage { + pub afi: Afi, + /// Zero for a plain request. RFC 7313 uses the octet, reserved in + /// RFC 2918, to mark the beginning and end of a refresh. + pub subtype: u8, + pub safi: Safi, +} + +impl RouteRefreshMessage { + /// A plain request for the routes of `family`. + pub fn request(family: AfiSafi) -> Self { + Self { + afi: family.afi, + subtype: 0, + safi: family.safi, + } + } + + pub fn family(&self) -> AfiSafi { + AfiSafi::new(self.afi, self.safi) + } +} + /// Represents a BGP Update message. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct UpdateMessage { @@ -661,12 +699,23 @@ impl PathAttribute { type_code, payload, } => { + // The flags are sent as they came, except that a payload too + // long for a one-octet length gets the two-octet one. + let length = u16::try_from(payload.len()).map_err(|_| EncodeError::TooLong { + what: "path attribute", + len: payload.len(), + max: u16::MAX.into(), + })?; + let mut flags = flags.clone(); + if length > 255 { + flags.set_extended_length(true); + } out.put_u8(flags.0); out.put_u8(*type_code); if flags.extended_length() { - out.put_u16(payload.len() as u16); + out.put_u16(length); } else { - out.put_u8(payload.len() as u8); + out.put_u8(length as u8); } out.put(payload.as_slice()); } @@ -693,16 +742,16 @@ impl PathAttribute { PathAttribute::As4Path(a) => a.wire_len_with_width(true)?, PathAttribute::LargeCommunities(a) => a.wire_len(ctx)?, PathAttribute::UnknownPathAttribute { flags, payload, .. } => { - // An unknown attribute preserves its original extended_length - // flag on re-serialization, so use it verbatim here rather than - // re-deriving it from the payload length. + // An unknown attribute keeps the length width it came with, + // unless its payload needs the wider one. let payload_len = u16::try_from(payload.len()).map_err(|_| EncodeError::TooLong { what: "path attribute", len: payload.len(), max: u16::MAX.into(), })?; - let header_len: u16 = if flags.extended_length() { 4 } else { 3 }; + let extended = flags.extended_length() || payload_len > 255; + let header_len: u16 = if extended { 4 } else { 3 }; return Ok(header_len + payload_len); } }; @@ -1002,17 +1051,21 @@ pub struct AggregatorPathAttribute { } impl AggregatorPathAttribute { + /// The AS number is as wide as the session's AS numbers (RFC 6793), so + /// the attribute is six or eight octets. pub fn from_wire<'a>( ctx: &ParserContext, buf: &'a [u8], ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { - if ctx.four_octet_asn.is_none_or(|val| !val) { - return Err(nom::Err::Failure(BgpParserError::Malformed( - "AggregatorPathAttribute can only be parsed for four_octet_asn enabled peers" - .into(), - ))); - } - let (buf, asn) = be_u32(buf)?; + let (buf, asn) = match ctx.four_octet_asn { + Some(true) => be_u32(buf)?, + Some(false) => be_u16(buf).map(|(buf, asn)| (buf, u32::from(asn)))?, + None => { + return Err(nom::Err::Failure(BgpParserError::Malformed( + "Context must set four_octet_asn before being used".into(), + ))); + } + }; let (buf, ip) = be_u32(buf)?; Ok(( buf, @@ -1023,19 +1076,20 @@ impl AggregatorPathAttribute { )) } + /// On a two-octet session an AS number that does not fit is written as + /// AS_TRANS (RFC 6793 §4.2.2). pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { - if ctx.four_octet_asn.is_none_or(|val| !val) { - return Err(EncodeError::Unencodable( - "AGGREGATOR can only be sent to peers with four-octet AS numbers", - )); + if width(ctx)? { + out.put_u32(self.asn); + } else { + out.put_u16(u16::try_from(self.asn).unwrap_or(AS_TRANS)); } - out.put_u32(self.asn); out.put_u32(self.ip.into()); Ok(()) } - pub fn wire_len(&self, _: &ParserContext) -> Result { - Ok(8) + pub fn wire_len(&self, ctx: &ParserContext) -> Result { + Ok(if width(ctx)? { 8 } else { 6 }) } } @@ -1120,7 +1174,8 @@ pub enum ExtendedCommunity { Opaque { typ: u8, sub_typ: u8, - value: [u8; 5], + /// The six octets after the type and sub-type. + value: [u8; 6], }, /// Route Target Community as specified in Section 4 of RFC4360. RouteTarget { @@ -1203,10 +1258,10 @@ impl ExtendedCommunity { ) } _ => { - let (buf, payload) = nom::bytes::take(5_usize).parse(buf)?; - let value: [u8; 5] = payload.try_into().map_err(|_| { + let (buf, payload) = nom::bytes::take(6_usize).parse(buf)?; + let value: [u8; 6] = payload.try_into().map_err(|_| { Failure(BgpParserError::Malformed( - "Expected exactly 5 bytes from the parser".into(), + "Expected exactly 6 bytes from the parser".into(), )) })?; ( @@ -2027,6 +2082,96 @@ mod tests { ); // Rejects trailing garbage after the NLRI section (a partial prefix). + // Found by the property tests in tests/properties.rs. + + /// A prefix longer than its address family allows was accepted, and + /// showing it (as the session does when it logs a route) panicked. A + /// peer could end the session task with one UPDATE. + #[test] + fn a_prefix_longer_than_an_address_is_malformed() { + let ctx = default_v6_context(); + // No withdrawn routes, no attributes, NLRI of length 99. + let mut body = vec![0, 0, 0, 0, 99]; + body.extend_from_slice(&[0xff; 13]); + assert!(UpdateMessage::from_wire(&ctx, &body).is_err()); + + let mut unreach = vec![0x00, 0x02, 0x01, 129]; + unreach.extend_from_slice(&[0xff; 17]); + assert!(MpUnreachNlriPathAttribute::from_wire(&ctx, &unreach).is_err()); + } + + /// The bits of a prefix past its length mean nothing (RFC 4271 §4.3). + /// They used to be kept, which made 10.0.0.0/8 sent with stray bits a + /// different route from 10.0.0.0/8. + #[test] + fn bits_past_the_prefix_length_are_cleared() { + let ctx = default_v6_context(); + // 10.255.0.0/12 as sent, with the low four bits of the second octet set. + let body = [0, 0, 0, 0, 12, 10, 0xff]; + let (_, update) = UpdateMessage::from_wire(&ctx, &body).unwrap(); + assert_eq!(update.announced_routes, vec![v4_prefix(12, &[10, 0xf0])]); + assert_eq!(update.announced_routes[0].to_string(), "10.240.0.0/12"); + } + + /// An extended community is eight octets whatever its type. The opaque + /// kind was read as seven, which put every community after it out of + /// step. + #[test] + fn an_opaque_extended_community_is_eight_octets() { + let ctx = default_v6_context(); + #[rustfmt::skip] + let wire = [ + 0x03, 0x0c, 1, 2, 3, 4, 5, 6, // opaque + 0x00, 0x02, 0, 0, 0xfd, 0xe8, 0, 100, // route target 65000:100 + ]; + let (rest, parsed) = ExtendedCommunitiesPathAttribute::from_wire(&ctx, &wire).unwrap(); + assert!(rest.is_empty()); + assert_eq!( + parsed.extended_communities, + vec![ + ExtendedCommunity::Opaque { + typ: 0x03, + sub_typ: 0x0c, + value: [1, 2, 3, 4, 5, 6], + }, + ExtendedCommunity::RouteTarget { + typ: 0x00, + sub_typ: 0x02, + global_admin: 65000, + local_admin: 100, + }, + ] + ); + let mut out = BytesMut::new(); + parsed.to_wire(&ctx, &mut out).unwrap(); + assert_eq!(&out[..], &wire[..]); + } + + /// AGGREGATOR carries an AS number as wide as the session's. It could + /// neither be read from nor written to a two-octet peer, so one such + /// attribute ended the session. + #[test] + fn aggregator_is_six_octets_on_a_two_octet_session() { + let two_octet = ParserContext { + four_octet_asn: Some(false), + address_family: None, + }; + let wire = [0xfd, 0xe8, 192, 0, 2, 1]; + let (rest, parsed) = AggregatorPathAttribute::from_wire(&two_octet, &wire).unwrap(); + assert!(rest.is_empty()); + assert_eq!( + parsed, + AggregatorPathAttribute { + asn: 65000, + ip: Ipv4Addr::new(192, 0, 2, 1), + } + ); + let mut out = BytesMut::new(); + parsed.to_wire(&two_octet, &mut out).unwrap(); + assert_eq!(&out[..], &wire[..]); + assert_eq!(parsed.wire_len(&two_octet).unwrap(), 6); + } + /// The reserved octet after the next hop is there for every family. It /// used to be skipped only for IPv6, so an IPv4 MP_REACH_NLRI gained a /// 0.0.0.0/0 route that the peer never sent. diff --git a/crates/packet/src/open.rs b/crates/packet/src/open.rs index 8443a02..8147059 100644 --- a/crates/packet/src/open.rs +++ b/crates/packet/src/open.rs @@ -25,7 +25,7 @@ macro_rules! fail { /// Contains messages related to the BGP Open message, /// i.e. the Open message itself and options / capabilities. -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct OpenMessage { /// Version of the BGP protocol in use. pub version: u8, @@ -149,7 +149,7 @@ impl OpenMessage { } } -#[derive(Debug, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] #[repr(u8)] pub enum OpenOption { Capabilities(Vec) = 2, @@ -231,7 +231,7 @@ pub struct MultiLabelEntry { pub count: u8, } -#[derive(Debug, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)] #[repr(u8)] pub enum Capability { /// MultiProtocol Extension (RFC 2858). @@ -583,7 +583,7 @@ impl Capability { pub fn to_wire(&self, _ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { out.put_u8(self.discriminant()); - out.put_u8(self.payload_len()); + out.put_u8(self.payload_len()?); match self { Capability::MultiProtocol { afi, safi } => { out.put_u16(afi.0); @@ -595,6 +595,8 @@ impl Capability { out.put_u16(afi.0); out.put_u8(0); // Reserved byte. out.put_u8(safi.0); + // payload_len checked that there are few enough to count. + out.put_u8(orfs.len() as u8); for orf in orfs { out.put_u8(orf.r#type.clone().into()); out.put_u8(orf.send_recv.clone() as u8); @@ -677,28 +679,33 @@ impl Capability { } pub fn wire_len(&self, _ctx: &ParserContext) -> Result { - Ok(2 + self.payload_len() as u16) + Ok(2 + u16::from(self.payload_len()?)) } - fn payload_len(&self) -> u8 { - match self { + /// The length of the capability's value, which has to fit the one octet + /// that carries it. + fn payload_len(&self) -> Result { + let len = match self { Capability::MultiProtocol { .. } => 4, Capability::RouteRefresh => 0, - Capability::OutboundRouteFilter { orfs, .. } => 4 + 2 * orfs.len() as u8, - Capability::ExtendedNextHop { entries } => 6 * entries.len() as u8, + Capability::OutboundRouteFilter { orfs, .. } => 5 + 2 * orfs.len(), + Capability::ExtendedNextHop { entries } => 6 * entries.len(), Capability::ExtendedMessage => 0, Capability::BgpSec { .. } => 3, - Capability::MultiLabelCompat { entries } => 4 * entries.len() as u8, + Capability::MultiLabelCompat { entries } => 4 * entries.len(), Capability::BgpRole { .. } => 1, - Capability::GracefulRestart { entries, .. } => 2 + 4 * entries.len() as u8, + Capability::GracefulRestart { entries, .. } => 2 + 4 * entries.len(), Capability::FourByteAsn { .. } => 4, Capability::AddPath { .. } => 4, Capability::EnhancedRouteRefresh => 0, - Capability::LongLivedGracefulRestart(long_lived_graceful_restarts) => { - 8 * long_lived_graceful_restarts.len() as u8 - } - Capability::UnknownCapability { value, .. } => value.len() as u8, - } + Capability::LongLivedGracefulRestart(entries) => 7 * entries.len(), + Capability::UnknownCapability { value, .. } => value.len(), + }; + u8::try_from(len).map_err(|_| EncodeError::TooLong { + what: "capability", + len, + max: u8::MAX.into(), + }) } } @@ -876,12 +883,12 @@ bitfield! { /// Represents an entry in the BGP Graceful Restart capability. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct GracefulRestartEntry { - afi: Afi, - safi: Safi, + pub afi: Afi, + pub safi: Safi, /// Forwarding State (F) bit, which can be used to indicate whether the /// forwarding state for routes that were advertised with the given AFI /// and SAFI has indeed been preserved during the previous BGP restart. - preserve_forwarding: bool, + pub preserve_forwarding: bool, } #[cfg(test)] @@ -1147,6 +1154,59 @@ mod tests { assert!(Capability::from_wire(&ctx, &[0x01, 0x03, 0x00, 0x01, 0x00]).is_err()); } + // Found by the property tests in tests/properties.rs: both were written + // differently from how they are read, so an OPEN of ours carrying one + // would have been malformed. + + #[test] + fn outbound_route_filter_capability_is_written_with_its_count() { + let ctx = ParserContext::default(); + // IPv4 unicast, one ORF: address prefix (64), both directions. + let wire = [0x03, 0x07, 0x00, 0x01, 0x00, 0x01, 0x01, 0x40, 0x03]; + let (rest, cap) = Capability::from_wire(&ctx, &wire).unwrap(); + assert!(rest.is_empty()); + let mut out = BytesMut::new(); + cap.to_wire(&ctx, &mut out).unwrap(); + assert_eq!(&out[..], &wire[..]); + } + + #[test] + fn long_lived_graceful_restart_entries_are_seven_octets() { + let ctx = ParserContext::default(); + // IPv4 unicast, no flags, stale time 3600 s. + let wire = [0x47, 0x07, 0x00, 0x01, 0x01, 0x00, 0x00, 0x0e, 0x10]; + let (rest, cap) = Capability::from_wire(&ctx, &wire).unwrap(); + assert!(rest.is_empty()); + let mut out = BytesMut::new(); + cap.to_wire(&ctx, &mut out).unwrap(); + assert_eq!(&out[..], &wire[..]); + assert_eq!(cap.wire_len(&ctx).unwrap(), 9); + } + + /// A capability's value has a one-octet length. One that does not fit + /// used to be written with the length wrapped around. + #[test] + fn a_capability_too_long_for_its_length_octet_is_refused() { + let ctx = ParserContext::default(); + let entry = ExtendedNextHop { + afi: Afi::IPV4, + safi: Safi::UNICAST, + nh_afi: Afi::IPV6, + }; + let fits = Capability::ExtendedNextHop { + entries: vec![entry.clone(); 42], + }; + let mut out = BytesMut::new(); + fits.to_wire(&ctx, &mut out).unwrap(); + assert_eq!(out.len(), 2 + 252); + + let too_long = Capability::ExtendedNextHop { + entries: vec![entry; 43], + }; + assert!(too_long.wire_len(&ctx).is_err()); + assert!(too_long.to_wire(&ctx, &mut BytesMut::new()).is_err()); + } + #[test] fn capability_parsers_leave_the_following_bytes() { let ctx = ParserContext::default(); diff --git a/crates/packet/tests/properties.rs b/crates/packet/tests/properties.rs new file mode 100644 index 0000000..7117a23 --- /dev/null +++ b/crates/packet/tests/properties.rs @@ -0,0 +1,568 @@ +//! Property tests for the wire codec. +//! +//! Two things are checked for every kind of message: +//! +//! - **Round trip.** A message that can be written can be read back, comes +//! back equal, and is exactly as long as `wire_len` said. +//! - **Hostile input.** Whatever bytes arrive, decoding returns a message or +//! an error: it never panics. What it does accept can be shown, written +//! again, and read back as the same message. +//! +//! The second is what a peer can do to us, so it is run on three kinds of +//! input: plain noise, noise behind a valid header (so it reaches the body +//! parsers), and valid messages with a few bytes damaged (so it reaches deep +//! into them). + +use std::net::{Ipv4Addr, Ipv6Addr}; + +use bgp_packet::errors::BgpError; +use bgp_packet::message::{ + AggregatorPathAttribute, AsPathAttribute, AsPathSegment, AtomicAggregatePathAttribute, + BGP_HEADER_LEN, BGP_MAX_MSG_LEN, CommunitiesPathAttribute, ExtendedCommunitiesPathAttribute, + ExtendedCommunity, LargeCommunitiesPathAttribute, LargeCommunity, LocalPrefPathAttribute, + Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, MultiExitDiscPathAttribute, + NextHopPathAttribute, NlriNextHop, NotificationMessage, OriginPathAttribute, PathAttribute, + PathAttributeFlags, RouteRefreshMessage, UpdateMessage, +}; +use bgp_packet::open::{ + AddPathSendRecv, BgpRole, BgpSecHeader, Capability, ExtendedNextHop, GracefulRestartEntry, + LongLivedGracefulRestart, MultiLabelEntry, OpenMessage, OpenOption, OrfSendRecv, OrfType, + OutboundRouteFilterEntry, +}; +use bgp_primitives::constants::{AddressFamilyId, Afi, Safi}; +use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::parser::{EncodeError, ParserContext}; +use bytes::BytesMut; +use proptest::collection::vec; +use proptest::prelude::*; + +fn ctx(four_octet_asn: bool) -> ParserContext { + ParserContext { + four_octet_asn: Some(four_octet_asn), + address_family: None, + } +} + +// --------------------------------------------------------------------------- +// Generators +// --------------------------------------------------------------------------- + +fn prefix(version: AddressFamilyId) -> impl Strategy { + let max_len = match version { + AddressFamilyId::Ipv4 => 32u8, + AddressFamilyId::Ipv6 => 128, + }; + (0..=max_len, any::<[u8; 16]>()).prop_map(move |(length, bytes)| { + IpPrefix::new(version, bytes.to_vec(), length).expect("16 bytes are enough for any prefix") + }) +} + +fn afi() -> impl Strategy { + any::().prop_map(Afi) +} + +fn safi() -> impl Strategy { + any::().prop_map(Safi) +} + +/// AS numbers that fit the session: any on a four-octet one, two-octet ones +/// otherwise (wider ones are replaced by AS_TRANS on the wire, by design). +fn asn(four_octet_asn: bool) -> BoxedStrategy { + if four_octet_asn { + any::().boxed() + } else { + any::().prop_map(u32::from).boxed() + } +} + +fn as_path(asn: BoxedStrategy) -> impl Strategy { + vec((any::(), vec(asn, 0..6)), 0..4).prop_map(|segments| AsPathAttribute { + segments: segments + .into_iter() + .map(|(ordered, path)| AsPathSegment { ordered, path }) + .collect(), + }) +} + +/// Extended communities are classified by their type octets when read, so +/// they are generated from the eight octets they are on the wire. +fn extended_community() -> impl Strategy { + any::<[u8; 8]>().prop_map(|bytes| { + ExtendedCommunity::from_wire(&ctx(true), &bytes) + .expect("any eight octets are an extended community") + .1 + }) +} + +fn mp_reach() -> impl Strategy { + let ip_safi = prop_oneof![Just(Safi::UNICAST), Just(Safi::MULTICAST)]; + let v4 = ( + ip_safi.clone(), + any::<[u8; 4]>(), + vec(prefix(AddressFamilyId::Ipv4), 0..5), + ) + .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute { + afi: Afi::IPV4, + safi, + next_hop: NlriNextHop::Ipv4(Ipv4Addr::from(next_hop)), + prefixes, + }); + let v6_next_hop = prop_oneof![ + any::<[u8; 16]>().prop_map(|a| NlriNextHop::Ipv6(Ipv6Addr::from(a))), + (any::<[u8; 16]>(), any::<[u8; 16]>()).prop_map(|(global, link_local)| { + NlriNextHop::Ipv6WithLl { + global: Ipv6Addr::from(global), + link_local: Ipv6Addr::from(link_local), + } + }), + ]; + let v6 = ( + ip_safi, + v6_next_hop, + vec(prefix(AddressFamilyId::Ipv6), 0..5), + ) + .prop_map(|(safi, next_hop, prefixes)| MpReachNlriPathAttribute { + afi: Afi::IPV6, + safi, + next_hop, + prefixes, + }); + prop_oneof![v4, v6] +} + +fn mp_unreach() -> impl Strategy { + let of = |version: AddressFamilyId| { + vec(prefix(version), 0..5).prop_map(move |prefixes| MpUnreachNlriPathAttribute { + afi: version.into(), + safi: Safi::UNICAST, + prefixes, + }) + }; + prop_oneof![of(AddressFamilyId::Ipv4), of(AddressFamilyId::Ipv6)] +} + +/// Type codes that have their own variant; an unknown attribute has none of +/// these. +const KNOWN_ATTRIBUTES: [u8; 13] = [1, 2, 3, 4, 5, 6, 7, 8, 14, 15, 16, 17, 32]; + +fn unknown_attribute() -> impl Strategy { + ( + any::().prop_filter("a type code without a variant", |code| { + !KNOWN_ATTRIBUTES.contains(code) + }), + any::(), + vec(any::(), 0..300), + ) + .prop_map(|(type_code, flags, payload)| { + // Bit 4 selects the two-octet length, which a payload this + // long needs. + let flags = PathAttributeFlags(if payload.len() > 255 { + flags | 0x10 + } else { + flags + }); + PathAttribute::UnknownPathAttribute { + flags, + type_code, + payload, + } + }) +} + +fn path_attribute(four_octet_asn: bool) -> impl Strategy { + prop_oneof![ + prop_oneof![ + Just(OriginPathAttribute::IGP), + Just(OriginPathAttribute::EGP), + Just(OriginPathAttribute::INCOMPLETE) + ] + .prop_map(PathAttribute::Origin), + as_path(asn(four_octet_asn)).prop_map(PathAttribute::AsPath), + any::<[u8; 4]>() + .prop_map(|a| PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::from(a)))), + any::().prop_map(|v| PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(v))), + any::().prop_map(|v| PathAttribute::LocalPref(LocalPrefPathAttribute(v))), + Just(PathAttribute::AtomicAggregate( + AtomicAggregatePathAttribute {} + )), + (asn(four_octet_asn), any::<[u8; 4]>()).prop_map(|(asn, ip)| { + PathAttribute::Aggregator(AggregatorPathAttribute { + asn, + ip: Ipv4Addr::from(ip), + }) + }), + vec(any::<(u16, u16)>(), 0..8) + .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))), + mp_reach().prop_map(PathAttribute::MpReachNlri), + mp_unreach().prop_map(PathAttribute::MpUnreachNlri), + vec(extended_community(), 1..6).prop_map(|extended_communities| { + PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute { + extended_communities, + }) + }), + // AS4_PATH is four octets wide on every session. + as_path(any::().boxed()).prop_map(PathAttribute::As4Path), + vec(any::<(u32, u32, u32)>(), 1..6).prop_map(|communities| { + PathAttribute::LargeCommunities(LargeCommunitiesPathAttribute { + communities: communities + .into_iter() + .map(|(global_admin, data_1, data_2)| LargeCommunity { + global_admin, + data_1, + data_2, + }) + .collect(), + }) + }), + unknown_attribute(), + ] +} + +fn update(four_octet_asn: bool) -> impl Strategy { + ( + vec(prefix(AddressFamilyId::Ipv4), 0..5), + vec(path_attribute(four_octet_asn), 0..8), + vec(prefix(AddressFamilyId::Ipv4), 0..5), + ) + .prop_map( + |(withdrawn_routes, path_attributes, announced_routes)| UpdateMessage { + withdrawn_routes, + path_attributes, + announced_routes, + }, + ) +} + +/// Capability codes that have their own variant. +const KNOWN_CAPABILITIES: [u8; 13] = [1, 2, 3, 5, 6, 7, 8, 9, 64, 65, 69, 70, 71]; + +fn capability() -> impl Strategy { + let orf = ( + any::().prop_map(OrfType::from), + prop_oneof![ + Just(OrfSendRecv::Receive), + Just(OrfSendRecv::Send), + Just(OrfSendRecv::Both) + ], + ) + .prop_map(|(r#type, send_recv)| OutboundRouteFilterEntry { r#type, send_recv }); + let add_path = prop_oneof![ + Just(AddPathSendRecv::Receive), + Just(AddPathSendRecv::Send), + Just(AddPathSendRecv::Both) + ]; + prop_oneof![ + (afi(), safi()).prop_map(|(afi, safi)| Capability::MultiProtocol { afi, safi }), + Just(Capability::RouteRefresh), + (afi(), safi(), vec(orf, 0..4)) + .prop_map(|(afi, safi, orfs)| Capability::OutboundRouteFilter { afi, safi, orfs }), + vec((afi(), safi(), afi()), 1..4).prop_map(|entries| Capability::ExtendedNextHop { + entries: entries + .into_iter() + .map(|(afi, safi, nh_afi)| ExtendedNextHop { afi, safi, nh_afi }) + .collect(), + }), + Just(Capability::ExtendedMessage), + (any::(), afi()).prop_map(|(header, afi)| Capability::BgpSec { + header: BgpSecHeader(header), + afi, + }), + vec((afi(), safi(), any::()), 1..4).prop_map(|entries| { + Capability::MultiLabelCompat { + entries: entries + .into_iter() + .map(|(afi, safi, count)| MultiLabelEntry { afi, safi, count }) + .collect(), + } + }), + any::().prop_map(|role| Capability::BgpRole { + role: BgpRole::from(role), + }), + ( + any::(), + 0u16..4096, + vec((afi(), safi(), any::()), 0..4) + ) + .prop_map(|(restart_state, restart_time, entries)| { + Capability::GracefulRestart { + restart_state, + restart_time, + entries: entries + .into_iter() + .map(|(afi, safi, preserve_forwarding)| GracefulRestartEntry { + afi, + safi, + preserve_forwarding, + }) + .collect(), + } + }), + any::().prop_map(|asn| Capability::FourByteAsn { asn }), + (afi(), safi(), add_path).prop_map(|(afi, safi, send_recv)| Capability::AddPath { + afi, + safi, + send_recv, + }), + Just(Capability::EnhancedRouteRefresh), + vec((afi(), safi(), any::(), 0u32..(1 << 24)), 0..4).prop_map(|entries| { + Capability::LongLivedGracefulRestart( + entries + .into_iter() + .map(|(afi, safi, flags, stale_time)| LongLivedGracefulRestart { + afi, + safi, + flags, + stale_time, + }) + .collect(), + ) + }), + ( + any::().prop_filter("a code without a variant", |code| { + !KNOWN_CAPABILITIES.contains(code) + }), + vec(any::(), 0..20) + ) + .prop_map(|(type_code, value)| Capability::UnknownCapability { + type_code, + length: value.len() as u8, + value, + }), + ] +} + +fn open() -> impl Strategy { + ( + any::(), + any::(), + any::<[u8; 4]>(), + // Enough to go past 255 octets of options now and then, which + // switches to the extended length of RFC 9072. + vec(vec(capability(), 0..10), 0..8), + ) + .prop_map(|(asn, hold_time, identifier, options)| OpenMessage { + version: 4, + asn, + hold_time, + identifier: Ipv4Addr::from(identifier), + options: options.into_iter().map(OpenOption::Capabilities).collect(), + }) +} + +fn notification() -> impl Strategy { + (1u8..=8, 0u8..12, vec(any::(), 0..40)).prop_filter_map( + "an error code and subcode that are defined", + |(code, subcode, data)| { + BgpError::from_code_subcode(code, subcode) + .ok() + .map(|error| NotificationMessage { error, data }) + }, + ) +} + +fn message(four_octet_asn: bool) -> impl Strategy { + prop_oneof![ + open().prop_map(Message::Open), + update(four_octet_asn).prop_map(Message::Update), + notification().prop_map(Message::Notification), + Just(Message::KeepAlive), + (afi(), any::(), safi()).prop_map(|(afi, subtype, safi)| { + Message::RouteRefresh(RouteRefreshMessage { afi, subtype, safi }) + }), + ] +} + +/// A session's AS number width, and a message that fits it. +fn session_and_message() -> impl Strategy { + any::().prop_flat_map(|four_octet_asn| (Just(four_octet_asn), message(four_octet_asn))) +} + +// --------------------------------------------------------------------------- +// Properties +// --------------------------------------------------------------------------- + +/// What must hold for anything the decoder accepts: it can be shown, +/// written, and read back as itself. +fn check_accepted(ctx: &ParserContext, msg: &Message) -> Result<(), TestCaseError> { + // Display of what a peer sent must not be able to panic: it is logged. + let _ = format!("{msg:?}"); + if let Message::Update(update) = msg { + for prefix in update + .withdrawn_routes + .iter() + .chain(&update.announced_routes) + { + let _ = prefix.to_string(); + } + for attribute in &update.path_attributes { + match attribute { + PathAttribute::MpReachNlri(mp) => { + mp.prefixes.iter().for_each(|p| drop(p.to_string())) + } + PathAttribute::MpUnreachNlri(mp) => { + mp.prefixes.iter().for_each(|p| drop(p.to_string())) + } + _ => {} + } + } + } + + let mut wire = BytesMut::new(); + msg.to_wire(ctx, &mut wire) + .map_err(|e| TestCaseError::fail(format!("accepted but cannot be written: {e}")))?; + prop_assert_eq!(usize::from(msg.wire_len(ctx).unwrap()), wire.len()); + let again = Message::decode(ctx, &wire) + .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e}")))?; + prop_assert_eq!(&again, msg); + Ok(()) +} + +/// Writes a generated message. `None` when it is too big for one BGP +/// message, which the generators can produce and the encoder must refuse; +/// any other refusal is a bug, since everything generated is meant to fit +/// the session. +fn written(ctx: &ParserContext, msg: &Message) -> Result, TestCaseError> { + let mut wire = BytesMut::new(); + match msg.to_wire(ctx, &mut wire) { + Ok(()) => Ok(Some(wire)), + Err(EncodeError::TooLong { .. }) => Ok(None), + Err(e) => Err(TestCaseError::fail(format!("cannot be written: {e}"))), + } +} + +fn framed(msg_type: u8, body: &[u8]) -> Vec { + let mut wire = vec![0xff; 16]; + let length = u16::try_from(usize::from(BGP_HEADER_LEN) + body.len()).unwrap(); + wire.extend_from_slice(&length.to_be_bytes()); + wire.push(msg_type); + wire.extend_from_slice(body); + wire +} + +/// Ways to damage a message. +#[derive(Debug, Clone)] +enum Damage { + Set { at: prop::sample::Index, to: u8 }, + Remove { at: prop::sample::Index }, + Insert { at: prop::sample::Index, byte: u8 }, + Truncate { to: prop::sample::Index }, +} + +fn damage() -> impl Strategy { + prop_oneof![ + (any::(), any::()).prop_map(|(at, to)| Damage::Set { at, to }), + any::().prop_map(|at| Damage::Remove { at }), + (any::(), any::()) + .prop_map(|(at, byte)| Damage::Insert { at, byte }), + any::().prop_map(|to| Damage::Truncate { to }), + ] +} + +/// Applies `damage` to the body of `wire` and makes the header's length +/// right again, so that the damage is the body parsers' problem. +fn damaged(wire: &[u8], damage: &[Damage]) -> Vec { + let header = usize::from(BGP_HEADER_LEN); + let msg_type = wire[header - 1]; + let mut body = wire[header..].to_vec(); + for d in damage { + match d { + Damage::Set { at, to } if !body.is_empty() => { + let i = at.index(body.len()); + body[i] = *to; + } + Damage::Remove { at } if !body.is_empty() => { + body.remove(at.index(body.len())); + } + Damage::Insert { at, byte } => body.insert(at.index(body.len() + 1), *byte), + Damage::Truncate { to } if !body.is_empty() => body.truncate(to.index(body.len())), + _ => {} + } + } + body.truncate(usize::from(BGP_MAX_MSG_LEN - BGP_HEADER_LEN)); + framed(msg_type, &body) +} + +/// 2000 cases a property, which takes about a second. For a longer hunt +/// set `PROPTEST_CASES`, e.g. `PROPTEST_CASES=1000000 cargo test --release +/// -p bgp-packet --test properties`. +fn config() -> ProptestConfig { + let cases = std::env::var("PROPTEST_CASES") + .ok() + .and_then(|cases| cases.parse().ok()) + .unwrap_or(2000); + ProptestConfig { + cases, + // Failures are not saved to a file: one that is found becomes a + // named test next to the code it broke. + failure_persistence: None, + ..ProptestConfig::default() + } +} + +proptest! { + #![proptest_config(config())] + + #[test] + fn a_message_that_can_be_written_reads_back_equal((four_octet_asn, msg) in session_and_message()) { + let ctx = ctx(four_octet_asn); + let Some(wire) = written(&ctx, &msg)? else { + return Ok(()); + }; + prop_assert_eq!(usize::from(msg.wire_len(&ctx).unwrap()), wire.len()); + + let decoded = Message::decode(&ctx, &wire) + .map_err(|e| TestCaseError::fail(format!("{e}")))?; + prop_assert_eq!(&decoded, &msg); + let (rest, _) = Message::from_wire(&ctx, &wire).unwrap(); + prop_assert!(rest.is_empty()); + } + + #[test] + fn noise_is_never_more_than_an_error(four_octet_asn: bool, bytes in vec(any::(), 0..200)) { + let ctx = ctx(four_octet_asn); + if let Ok(msg) = Message::decode(&ctx, &bytes) { + check_accepted(&ctx, &msg)?; + } + } + + #[test] + fn noise_behind_a_valid_header_is_never_more_than_an_error( + four_octet_asn: bool, + msg_type in 0u8..7, + body in vec(any::(), 0..300), + ) { + let ctx = ctx(four_octet_asn); + if let Ok(msg) = Message::decode(&ctx, &framed(msg_type, &body)) { + check_accepted(&ctx, &msg)?; + } + } + + #[test] + fn a_damaged_message_is_never_more_than_an_error( + (four_octet_asn, msg) in session_and_message(), + damage in vec(damage(), 1..4), + ) { + let ctx = ctx(four_octet_asn); + let Some(wire) = written(&ctx, &msg)? else { + return Ok(()); + }; + if let Ok(msg) = Message::decode(&ctx, &damaged(&wire, &damage)) { + check_accepted(&ctx, &msg)?; + } + } + + /// The parsers below the message level are public too. + #[test] + fn noise_is_never_more_than_an_error_for_the_parts( + four_octet_asn: bool, + bytes in vec(any::(), 0..120), + ) { + let ctx = ctx(four_octet_asn); + let _ = OpenMessage::from_wire(&ctx, &bytes); + let _ = OpenOption::from_wire(&ctx, &bytes); + let _ = Capability::from_wire(&ctx, &bytes); + let _ = UpdateMessage::from_wire(&ctx, &bytes); + let _ = PathAttribute::from_wire(&ctx, &bytes); + let _ = NotificationMessage::from_wire(&ctx, &bytes); + } +} diff --git a/crates/primitives/Cargo.toml b/crates/primitives/Cargo.toml index 75c555e..3894739 100644 --- a/crates/primitives/Cargo.toml +++ b/crates/primitives/Cargo.toml @@ -17,5 +17,8 @@ serde_repr.workspace = true thiserror.workspace = true strum.workspace = true +[dev-dependencies] +proptest.workspace = true + [lints] workspace = true diff --git a/crates/primitives/src/constants.rs b/crates/primitives/src/constants.rs index e02182a..7141aaf 100644 --- a/crates/primitives/src/constants.rs +++ b/crates/primitives/src/constants.rs @@ -18,6 +18,16 @@ pub enum AddressFamilyId { Ipv6 = 2, } +impl AddressFamilyId { + /// How many bits an address of this version has. + pub fn bits(self) -> u8 { + match self { + Self::Ipv4 => 32, + Self::Ipv6 => 128, + } + } +} + impl TryFrom for AddressFamilyId { type Error = UnknownAfi; diff --git a/crates/primitives/src/ip_prefix.rs b/crates/primitives/src/ip_prefix.rs index 933fe7e..e994ca5 100644 --- a/crates/primitives/src/ip_prefix.rs +++ b/crates/primitives/src/ip_prefix.rs @@ -40,6 +40,10 @@ impl IpPrefix { prefix: Vec, length: u8, ) -> Result { + let max = address_family.bits(); + if length > max { + return Err(PrefixError::LengthTooLong { length, max }); + } // Ensure that the prefix we are given contains the right number of bytes corresponding to the prefix length. let needed = usize::from(length.div_ceil(8)); if prefix.len() < needed { @@ -49,11 +53,26 @@ impl IpPrefix { got: prefix.len(), }); } - Ok(Self { + Ok(Self::normalised(address_family, prefix, length)) + } + + /// Builds the one representation of a prefix: exactly the bytes its + /// length covers, with the bits past the length cleared. Those bits mean + /// nothing (RFC 4271 §4.3), so two prefixes that differ only there are + /// the same prefix and must compare equal. + /// + /// `length` must fit the address family and `prefix` must have at least + /// the bytes it covers. + fn normalised(address_family: AddressFamilyId, mut prefix: Vec, length: u8) -> Self { + prefix.truncate(usize::from(length.div_ceil(8))); + if let (Some(last), rem @ 1..) = (prefix.last_mut(), length % 8) { + *last &= u8::MAX << (8 - rem); + } + Self { address_family, prefix, length, - }) + } } /// Returns true if `other` is equal to or more specific than `self`, i.e. @@ -75,6 +94,16 @@ impl IpPrefix { (self.prefix[full_bytes] & mask) == (other.prefix[full_bytes] & mask) } + /// The prefix as a whole address, zero where the prefix says nothing. + /// The fields are public, so this cannot assume `prefix` has a sensible + /// length: showing a prefix must never fail, whatever is in it. + fn padded(&self) -> [u8; N] { + let mut address = [0u8; N]; + let len = self.prefix.len().min(N); + address[..len].copy_from_slice(&self.prefix[..len]); + address + } + pub fn to_wire(&self, _: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { // Verify that there is enough space to write the IpPrefix. if out.remaining() < (self.prefix.len() + 1) { @@ -98,36 +127,14 @@ impl TryFrom<&str> for IpPrefix { let (addr, length) = value.split_once('/').ok_or_else(format)?; let length: u8 = length.parse().map_err(|_| format())?; - let (afi, mut octets, max) = if addr.contains(':') { + let (afi, octets) = if addr.contains(':') { let addr = Ipv6Addr::from_str(addr).map_err(|_| address())?; - (AddressFamilyId::Ipv6, addr.octets().to_vec(), 128) + (AddressFamilyId::Ipv6, addr.octets().to_vec()) } else { let addr = Ipv4Addr::from_str(addr).map_err(|_| address())?; - (AddressFamilyId::Ipv4, addr.octets().to_vec(), 32) + (AddressFamilyId::Ipv4, addr.octets().to_vec()) }; - if length > max { - return Err(PrefixError::LengthTooLong { length, max }); - } - - // Truncate the octets we have to the right number of bytes to match the prefix length.. - if length.is_multiple_of(8) { - // We can cleanly truncate the number of bytes since we are at a byte boundary. - octets.truncate((length / 8).into()); - } else { - // We need to keep length % 8 bits of the last byte. - let num_bytes = (length / 8) + 1; - let mask = u8::MAX << (8 - (length % 8)); - octets.truncate(num_bytes.into()); - // Fix up the last byte. - let last_pos = octets.len() - 1; - octets[last_pos] &= mask; - } - - Ok(IpPrefix { - address_family: afi, - prefix: octets, - length, - }) + IpPrefix::new(afi, octets, length) } } @@ -135,29 +142,10 @@ impl Display for IpPrefix { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self.address_family { AddressFamilyId::Ipv4 => { - // Pad the prefix with 0 bytes if it's less than 4 bytes long. - let bytes = &mut self.prefix.clone(); - if bytes.len() < 4 { - bytes.extend(std::iter::repeat_n(0, 4 - bytes.len())); - } - let four_bytes: [u8; 4] = bytes - .as_slice() - .try_into() - .map_err(|_| std::fmt::Error {})?; - let ipv4_addr = Ipv4Addr::from(four_bytes); - write!(f, "{}/{}", ipv4_addr, self.length) + write!(f, "{}/{}", Ipv4Addr::from(self.padded()), self.length) } AddressFamilyId::Ipv6 => { - let bytes = &mut self.prefix.clone(); - if bytes.len() < 16 { - bytes.extend(std::iter::repeat_n(0, 16 - bytes.len())); - } - let sixteen_bytes: [u8; 16] = bytes - .as_slice() - .try_into() - .map_err(|_| std::fmt::Error {})?; - let ipv6_addr = Ipv6Addr::from(sixteen_bytes); - write!(f, "{}/{}", ipv6_addr, self.length) + write!(f, "{}/{}", Ipv6Addr::from(self.padded()), self.length) } } } @@ -167,6 +155,8 @@ impl Display for IpPrefix { mod tests { use crate::constants::AddressFamilyId; + use proptest::prelude::*; + use super::{IpPrefix, PrefixError}; macro_rules! verify_roundtrip { @@ -273,6 +263,66 @@ mod tests { ); } + #[test] + fn a_prefix_has_one_representation() { + // Extra bytes and stray bits past the length are dropped. + let noisy = IpPrefix::new(AddressFamilyId::Ipv4, vec![10, 0xff, 3, 4], 12).unwrap(); + assert_eq!(noisy, IpPrefix::try_from("10.240.0.0/12").unwrap()); + assert_eq!(noisy.prefix, vec![10, 0xf0]); + assert_eq!( + IpPrefix::new(AddressFamilyId::Ipv4, vec![0xff; 13], 99), + Err(PrefixError::LengthTooLong { + length: 99, + max: 32 + }) + ); + } + + /// The fields are public, so a prefix can be put together wrongly. + /// Showing it must still not fail: prefixes are shown in log lines. + #[test] + fn showing_a_prefix_never_fails() { + let wrong = IpPrefix { + address_family: AddressFamilyId::Ipv4, + prefix: vec![1, 2, 3, 4, 5, 6], + length: 200, + }; + assert_eq!(wrong.to_string(), "1.2.3.4/200"); + } + + proptest! { + #[test] + fn text_round_trips(version: bool, bytes: [u8; 16], length in 0u8..=128) { + let version = if version { AddressFamilyId::Ipv6 } else { AddressFamilyId::Ipv4 }; + let length = length.min(version.bits()); + let prefix = IpPrefix::new(version, bytes.to_vec(), length).unwrap(); + let text = prefix.to_string(); + prop_assert_eq!(IpPrefix::try_from(text.as_str()), Ok(prefix.clone())); + prop_assert!(prefix.contains(&prefix)); + } + + #[test] + fn text_that_is_noise_is_an_error_or_a_prefix_that_round_trips(text in "\\PC{0,40}") { + if let Ok(prefix) = IpPrefix::try_from(text.as_str()) { + let shown = prefix.to_string(); + prop_assert_eq!(IpPrefix::try_from(shown.as_str()), Ok(prefix)); + } + } + + /// `contains` agrees with comparing the leading bits by hand. + #[test] + fn contains_is_a_comparison_of_leading_bits( + a: [u8; 4], a_len in 0u8..=32, b: [u8; 4], b_len in 0u8..=32, + ) { + let outer = IpPrefix::new(AddressFamilyId::Ipv4, a.to_vec(), a_len).unwrap(); + let inner = IpPrefix::new(AddressFamilyId::Ipv4, b.to_vec(), b_len).unwrap(); + let mask = if a_len == 0 { 0 } else { u32::MAX << (32 - a_len) }; + let expected = b_len >= a_len + && (u32::from_be_bytes(a) & mask) == (u32::from_be_bytes(b) & mask); + prop_assert_eq!(outer.contains(&inner), expected); + } + } + #[test] fn contains_covers_more_specifics() { let p = |s: &str| IpPrefix::try_from(s).unwrap(); diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index b6f3823..c606518 100644 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -24,5 +24,8 @@ thiserror.workspace = true [build-dependencies] protobuf-codegen.workspace = true +[dev-dependencies] +proptest.workspace = true + [lints] workspace = true diff --git a/crates/server/src/connection.rs b/crates/server/src/connection.rs index 55147f9..2cce3a0 100644 --- a/crates/server/src/connection.rs +++ b/crates/server/src/connection.rs @@ -431,6 +431,10 @@ fn prefix_counts(update: &UpdateMessage) -> (usize, usize) { mod tests { use std::time::Duration; + use bgp_packet::errors::CeaseSubcode; + use bgp_packet::message::NotificationMessage; + use proptest::collection::vec; + use proptest::prelude::*; use tokio::io::{AsyncWriteExt, duplex}; use super::*; @@ -482,6 +486,62 @@ mod tests { assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive))); } + proptest! { + /// TCP delivers a stream, not messages: however the bytes of a run + /// of messages are cut into reads, the same messages come out. + #[test] + fn framing_does_not_depend_on_how_the_stream_is_cut( + bodies in vec(vec(any::(), 0..60), 1..8), + cuts in vec(1usize..40, 0..30), + ) { + // NOTIFICATIONs, since their body may be any bytes after a + // valid code and subcode. + let sent: Vec = bodies + .into_iter() + .map(|data| { + Message::Notification(NotificationMessage { + error: BgpError::Cease(CeaseSubcode::AdminShutdown), + data, + }) + }) + .collect(); + let ctx = ParserContext::default(); + let mut stream = BytesMut::new(); + for msg in &sent { + msg.to_wire(&ctx, &mut stream).unwrap(); + } + + let runtime = tokio::runtime::Builder::new_current_thread().build().unwrap(); + let received = runtime.block_on(async { + let (mut peer, ours) = duplex(8192); + let mut frames = FrameReader::new(ours); + let writer = async { + let mut rest = &stream[..]; + for cut in cuts.iter().copied().chain(std::iter::repeat(40)) { + if rest.is_empty() { + break; + } + let (now, later) = rest.split_at(cut.min(rest.len())); + peer.write_all(now).await.unwrap(); + // Let the reader see this piece on its own. + tokio::task::yield_now().await; + rest = later; + } + drop(peer); + }; + let reader = async { + let mut received = Vec::new(); + while let Ok(msg) = frames.next(&ctx).await { + received.push(msg); + } + received + }; + tokio::join!(writer, reader).1 + }); + prop_assert_eq!(received, sent); + } + } + #[tokio::test] async fn an_impossible_length_is_rejected_from_the_header_alone() { let (mut peer, ours) = duplex(4096); diff --git a/crates/server/src/fsm.rs b/crates/server/src/fsm.rs index 97deaa4..ec0b675 100644 --- a/crates/server/src/fsm.rs +++ b/crates/server/src/fsm.rs @@ -227,7 +227,7 @@ impl Fsm { actions.push(Action::DeliverUpdate(update)); actions } - (Phase::Established(negotiated), Message::RouteRefresh) => { + (Phase::Established(negotiated), Message::RouteRefresh(_)) => { let mut actions = heard_from_peer(negotiated); actions.push(Action::RouteRefreshRequested); actions @@ -372,6 +372,7 @@ mod tests { use bgp_packet::errors::{CeaseSubcode, FsmSubcode, MsgHeaderSubcode}; use bgp_packet::message::{ MpReachNlriPathAttribute, NlriNextHop, NotificationMessage, PathAttribute, + RouteRefreshMessage, }; use bgp_primitives::constants::{Afi, AfiSafi, Safi}; use bgp_primitives::ip_prefix::IpPrefix; @@ -790,7 +791,9 @@ mod tests { [Action::ResetHoldTimer(d), Action::DeliverUpdate(_)] if *d == hold )); - let actions = fsm.handle(Event::Message(Message::RouteRefresh)); + let actions = fsm.handle(Event::Message(Message::RouteRefresh( + RouteRefreshMessage::request(AfiSafi::IPV4_UNICAST), + ))); assert!(matches!( &actions[..], [Action::ResetHoldTimer(d), Action::RouteRefreshRequested] if *d == hold diff --git a/crates/server/src/metrics.rs b/crates/server/src/metrics.rs index 1b1fe08..ebfef25 100644 --- a/crates/server/src/metrics.rs +++ b/crates/server/src/metrics.rs @@ -82,6 +82,6 @@ pub fn message_type_name(msg: &Message) -> &'static str { Message::Update(_) => "update", Message::Notification(_) => "notification", Message::KeepAlive => "keepalive", - Message::RouteRefresh => "route_refresh", + Message::RouteRefresh(_) => "route_refresh", } } diff --git a/fuzz/.gitignore b/fuzz/.gitignore new file mode 100644 index 0000000..45eaf5a --- /dev/null +++ b/fuzz/.gitignore @@ -0,0 +1,6 @@ +# What a fuzzing run produces. Seeds worth keeping live in seeds/. +target/ +corpus/ +artifacts/ +coverage/ +Cargo.lock diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml new file mode 100644 index 0000000..498713c --- /dev/null +++ b/fuzz/Cargo.toml @@ -0,0 +1,54 @@ +# Coverage-guided fuzz targets, run with cargo-fuzz on a nightly toolchain. +# See README.md in this directory. +[package] +name = "bgp-fuzz" +version = "0.0.0" +edition = "2024" +license = "Apache-2.0" +publish = false + +[package.metadata] +cargo-fuzz = true + +# Not part of the main workspace: libfuzzer needs nightly, the workspace +# builds on stable. +[workspace] + +[dependencies] +bgp-packet = { path = "../crates/packet" } +bgp-primitives = { path = "../crates/primitives" } +bgp-server = { path = "../crates/server" } +bytes = "1.10.1" +libfuzzer-sys = "0.4" +protobuf = "3.7.2" + +[lib] +path = "src/lib.rs" + +[[bin]] +name = "message" +path = "fuzz_targets/message.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "message_body" +path = "fuzz_targets/message_body.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "prefix_text" +path = "fuzz_targets/prefix_text.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "config" +path = "fuzz_targets/config.rs" +test = false +doc = false +bench = false diff --git a/fuzz/README.md b/fuzz/README.md new file mode 100644 index 0000000..a62241b --- /dev/null +++ b/fuzz/README.md @@ -0,0 +1,47 @@ +# Fuzz targets + +Coverage-guided fuzzing of everything that reads input we do not control. +The property tests (`cargo test`, e.g. `crates/packet/tests/properties.rs`) +check the same things on generated input and run on every build; these +targets go further by letting libFuzzer follow code coverage for as long as +you leave them running. + +| Target | Input | +|----------------|------------------------------------------------------------------| +| `message` | Raw bytes as they come off a TCP connection | +| `message_body` | A message body behind a correct header (reaches the body parsers) | +| `prefix_text` | A prefix as text | +| `config` | The configuration file | + +For the two message targets a finding is either a panic, or a message the +decoder accepted that cannot be written and read back as itself. + +## Running + +libFuzzer needs a nightly toolchain and `cargo-fuzz`: + +```bash +rustup toolchain install nightly +cargo install cargo-fuzz +``` + +Then, from this directory: + +```bash +cargo +nightly fuzz run message_body corpus/message_body seeds/message_body -- -max_total_time=300 +``` + +The first directory is the working corpus, which the fuzzer adds to and +which is not checked in. `seeds/` holds a few valid inputs to start from. +Leave out `-max_total_time` to run until interrupted. + +## When it finds something + +The input is saved under `artifacts//`. Reproduce it with + +```bash +cargo +nightly fuzz run message_body artifacts/message_body/crash-... +``` + +Fix the bug, and add a unit test with the input next to the code it broke, +so that `cargo test` keeps it fixed without anyone having to run the fuzzer. diff --git a/fuzz/fuzz_targets/config.rs b/fuzz/fuzz_targets/config.rs new file mode 100644 index 0000000..adbc213 --- /dev/null +++ b/fuzz/fuzz_targets/config.rs @@ -0,0 +1,13 @@ +//! The configuration file. It is written by the operator and not by a peer, +//! but a typo in it should be an error message, never a panic at startup. +#![no_main] + +use bgp_server::config::Config; +use bgp_server::proto::config::BgpConfig; +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|text: &str| { + if let Ok(proto) = protobuf::text_format::parse_from_str::(text) { + let _ = Config::from_proto(proto); + } +}); diff --git a/fuzz/fuzz_targets/message.rs b/fuzz/fuzz_targets/message.rs new file mode 100644 index 0000000..8057605 --- /dev/null +++ b/fuzz/fuzz_targets/message.rs @@ -0,0 +1,12 @@ +//! Raw bytes, as they come off a TCP connection. The first byte picks the +//! session's AS number width. +#![no_main] + +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|data: &[u8]| { + let Some((selector, wire)) = data.split_first() else { + return; + }; + bgp_fuzz::decode_and_check(selector & 1 == 1, wire); +}); diff --git a/fuzz/fuzz_targets/message_body.rs b/fuzz/fuzz_targets/message_body.rs new file mode 100644 index 0000000..547e5e2 --- /dev/null +++ b/fuzz/fuzz_targets/message_body.rs @@ -0,0 +1,24 @@ +//! A message body behind a correct header, so that the fuzzer spends its +//! time in the OPEN, UPDATE and NOTIFICATION parsers and not on guessing +//! the marker and length. The first byte picks the AS number width and the +//! message type. +#![no_main] + +use libfuzzer_sys::fuzz_target; + +/// The longest body that fits a BGP message (4096 less the 19-byte header). +const MAX_BODY: usize = 4096 - 19; + +fuzz_target!(|data: &[u8]| { + let Some((selector, body)) = data.split_first() else { + return; + }; + let body = &body[..body.len().min(MAX_BODY)]; + let msg_type = 1 + (selector >> 1) % 5; + + let mut wire = vec![0xff; 16]; + wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes()); + wire.push(msg_type); + wire.extend_from_slice(body); + bgp_fuzz::decode_and_check(selector & 1 == 1, &wire); +}); diff --git a/fuzz/fuzz_targets/prefix_text.rs b/fuzz/fuzz_targets/prefix_text.rs new file mode 100644 index 0000000..e326aaa --- /dev/null +++ b/fuzz/fuzz_targets/prefix_text.rs @@ -0,0 +1,12 @@ +//! Prefixes as text, as they appear in the configuration file. +#![no_main] + +use bgp_primitives::ip_prefix::IpPrefix; +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|text: &str| { + if let Ok(prefix) = IpPrefix::try_from(text) { + let shown = prefix.to_string(); + assert_eq!(IpPrefix::try_from(shown.as_str()), Ok(prefix)); + } +}); diff --git a/fuzz/seeds/config/00 b/fuzz/seeds/config/00 new file mode 100644 index 0000000..63dfe35 --- /dev/null +++ b/fuzz/seeds/config/00 @@ -0,0 +1,38 @@ +# A worked example; the fields are documented in +# crates/server/proto/config.proto. +router_id: "10.0.0.1" +local_asn: 65000 + +# Where to accept BGP connections. Port 179 needs privileges; pick another +# port to run unprivileged against a peer that dials a custom port. +listen_endpoints { address: "127.0.0.1" port: 179 } +listen_endpoints { address: "::1" port: 179 } + +# A peer we connect to (and accept connections from). +peers { + name: "r1" + endpoint { address: "127.0.0.1" port: 1791 } + remote_asn: 65001 + hold_time_seconds: 30 + # Offer only these families to the peer. Left out, all are offered. + address_families: IPV4_UNICAST + address_families: IPV6_UNICAST +} +connect_retry_seconds: 5 + +# Let a restarted peer replace a stale Established session immediately +# (RFC 4271 CollisionDetectEstablishedState). Default false. +collision_detect_established_state: false + +# Routes this router originates, announced to every peer. +originate { prefix: "10.100.0.0/16" } +originate { + prefix: "2001:db8:100::/48" + med: 50 + communities: "65000:100" + large_communities: "65000:1:2" +} + +# Read-only status page, JSON API and metrics. It has no authentication: +# keep it on loopback or behind a reverse proxy. +http_endpoint { address: "127.0.0.1" port: 8080 } diff --git a/fuzz/seeds/message/00 b/fuzz/seeds/message/00 new file mode 100644 index 0000000000000000000000000000000000000000..2daddbe6995952b7081d96b23912b5b26ece23c3 GIT binary patch literal 56 ycmZShj|Ldb8CjwiFnr)*U|^DBl3-+EU}9ioVqjx%U}9i!WMN=7DRE5_jjv1#g*f=mUGB_}?F)=bQ{C(-b%yNK%iLrr&g@J*==l}=H-xmxi j2Lw16nEt+CU|>i&puoex0A?~WFfcGla4|CRaD4y(-TokS literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/05 b/fuzz/seeds/message/05 new file mode 100644 index 0000000000000000000000000000000000000000..009cdc3bb29a777f0ff7a403032dac3e8fe121f1 GIT binary patch literal 95 zcmZSlj|Ldxm>5_jjv1#g*f=mUGB_}?F)=bQ{C(-b%yNK%iLrr&g@J*==l}=H-xmxi j2Lw16nEt+CU|>i&puoex0A?~WFfcGla4|CRaD4y(-d!Md literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/06 b/fuzz/seeds/message/06 new file mode 100644 index 0000000000000000000000000000000000000000..20b66d5dd7b812fe0201fc0aad51a90493b4009e GIT binary patch literal 80 zcmZShj|Le0nHU%tEF2ga8623{m>3xt{=S^R#~{PN#3-P^$h(690T>w!z#@zd{Ok-& Ij0zwQ0A1=AR{#J2 literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/07 b/fuzz/seeds/message/07 new file mode 100644 index 0000000000000000000000000000000000000000..04d5475976ac031c06568450e71ea3577336c680 GIT binary patch literal 80 zcmZSlj|Le0nHU%tEF2ga8623{m>3xt{=S^R#~{PN#3-P^$h(690T>w!z#@zd{Ok-& Ij0zwQ0A4y5SO5S3 literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/08 b/fuzz/seeds/message/08 new file mode 100644 index 0000000000000000000000000000000000000000..2e1342727269d6754d92aefc7d223703d47f7889 GIT binary patch literal 24 RcmZShj|LdTnHU%t7y$RQ5DEYQ literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/09 b/fuzz/seeds/message/09 new file mode 100644 index 0000000000000000000000000000000000000000..bc1206e33cc6156518a810f3e0114ddf56dd0eba GIT binary patch literal 24 RcmZSlj|LdTnHU%t7y$Ro5DNeR literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/10 b/fuzz/seeds/message/10 new file mode 100644 index 0000000000000000000000000000000000000000..c4bc75bf280aefa83a3386a3cd3a8bc665e2caed GIT binary patch literal 25 ScmZShj|LbdnAw<;DpLUpeGyau literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/11 b/fuzz/seeds/message/11 new file mode 100644 index 0000000000000000000000000000000000000000..94db0477f632ca4575d4f406a975900617628877 GIT binary patch literal 25 ScmZSlj|LbdnAw<;DpLUpmJw9| literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/12 b/fuzz/seeds/message/12 new file mode 100644 index 0000000000000000000000000000000000000000..be2f2cc1ea5c962a92645c2dafbb4ace78d8ba4f GIT binary patch literal 20 NcmZShj|LcoSpc_&5C{MO literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/13 b/fuzz/seeds/message/13 new file mode 100644 index 0000000000000000000000000000000000000000..de9cf0d57afc0243cfc30fc212c8bbdc585b2adc GIT binary patch literal 20 NcmZSlj|LcoSpc`15D5SP literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message/14 b/fuzz/seeds/message/14 new file mode 100644 index 0000000000000000000000000000000000000000..3200a2f019c06259c927f594165773d11f3d985c GIT binary patch literal 24 RcmZShj|LdTSs54^7y>we2IK$$ literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message_body/03 b/fuzz/seeds/message_body/03 new file mode 100644 index 0000000000000000000000000000000000000000..a68ff5bb584295e3fbe04ce0b67c390e63e7f7c4 GIT binary patch literal 67 zcmZQ%`TK%l3l{?eqXiQiBMSo~10xet0|OHi69W^QBMSq=-xo|A4s47%42+BnOd_m8 RAQcQu42%p+V6uUM2>>xM2IT+% literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message_body/04 b/fuzz/seeds/message_body/04 new file mode 100644 index 0000000000000000000000000000000000000000..b93d6954d142e707e7199dd51ae78010748b0c71 GIT binary patch literal 76 zcmZQ#V39axoWfw^z{tqpz{JMH$iVRTr2{j|0R|?<1{M|u1_q-894vodFr*w1;9y|- d`+|XiA?1Jq4+8_3$;iOKz$C%N$i&0-0RUdY5GDWs literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message_body/05 b/fuzz/seeds/message_body/05 new file mode 100644 index 0000000000000000000000000000000000000000..d2f8b81b9a2c9a530e1a6a5ca19bb8df87307c5f GIT binary patch literal 76 zcmZQ(V39axoWfw^z{tqpz{JMH$iVRTr2{j|0R|?<1{M|u1_q-894vodFr*w1;9y|- d`+|XiA?1Jq4+8_3$;iOKz$C%N$i&0-0RUeP5GMct literal 0 HcmV?d00001 diff --git a/fuzz/seeds/message_body/06 b/fuzz/seeds/message_body/06 new file mode 100644 index 0000000000000000000000000000000000000000..42e18a6fa2b9951ff96b779e0d976b916df58474 GIT binary patch literal 61 zcmZQ#U|_ItU}R))U}9rpWMKIFasnTN3