From b047e872c268c50faf0382bfa25f63cf37a20177 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Wed, 9 Jul 2025 10:17:06 -0700 Subject: [PATCH 1/2] fix: simply fuzz targets, add a coverage viewer --- .github/workflows/ci.yml | 3 +- .gitignore | 4 +- justfile | 24 +++++-- protocol/fuzz/Cargo.toml | 9 +-- protocol/fuzz/coverage.sh | 44 ++++++++++++ protocol/fuzz/fuzz_targets/receive_garbage.rs | 71 ++++++++++--------- protocol/fuzz/fuzz_targets/receive_key.rs | 40 +++++++---- protocol/fuzz/fuzz_targets/receive_version.rs | 70 ------------------ 8 files changed, 128 insertions(+), 137 deletions(-) create mode 100755 protocol/fuzz/coverage.sh delete mode 100644 protocol/fuzz/fuzz_targets/receive_version.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 60e052e..bcaf739 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,11 +63,10 @@ jobs: - uses: extractions/setup-just@v3 - uses: actions-rust-lang/setup-rust-toolchain@v1 - run: just bench - # Light smoke test fuzz. fuzz: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - uses: extractions/setup-just@v3 - uses: actions-rust-lang/setup-rust-toolchain@v1 - - run: just fuzz + - run: just test fuzz diff --git a/.gitignore b/.gitignore index 08b0bcf..ae34dc2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,2 @@ Cargo.lock -/target -# IDEs -.vscode/ +**/target diff --git a/justfile b/justfile index 67e5c3e..239b84d 100644 --- a/justfile +++ b/justfile @@ -3,8 +3,9 @@ # The recipes make heavy use of `rustup`'s toolchain syntax (e.g. `cargo +nightly`). `rustup` is # required on the system in order to intercept the `cargo` commands and to install and use the appropriate toolchain with components. -NIGHTLY_TOOLCHAIN := "nightly-2025-06-19" +NIGHTLY_TOOLCHAIN := "nightly-2025-07-10" STABLE_TOOLCHAIN := "1.87.0" +FUZZ_VERSION := "0.12.0" _default: @just --list @@ -29,7 +30,7 @@ _default: # Adding --fix flag to apply suggestions with --allow-dirty. cargo +{{NIGHTLY_TOOLCHAIN}} clippy --workspace --all-features --all-targets --fix --allow-dirty -- -D warnings -# Run a test suite: unit, features, msrv, constraints, or no-std. +# Run a test suite: unit, features, msrv, constraints, no-std, or fuzz. @test suite="unit": just _test-{{suite}} @@ -71,14 +72,25 @@ _default: cargo install cross@0.2.5 $HOME/.cargo/bin/cross build --package bip324 --target thumbv7m-none-eabi --no-default-features +# Type check the fuzz targets. +@_test-fuzz: + cargo install cargo-fuzz@{{FUZZ_VERSION}} + cd protocol && cargo +{{NIGHTLY_TOOLCHAIN}} fuzz check + # Run benchmarks. bench: cargo +{{NIGHTLY_TOOLCHAIN}} bench --package bip324 --bench cipher_session -# Run fuzz test: receive_key, receive_garbage, receive_version. -@fuzz target="receive_garbage" time="60": - cargo install cargo-fuzz@0.12.0 - cd protocol && cargo +{{NIGHTLY_TOOLCHAIN}} fuzz run {{target}} -- -max_total_time={{time}} +# Run fuzz target: receive_key or receive_garbage. +@fuzz target seconds: + rustup component add --toolchain {{NIGHTLY_TOOLCHAIN}} llvm-tools-preview + cargo install cargo-fuzz@{{FUZZ_VERSION}} + # Generate new test cases and add to corpus. Bumping length for garbage. + cd protocol && cargo +{{NIGHTLY_TOOLCHAIN}} fuzz run {{target}} -- -max_len=5120 -max_total_time={{seconds}} + # Measure coverage of corpus against code. + cd protocol && cargo +{{NIGHTLY_TOOLCHAIN}} fuzz coverage {{target}} + # Generate HTML coverage report. + protocol/fuzz/coverage.sh {{NIGHTLY_TOOLCHAIN}} {{target}} # Add a release tag and publish to the upstream remote. Need write privileges on the repository. @tag crate version remote="upstream": diff --git a/protocol/fuzz/Cargo.toml b/protocol/fuzz/Cargo.toml index e1d6c35..172a00a 100644 --- a/protocol/fuzz/Cargo.toml +++ b/protocol/fuzz/Cargo.toml @@ -10,6 +10,8 @@ cargo-fuzz = true [dependencies] libfuzzer-sys = "0.4" bip324 = { path = ".." } +rand = "0.8" +secp256k1 = "0.29" [[bin]] name = "receive_garbage" @@ -18,13 +20,6 @@ test = false doc = false bench = false -[[bin]] -name = "receive_version" -path = "fuzz_targets/receive_version.rs" -test = false -doc = false -bench = false - [[bin]] name = "receive_key" path = "fuzz_targets/receive_key.rs" diff --git a/protocol/fuzz/coverage.sh b/protocol/fuzz/coverage.sh new file mode 100755 index 0000000..d3f0ece --- /dev/null +++ b/protocol/fuzz/coverage.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash + +# Generate HTML coverage report +# Usage: ./coverage.sh +# Example: ./coverage.sh nightly-2025-07-10 receive_key +# +# Grabbed from this blog: https://tweedegolf.nl/en/blog/154/what-is-my-fuzzer-doing +# Hopefully standardized soon: https://github.com/taiki-e/cargo-llvm-cov/pull/431 + +set -euo pipefail + +if [ $# -ne 2 ]; then + echo "Usage: $0 " + echo "Example: $0 nightly-2025-07-10 receive_key" + exit 1 +fi + +TOOLCHAIN="$1" +TARGET="$2" + +# Change to protocol directory. +cd "$(dirname "$0")/.." + +# Install rustfilt for demangling. +cargo install rustfilt@0.2.1 +RUSTFILT="$HOME/.cargo/bin/rustfilt" + +# Get toolchain info +SYSROOT=$(rustc +${TOOLCHAIN} --print sysroot) +HOST_TUPLE=$(rustc +${TOOLCHAIN} --print host-tuple) + +BINARY="target/$HOST_TUPLE/coverage/$HOST_TUPLE/release/$TARGET" + +echo "Generating HTML coverage report for $TARGET..." +"$SYSROOT/lib/rustlib/$HOST_TUPLE/bin/llvm-cov" show \ + "$BINARY" \ + -instr-profile=fuzz/coverage/"$TARGET"/coverage.profdata \ + -Xdemangler="$RUSTFILT" \ + --format=html \ + -output-dir=fuzz/coverage/"$TARGET"/html \ + -ignore-filename-regex="\.cargo|\.rustup|fuzz_target|/rustc/" + +REPORT_PATH="$(pwd)/fuzz/coverage/$TARGET/html/index.html" +echo "$REPORT_PATH" diff --git a/protocol/fuzz/fuzz_targets/receive_garbage.rs b/protocol/fuzz/fuzz_targets/receive_garbage.rs index 5e6a9ae..7aaff86 100644 --- a/protocol/fuzz/fuzz_targets/receive_garbage.rs +++ b/protocol/fuzz/fuzz_targets/receive_garbage.rs @@ -1,21 +1,36 @@ // SPDX-License-Identifier: CC0-1.0 //! Fuzz test for the receive_garbage function. -//! -//! This focused test fuzzes only the garbage terminator detection logic, -//! which is more effective than trying to fuzz the entire handshake. #![no_main] -use bip324::{GarbageResult, Handshake, Initialized, Network, ReceivedKey, Role}; +use bip324::{Handshake, Initialized, Network, ReceivedKey, Role}; use libfuzzer_sys::fuzz_target; +use rand::SeedableRng; fuzz_target!(|data: &[u8]| { + // Cap input size to avoid wasting time on obviously invalid large inputs + // The protocol limit is 4095 garbage bytes + 16 terminator bytes = 4111 total + // Test up to ~5000 bytes to cover boundary cases + if data.len() > 5000 { + return; + } + + // Use deterministic seeds for reproducible fuzzing + let seed = [42u8; 32]; + let mut rng = rand::rngs::StdRng::from_seed(seed); + let secp = secp256k1::Secp256k1::signing_only(); + // Set up a valid handshake in the SentVersion state - let initiator = Handshake::::new(Network::Bitcoin, Role::Initiator).unwrap(); + let initiator = + Handshake::::new_with_rng(Network::Bitcoin, Role::Initiator, &mut rng, &secp) + .unwrap(); let mut initiator_key = vec![0u8; Handshake::::send_key_len(None)]; let initiator = initiator.send_key(None, &mut initiator_key).unwrap(); - let responder = Handshake::::new(Network::Bitcoin, Role::Responder).unwrap(); + let mut rng2 = rand::rngs::StdRng::from_seed([43u8; 32]); + let responder = + Handshake::::new_with_rng(Network::Bitcoin, Role::Responder, &mut rng2, &secp) + .unwrap(); let mut responder_key = vec![0u8; Handshake::::send_key_len(None)]; let responder = responder.send_key(None, &mut responder_key).unwrap(); @@ -23,41 +38,29 @@ fuzz_target!(|data: &[u8]| { let initiator = initiator .receive_key(responder_key[..64].try_into().unwrap()) .unwrap(); - let _responder = responder + let responder = responder .receive_key(initiator_key[..64].try_into().unwrap()) .unwrap(); + // Get the real responder's garbage terminator from responder's send_version output + let mut responder_version = vec![0u8; Handshake::::send_version_len(None)]; + let _responder = responder + .send_version(&mut responder_version, None) + .unwrap(); + + // The responder's garbage terminator is in the first 16 bytes of their version output + let responder_terminator = &responder_version[..16]; + // Send version to reach SentVersion state let mut initiator_version = vec![0u8; Handshake::::send_version_len(None)]; let initiator = initiator .send_version(&mut initiator_version, None) .unwrap(); - // Now fuzz the receive_garbage function with arbitrary data - match initiator.receive_garbage(data) { - Ok(GarbageResult::FoundGarbage { - handshake: _, - consumed_bytes, - }) => { - // Successfully found garbage terminator - // Verify consumed_bytes is reasonable - assert!(consumed_bytes <= data.len()); - assert!(consumed_bytes >= 16); // At least the terminator size - - // The garbage should be everything before the terminator - let garbage_len = consumed_bytes - 16; - assert!(garbage_len <= 4095); // Max garbage size - } - Ok(GarbageResult::NeedMoreData(_)) => { - // Need more data - valid outcome for short inputs - // This should happen when: - // 1. Buffer is too short to contain terminator - // 2. Buffer doesn't contain the terminator yet - } - Err(_) => { - // Error parsing garbage - valid outcome - // This should happen when: - // 1. No terminator found within max garbage size - } - } + // Create realistic test case: fuzz_data + real_terminator + let mut realistic_input = data.to_vec(); + realistic_input.extend_from_slice(responder_terminator); + + // Test the receive_garbage function with realistic input + let _ = initiator.receive_garbage(&realistic_input); }); diff --git a/protocol/fuzz/fuzz_targets/receive_key.rs b/protocol/fuzz/fuzz_targets/receive_key.rs index 9301c23..8ae2dd1 100644 --- a/protocol/fuzz/fuzz_targets/receive_key.rs +++ b/protocol/fuzz/fuzz_targets/receive_key.rs @@ -7,37 +7,47 @@ #![no_main] use bip324::{Handshake, Initialized, Network, Role}; use libfuzzer_sys::fuzz_target; +use rand::SeedableRng; fuzz_target!(|data: &[u8]| { - // Skip if data is not exactly 64 bytes + // We need exactly 64 bytes for an ElligatorSwift key. + // This gives the fuzzer a clear signal about the expected input size. if data.len() != 64 { return; } - // Set up a handshake in the SentKey state - let handshake = Handshake::::new(Network::Bitcoin, Role::Initiator).unwrap(); + // Use a fixed seed to make the fuzzing deterministic. + // This ensures same input always produces same result. + let seed = [42u8; 32]; + let mut rng = rand::rngs::StdRng::from_seed(seed); + let secp = secp256k1::Secp256k1::signing_only(); + + // Set up a handshake in the SentKey state. + let handshake = + Handshake::::new_with_rng(Network::Bitcoin, Role::Initiator, &mut rng, &secp) + .unwrap(); let mut key_buffer = vec![0u8; Handshake::::send_key_len(None)]; let handshake = handshake.send_key(None, &mut key_buffer).unwrap(); - // Fuzz the receive_key function with arbitrary 64-byte data + // Convert the data to the required array format. let mut key_bytes = [0u8; 64]; key_bytes.copy_from_slice(data); match handshake.receive_key(key_bytes) { Ok(_handshake) => { - // Successfully processed the key - // This means: - // 1. The 64 bytes represent a valid ElligatorSwift encoding - // 2. The ECDH operation succeeded - // 3. The key derivation worked - // 4. It's not the V1 protocol magic bytes + // Successfully processed the key. + // + // 1. The 64 bytes represent a valid ElligatorSwift encoding. + // 2. The ECDH operation succeeded. + // 3. The key derivation worked. + // 4. It's not the V1 protocol magic bytes. } Err(_) => { - // Failed to process the key - // This could be: - // 1. Invalid ElligatorSwift encoding - // 2. V1 protocol detected (first 4 bytes match network magic) - // 3. ECDH or key derivation failure + // Failed to process the key. + // + // 1. Invalid ElligatorSwift encoding. + // 2. V1 protocol detected (first 4 bytes match network magic). + // 3. ECDH or key derivation failure. } } }); diff --git a/protocol/fuzz/fuzz_targets/receive_version.rs b/protocol/fuzz/fuzz_targets/receive_version.rs deleted file mode 100644 index bbb23fa..0000000 --- a/protocol/fuzz/fuzz_targets/receive_version.rs +++ /dev/null @@ -1,70 +0,0 @@ -// SPDX-License-Identifier: CC0-1.0 - -//! Fuzz test for the receive_version function. -//! -//! This focused test fuzzes only the version packet decryption logic. - -#![no_main] -use bip324::{GarbageResult, Handshake, Initialized, Network, ReceivedKey, Role, VersionResult}; -use libfuzzer_sys::fuzz_target; - -fuzz_target!(|data: &[u8]| { - // Skip if data is too small - if data.is_empty() { - return; - } - - // Set up a valid handshake in the ReceivedGarbage state - let initiator = Handshake::::new(Network::Bitcoin, Role::Initiator).unwrap(); - let mut initiator_key = vec![0u8; Handshake::::send_key_len(None)]; - let initiator = initiator.send_key(None, &mut initiator_key).unwrap(); - - let responder = Handshake::::new(Network::Bitcoin, Role::Responder).unwrap(); - let mut responder_key = vec![0u8; Handshake::::send_key_len(None)]; - let responder = responder.send_key(None, &mut responder_key).unwrap(); - - // Exchange keys - let initiator = initiator - .receive_key(responder_key[..64].try_into().unwrap()) - .unwrap(); - let responder = responder - .receive_key(initiator_key[..64].try_into().unwrap()) - .unwrap(); - - // Both send version packets - let mut initiator_version = vec![0u8; Handshake::::send_version_len(None)]; - let initiator = initiator - .send_version(&mut initiator_version, None) - .unwrap(); - - let mut responder_version = vec![0u8; Handshake::::send_version_len(None)]; - let _responder = responder - .send_version(&mut responder_version, None) - .unwrap(); - - // Process the responder's garbage terminator to get to ReceivedGarbage state - let (handshake, _consumed) = match initiator.receive_garbage(&responder_version) { - Ok(GarbageResult::FoundGarbage { - handshake, - consumed_bytes, - }) => (handshake, consumed_bytes), - _ => panic!("Should find garbage terminator in valid version buffer"), - }; - - // Now fuzz the receive_version function with arbitrary packet data - let mut packet_data = data.to_vec(); - match handshake.receive_version(&mut packet_data) { - Ok(VersionResult::Complete { cipher: _ }) => { - // Successfully completed handshake - // This should only happen with valid encrypted version packet - } - Ok(VersionResult::Decoy(_)) => { - // Received a decoy packet - // This should happen when packet type indicates decoy - } - Err(_) => { - // Decryption or authentication failed - // This is the most common outcome with random data - } - } -}); From 3189068bdc72aab076e0867db4824f2d23f1f684 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Thu, 10 Jul 2025 13:44:52 -0700 Subject: [PATCH 2/2] fix: cap the amount of garbage to search Cap a DOS vector by setting a limit on the amount of garbage to search for a terminator. --- protocol/src/handshake.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/protocol/src/handshake.rs b/protocol/src/handshake.rs index aa6f3f1..c04f669 100644 --- a/protocol/src/handshake.rs +++ b/protocol/src/handshake.rs @@ -472,7 +472,12 @@ impl Handshake { fn split_garbage<'b>(&self, buffer: &'b [u8]) -> Result<(&'b [u8], &'b [u8]), Error> { let terminator = &self.state.remote_garbage_terminator; - if let Some(index) = buffer + // Only search up to the maximum amount of garbage bytes + // allowed in the spec to avoid a DOS vector. + if let Some(index) = buffer[..core::cmp::min( + buffer.len(), + MAX_NUM_GARBAGE_BYTES + NUM_GARBAGE_TERMINTOR_BYTES, + )] .windows(terminator.len()) .position(|window| window == terminator) { @@ -846,32 +851,40 @@ mod tests { // Test split_garbage error conditions. // - // 1. NoGarbageTerminator - when buffer exceeds max size without finding terminator - // 2. CiphertextTooSmall - when buffer is too short to possibly contain terminator + // 1. NoGarbageTerminator - Buffer exceeds max size without finding terminator. + // 2. CiphertextTooSmall - Buffer is too short to possibly contain terminator.i #[test] fn test_handshake_split_garbage() { - // Create a handshake and bring it to the SentVersion state to test split_garbage + // Create a handshake and bring it to the SentVersion state to test split_garbage. let handshake = Handshake::::new(Network::Bitcoin, Role::Initiator).unwrap(); let mut buffer = vec![0u8; NUM_ELLIGATOR_SWIFT_BYTES]; let handshake = handshake.send_key(None, &mut buffer).unwrap(); - // Create a fake peer key to receive + // Create a fake peer key to receive. let fake_peer_key = [0u8; NUM_ELLIGATOR_SWIFT_BYTES]; let handshake = handshake.receive_key(fake_peer_key).unwrap(); - // Send version to get to SentVersion state + // Send version to get to SentVersion state. let mut version_buffer = vec![0u8; 1024]; let handshake = handshake.send_version(&mut version_buffer, None).unwrap(); - // Test with a buffer that is too long (should fail to find terminator) + // Test with a buffer that is too long (should fail to find terminator). let test_buffer = vec![0; MAX_NUM_GARBAGE_BYTES + NUM_GARBAGE_TERMINTOR_BYTES]; let result = handshake.split_garbage(&test_buffer); assert!(matches!(result, Err(Error::NoGarbageTerminator))); - // Test with a buffer that's just short of the required length + // Test with a buffer that's just short of the required length. let short_buffer = vec![0; MAX_NUM_GARBAGE_BYTES + NUM_GARBAGE_TERMINTOR_BYTES - 1]; let result = handshake.split_garbage(&short_buffer); assert!(matches!(result, Err(Error::CiphertextTooSmall))); + + // Even if terminator is present, it's beyond the search limit. + let mut oversized_buffer = vec![0xEE; 5000]; + oversized_buffer.extend_from_slice(&handshake.state.remote_garbage_terminator); + oversized_buffer.extend_from_slice(&[0xFF; 32]); + let result = handshake.split_garbage(&oversized_buffer); + // Should return NoGarbageTerminator because terminator is beyond search limit. + assert!(matches!(result, Err(Error::NoGarbageTerminator))); } // Test that receive_key detects V1 protocol when peer's key starts with network magic.