What the developer wrote
Authored by Mitchell Bagot
Prevent panic in UserAgent arbitrary
The current UserAgent arbitrary impl can panic when calling into
UserAgent::new, as it doesn't correctly sanitise the characters and
length of the name. While the decoder doesn't enforce these checks,
unexpected panics in the arbitrary impl are surprising and should be
avoided.
Prevent panics in UserAgent arbitrary impl by sanitising inputs to
UserAgent::new.
What changed, and why it matters
This commit fixes a panic (crash) in a test-only feature called 'arbitrary' that generates random fake user-agent strings for fuzz testing. The panic happened because the random generator could produce characters or lengths that the UserAgent constructor rejects. It is not a normal runtime bug in Bitcoin networking code, but it could cause fuzz tests or property-based tests to crash unexpectedly.
Low priority for production systems; update fuzzing/test dependencies to include this fix so fuzz campaigns are not interrupted by avoidable panics. Review other Arbitrary implementations for similar validation mismatches.
Security signals we found
Denial-of-service-like panic in test/fuzz generation path
Input sanitisation bypass in derived/test trait implementation
Length and character validation mismatch between Arbitrary and constructor
Evidence from the diff
The Arbitrary implementation for UserAgent in p2p/src/message_network.rs previously called UserAgent::new with a raw arbitrary String and arbitrary version. UserAgent::new sanitises/validates the name (forbidding ‘/’, ‘(‘, ‘)’, ‘:’ and enforcing a maximum length including version overhead), and could panic on invalid input. The patch sanitises the arbitrary string by filtering forbidden characters and truncating to the remaining length budget before calling UserAgent::new, preventing panics during fuzz/property-test generation.
Changed components
p2p/src/message_network.rsUserAgent::arbitrary implementation (behind 'arbitrary' feature flag)Inspect captured patch +13 / −1
diff --git a/p2p/src/message_network.rs b/p2p/src/message_network.rs
index c1b10674..e8ccd5d9 100644
--- a/p2p/src/message_network.rs
+++ b/p2p/src/message_network.rs
@@ -778,7 +778,19 @@ impl<'a> Arbitrary<'a> for UserAgentVersion {
#[cfg(feature = "arbitrary")]
impl<'a> Arbitrary<'a> for UserAgent {
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
- Ok(Self::new(u.arbitrary::<String>()?, &u.arbitrary()?))
+ let version = UserAgentVersion::arbitrary(u)?;
+
+ let mut name: String = u
+ .arbitrary::<String>()?
+ .chars()
+ .filter(|c| !matches!(c, '/' | '(' | ')' | ':'))
+ .collect();
+
+ let overhead = 3 + version.to_string().chars().count();
+ let max_name = Self::MAX_USER_AGENT_LEN - overhead;
+ name.truncate(max_name);
+
+ Ok(Self::new(name, &version))
}
}
Why this scored 24/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.