p2p: error on `CommandString` interior null bytes
What the developer wrote
Authored by yancy
p2p: error on `CommandString` interior null bytes
Add an invariant that a `CommandString` cannot have a null byte
preceding a non null byte as part of the command string.
What changed, and why it matters
This commit tightens validation for Bitcoin P2P message command strings. Previously, a command string could contain a null byte in the middle followed by more characters (for example "And\0rew"). Such malformed strings are now rejected both when created from Rust code and when received from the network. This prevents odd or potentially dangerous command strings from being accepted as valid.
Review downstream consumers of `CommandString` to ensure they do not rely on interior-null command strings. Consider whether the decoder error mapping (`NotAscii`) accurately describes the new failure mode, and add a dedicated error variant if needed. Otherwise, this is a defensive hardening change that should be merged and released normally.
Security signals we found
New input-validation invariant added to network-facing type
Rejects malformed command strings during deserialization
Prevents acceptance of command strings with embedded null bytes followed by data
Test expectations inverted from success to failure for interior-null inputs
Evidence from the diff
The patch adds an invariant to CommandString in p2p/src/message.rs: a 12-byte command buffer may not contain a null byte that precedes a later non-null byte. A new helper contains_interior_null checks whether, after the first zero byte, any subsequent byte is non-zero. The check is enforced in try_from_stringly (construction from &str/String) and in the decoder’s end function (network deserialization). Unit tests are updated to expect errors for interior-null command strings.
Changed components
p2p/src/message.rsCommandStringCommandString decoderInspect captured patch +24 / −6
diff --git a/p2p/src/message.rs b/p2p/src/message.rs
index edc3a292..36fbc064 100644
--- a/p2p/src/message.rs
+++ b/p2p/src/message.rs
@@ -56,6 +56,11 @@ impl CommandString {
/// The maximum length a [`CommandString`] can be once padding characters are trimmed.
pub const MAX_LEN: usize = 12;
+ // An interior null byte is a null the precedes a non-null char.
+ fn contains_interior_null(buf: [u8; 12]) -> bool {
+ buf.iter().skip_while(|&c| *c != 0).any(|c| *c != 0)
+ }
+
/// Create [`CommandString`].
///
/// # Parameters
@@ -66,13 +71,19 @@ impl CommandString {
///
/// - If `s` is more than 12 characters in length.
/// - If `s` has non-ascii characters.
+ /// - If `s` contains a null byte which preceeds a non null byte.
fn try_from_stringly<S: AsRef<str> + Into<String>>(s: S) -> Result<Self, CommandStringError> {
if !s.as_ref().is_ascii() || s.as_ref().len() > Self::MAX_LEN {
Err(CommandStringError(s.into()))
} else {
let mut buf = [0; Self::MAX_LEN];
buf[..s.as_ref().len()].copy_from_slice(s.as_ref().as_bytes());
- Ok(Self(buf))
+
+ if Self::contains_interior_null(buf) {
+ Err(CommandStringError(s.into()))
+ } else {
+ Ok(Self(buf))
+ }
}
}
}
@@ -152,7 +163,9 @@ crate::decoder_newtype! {
fn end(result: Result<[u8; 12], encoding::UnexpectedEofError>) -> Result<CommandString, CommandStringDecoderError> {
let bytes = result.map_err(CommandStringDecoderError::UnexpectedEof)?;
- if !bytes.is_ascii() {
+
+ let contains_interior_null = CommandString::contains_interior_null(bytes);
+ if !bytes.is_ascii() || contains_interior_null {
return Err(CommandStringDecoderError::NotAscii);
}
Ok(CommandString(bytes))
@@ -2521,12 +2534,11 @@ mod test {
assert_eq!(cs.as_ref().unwrap().to_string(), "Andrew".to_owned());
assert_eq!(cs.unwrap(), CommandString::try_from("Andrew").unwrap());
- // Test that embedded null bytes are preserved while trailing nulls are trimmed
+ // Test that a null ascii char cannot precede a non-null char.
let cs: Result<CommandString, _> =
encoding::decode_from_slice(&[0, 0x41u8, 0x6e, 0x64, 0, 0x72, 0x65, 0x77, 0, 0, 0, 0]);
- assert!(cs.is_ok());
- assert_eq!(cs.as_ref().unwrap().to_string(), "\0And\0rew".to_owned());
- assert_eq!(cs.unwrap(), CommandString::try_from("\0And\0rew").unwrap());
+ assert!(cs.is_err());
+ assert!(CommandString::try_from("And\0rew").is_err());
// Invalid CommandString, must be ASCII
assert!(encoding::decode_from_slice::<CommandString>(&[
@@ -2542,6 +2554,12 @@ mod test {
let s = "\u{1F980}";
assert!(CommandString::try_from(s).is_err());
+
+ let empty_buf = [0; 12];
+ assert!(!CommandString::contains_interior_null(empty_buf));
+
+ let full_buf = [1; 12];
+ assert!(!CommandString::contains_interior_null(full_buf));
}
#[test]
Why this scored 48/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.