Repository: rust-bitcoin/rust-bitcoin (language: rust)

Issue:
`PrivateKey::from_wif()` can return `Ok` on invalid private keys
For private WIF keys corresponding to a compressed address, the last byte of the key needs to be `0x01`, but the API doesn't enforce this when using `PrivateKey::from_wif()`. So, invalid keys can be accepted. Below are a couple example "bad" private keys this API currently returns `Ok` on.

`L2x4uC2YgfFWZm9tF4pjDnVR6nJkheizFhEr2KvDNnTEmEqVzPJY`
`L4pjDnVR6nJkheizFhEEgdfBX1xxjHagbpgVFa73xHJh2jTaU8YS`
`KzuqXc7xLsrjeCc7ESSZpBUm2obWXEEp5w5C6VsvkWdj8avD2bL4`

Not sure what the philosophy of the project is here, and it doesn't break my use case. But I thought I'd at least open an issue for it. I think this function can also be made to accept base58 keys which don't start with valid characters, though I haven't spent the time fuzzing to see if I can make it happen.


Discussion / hints:
How could `PrivateKey::from_wif` know whether the key is supposed to be compressed?
> How could `PrivateKey::from_wif` know whether the key is supposed to be compressed?

Uh, it already knows...? You can tell by the length. Here's a snippet from the library itself:

```rs
let compressed = match data.len() {
    33 => false,
    34 => true,
    length => {
        return Err(InvalidBase58PayloadLengthError { length }.into());
    }
};
```

That extra 34th byte should be `0x01` if this is a valid key.
I think @MCFX2 may be correct.

> If the private key corresponded to a compressed public key, also drop the last byte (it should be 0x01).

ref: https://en.bitcoin.it/wiki/Wallet_import_format

"it should be 0x01" kinda implies if its not the WIF string is invalid but its not that strongly worded?

Although there doesn't seem to be a WIF BIP so is the format even defined by that link above? What is the "spec" for wallet stuff like this if there is no BIP?
Ah, I understand. I think we should be strict here and enforce that the final byte is 1.

@tcharding the "spec" is that wiki page. It's pretty bad. But this stuff is all pretty old and deprecated and we should make our code that handles it as tight as possible.
Nice, thanks for raising the issue @MCFX2! 

Fix patch:
diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs
index 9e9f1aa8c7..9ba23615a9 100644
--- a/bitcoin/src/crypto/key.rs
+++ b/bitcoin/src/crypto/key.rs
@@ -497,7 +497,12 @@ impl PrivateKey {
 
         let compressed = match data.len() {
             33 => false,
-            34 => true,
+            34 => {
+                if data[33] != 1 {
+                    return Err(InvalidWifCompressionFlagError{ invalid: data[33] }.into());
+                }
+                true
+            },
             length => {
                 return Err(InvalidBase58PayloadLengthError { length }.into());
             }
@@ -963,6 +968,8 @@ pub enum FromWifError {
     InvalidAddressVersion(InvalidAddressVersionError),
     /// A secp256k1 error.
     Secp256k1(secp256k1::Error),
+    /// Invalid WIF compression flag.
+    InvalidWifCompressionFlag(InvalidWifCompressionFlagError),
 }
 
 impl From<Infallible> for FromWifError {
@@ -980,6 +987,8 @@ impl fmt::Display for FromWifError {
             InvalidAddressVersion(ref e) =>
                 write_err!(f, "decoded base58 data contained an invalid address version btye"; e),
             Secp256k1(ref e) => write_err!(f, "private key validation failed"; e),
+            InvalidWifCompressionFlag(ref e) => 
+                write_err!(f, "invalid WIF compression flag";e),
         }
     }
 }
@@ -994,6 +1003,7 @@ impl std::error::Error for FromWifError {
             InvalidBase58PayloadLength(ref e) => Some(e),
             InvalidAddressVersion(ref e) => Some(e),
             Secp256k1(ref e) => Some(e),
+            InvalidWifCompressionFlag(ref e) => Some(e),
         }
     }
 }
@@ -1016,6 +1026,12 @@ impl From<InvalidAddressVersionError> for FromWifError {
     fn from(e: InvalidAddressVersionError) -> FromWifError { Self::InvalidAddressVersion(e) }
 }
 
+impl From<InvalidWifCompressionFlagError> for FromWifError {
+    fn from(e: InvalidWifCompressionFlagError) -> FromWifError {
+        Self::InvalidWifCompressionFlag(e)
+    }
+}
+
 /// Error returned while constructing public key from string.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum ParsePublicKeyError {
@@ -1161,6 +1177,27 @@ impl fmt::Display for InvalidAddressVersionError {
 #[cfg(feature = "std")]
 impl std::error::Error for InvalidAddressVersionError {}
 
+/// Invalid compression flag for a WIF key
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct InvalidWifCompressionFlagError{
+    /// The invalid compression flag.
+    pub(crate) invalid: u8,
+}
+
+impl InvalidWifCompressionFlagError {
+    /// Returns the invalid compression flag.
+    pub fn invalid_compression_flag(&self) -> u8 { self.invalid }
+}
+
+impl fmt::Display for InvalidWifCompressionFlagError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "invalid WIF compression flag. Expected a 0x01 byte at the end of the key but found: {}", self.invalid)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidWifCompressionFlagError {}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -1168,6 +1205,11 @@ mod tests {
 
     #[test]
     fn key_derivation() {
+        // mainnet compressed WIF with invalid compression flag.
+        let sk =
+        PrivateKey::from_wif("L2x4uC2YgfFWZm9tF4pjDnVR6nJkheizFhEr2KvDNnTEmEqVzPJY");
+        assert!(matches!(sk, Err(FromWifError::InvalidWifCompressionFlag(InvalidWifCompressionFlagError { invalid: 49 }))));
+
         // testnet compressed
         let sk =
             PrivateKey::from_wif("cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy").unwrap();


Test patch: