Merge rust-bitcoin#4649: fix: preserve embedded nulls in CommandString for consensus validation

a6f63cf fix: validate ASCII and preserve embedded nulls in CommandString parsing (Erick Cestari)

Pull request description:

  Previously, `CommandString` decoding would silently normalize invalid commands containing embedded null bytes. For example, "mem\0pool" would be incorrectly decoded as "mempool", bypassing consensus validation.
  
  Found through differential fuzzing between btcd and rust-bitcoin, where rust-bitcoin was accepting messages with embedded null bytes in Command fields while btcd (and Bitcoin Core) correctly reject them.


ACKs for top commit:
  apoelstra:
    ACK a6f63cf; successfully ran local tests
  tcharding:
    ACK a6f63cf


Tree-SHA512: fccf7fc29fb3eac8ca0e3dd2e18d7fadbfdce913448d5e05f3b0c1e545993952a70680fc652acf6e5b7a4bc8b1b053622081f9af733e45d7c51ab47bc8675a1d

Author: apoelstra

p2p/src/message.rs

@@ -5,7 +5,7 @@
 //! This module defines the `NetworkMessage` and `RawNetworkMessage` types that
 //! are used for (de)serializing Bitcoin objects for transmission on the network.
 
-use core::{fmt, iter};
+use core::fmt;
 use std::borrow::{Cow, ToOwned};
 use std::boxed::Box;
 
@@ -114,14 +114,15 @@ impl Decodable for CommandString {
     #[inline]
     fn consensus_decode<R: BufRead + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
         let rawbytes: [u8; 12] = Decodable::consensus_decode(r)?;
-        let rv = iter::FromIterator::from_iter(rawbytes.iter().filter_map(|&u| {
-            if u > 0 {
-                Some(u as char)
-            } else {
-                None
-            }
-        }));
-        Ok(CommandString(rv))
+
+        // Find the last non-null byte and trim null padding from the end
+        let trimmed = &rawbytes[..rawbytes.iter().rposition(|&b| b != 0).map_or(0, |i| i + 1)];
+
+        if !trimmed.is_ascii() {
+            return Err(crate::consensus::parse_failed_error("Command string must be ASCII"));
+        }
+
+        Ok(CommandString(Cow::Owned(unsafe { String::from_utf8_unchecked(trimmed.to_vec()) })))
     }
 }
 
@@ -892,9 +893,18 @@ mod test {
         assert_eq!(cs.as_ref().unwrap().to_string(), "Andrew".to_owned());
         assert_eq!(cs.unwrap(), CommandString::try_from_static("Andrew").unwrap());
 
-        let short_cs: Result<CommandString, _> =
-            deserialize(&[0x41u8, 0x6e, 0x64, 0x72, 0x65, 0x77, 0, 0, 0, 0, 0]);
-        assert!(short_cs.is_err());
+        // Test that embedded null bytes are preserved while trailing nulls are trimmed
+        let cs: Result<CommandString, _> =
+            deserialize(&[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_static("\0And\0rew").unwrap());
+
+        // Invalid CommandString, must be ASCII
+        assert!(deserialize::<CommandString>(&[0, 0x41u8, 0x6e, 0xa4, 0, 0x72, 0x65, 0x77, 0, 0, 0, 0]).is_err());
+
+        // Invalid CommandString, must be 12 bytes
+        assert!(deserialize::<CommandString>(&[0x41u8, 0x6e, 0x64, 0x72, 0x65, 0x77, 0, 0, 0, 0, 0]).is_err());
     }
 
     #[test]