Merge rust-bitcoin#4230: Fix BIP32 validation for private keys and master key constraints (rust-bitcoin#4195)

8f74b82 Add validation for private key format and master key constraints (Erick Cestari)

Pull request description:

  This PR addresses issue rust-bitcoin#4195 by adding proper validation when decoding extended private keys:

  ### Changes

  - Add validation to ensure byte 45 is zero as required by BIP-32 specification for private keys
  - For master keys (depth=0), add validation to ensure parent fingerprint is zero
  - For master keys (depth=0), add validation to ensure child number is zero
  - Add corresponding error types to handle these validation failures
  - Add unit tests to verify each validation rule

  ### Validation Rationale
  These checks improve security by rejecting malformed extended keys that could potentially lead to unexpected behavior. As noted in the issue discussion, these validations are explicitly required by the BIP-32 specification.

  ### Testing
  Added three new unit tests to verify each validation rule:

  - test_reject_xpriv_with_non_zero_byte_at_index_45
  - test_reject_xpriv_with_zero_depth_and_non_zero_index
  - test_reject_xpriv_with_zero_depth_and_non_zero_parent_fingerprint

  Fixes rust-bitcoin#4195

ACKs for top commit:
  jrakibi:
    ACK 8f74b82
  tcharding:
    ACK 8f74b82
  apoelstra:
    ACK 8f74b82; successfully ran local tests

Tree-SHA512: 6a013e4917f83cfd7e39a2a18f7491853d791ab1d981a99eeea6204e1dab723fed7a168ff2a89e8850d512c3c381bfa1afef7fa32e5a0d246d949a46b01a3023

Author: apoelstra

bitcoin/src/bip32.rs

@@ -518,6 +518,12 @@ pub enum Error {
     InvalidPublicKeyHexLength(usize),
     /// Base58 decoded data was an invalid length.
     InvalidBase58PayloadLength(InvalidBase58PayloadLengthError),
+    /// Invalid private key prefix (byte 45 must be 0)
+    InvalidPrivateKeyPrefix,
+    /// Non-zero parent fingerprint for a master key (depth 0)
+    NonZeroParentFingerprintForMasterKey,
+    /// Non-zero child number for a master key (depth 0)
+    NonZeroChildNumberForMasterKey,
 }
 
 impl From<Infallible> for Error {
@@ -544,6 +550,11 @@ impl fmt::Display for Error {
             InvalidPublicKeyHexLength(got) =>
                 write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got),
             InvalidBase58PayloadLength(ref e) => write_err!(f, "base58 payload"; e),
+            InvalidPrivateKeyPrefix =>
+                f.write_str("invalid private key prefix, byte 45 must be 0 as required by BIP-32"),
+            NonZeroParentFingerprintForMasterKey =>
+                f.write_str("non-zero parent fingerprint in master key"),
+            NonZeroChildNumberForMasterKey => f.write_str("non-zero child number in master key"),
         }
     }
 }
@@ -565,6 +576,9 @@ impl std::error::Error for Error {
             | UnknownVersion(_)
             | WrongExtendedKeyLength(_)
             | InvalidPublicKeyHexLength(_) => None,
+            InvalidPrivateKeyPrefix => None,
+            NonZeroParentFingerprintForMasterKey => None,
+            NonZeroChildNumberForMasterKey => None,
         }
     }
 }
@@ -699,6 +713,23 @@ impl Xpriv {
             return Err(Error::UnknownVersion([b0, b1, b2, b3]));
         };
 
+        // Check that byte 45 is 0 as required by BIP-32 for private keys
+        if data[45] != 0 {
+            return Err(Error::InvalidPrivateKeyPrefix);
+        }
+
+        // For master keys (depth=0), both parent fingerprint and child number must be zero
+        let depth = data[4];
+        if depth == 0 {
+            if !data[5..9].iter().all(|&b| b == 0) {
+                return Err(Error::NonZeroParentFingerprintForMasterKey);
+            }
+
+            if !data[9..13].iter().all(|&b| b == 0) {
+                return Err(Error::NonZeroChildNumberForMasterKey);
+            }
+        }
+
         Ok(Xpriv {
             network,
             depth: data[4],
@@ -1235,6 +1266,44 @@ mod tests {
                   "xpub68NZiKmJWnxxS6aaHmn81bvJeTESw724CRDs6HbuccFQN9Ku14VQrADWgqbhhTHBaohPX4CjNLf9fq9MYo6oDaPPLPxSb7gwQN3ih19Zm4Y");
     }
 
+    #[test]
+    fn test_reject_xpriv_with_non_zero_byte_at_index_45() {
+        let mut xpriv = base58::decode_check("xprv9wSp6B7kry3Vj9m1zSnLvN3xH8RdsPP1Mh7fAaR7aRLcQMKTR2vidYEeEg2mUCTAwCd6vnxVrcjfy2kRgVsFawNzmjuHc2YmYRmagcEPdU9").unwrap();
+
+        // Modify byte at index 45 to be non-zero (e.g., 1)
+        xpriv[45] = 1;
+
+        let result = Xpriv::decode(&xpriv);
+        assert!(result.is_err());
+
+        match result {
+            Err(Error::InvalidPrivateKeyPrefix) => {}
+            _ => panic!("Expected InvalidPrivateKeyPrefix error, got {:?}", result),
+        }
+    }
+
+    #[test]
+    fn test_reject_xpriv_with_zero_depth_and_non_zero_index() {
+        let result = "xprv9s21ZrQH4r4TsiLvyLXqM9P7k1K3EYhA1kkD6xuquB5i39AU8KF42acDyL3qsDbU9NmZn6MsGSUYZEsuoePmjzsB3eFKSUEh3Gu1N3cqVUN".parse::<Xpriv>();
+        assert!(result.is_err());
+
+        match result {
+            Err(Error::NonZeroChildNumberForMasterKey) => {}
+            _ => panic!("Expected NonZeroChildNumberForMasterKey error, got {:?}", result),
+        }
+    }
+
+    #[test]
+    fn test_reject_xpriv_with_zero_depth_and_non_zero_parent_fingerprint() {
+        let result = "xprv9s2SPatNQ9Vc6GTbVMFPFo7jsaZySyzk7L8n2uqKXJen3KUmvQNTuLh3fhZMBoG3G4ZW1N2kZuHEPY53qmbZzCHshoQnNf4GvELZfqTUrcv".parse::<Xpriv>();
+        assert!(result.is_err());
+
+        match result {
+            Err(Error::NonZeroParentFingerprintForMasterKey) => {}
+            _ => panic!("Expected NonZeroParentFingerprintForMasterKey error, got {:?}", result),
+        }
+    }
+
     #[test]
     #[cfg(feature = "serde")]
     pub fn encode_decode_childnumber() {