Merge #829: Don't allow uncompressed public keys without prefix 0x04

c0d36ef Don't allow uncompressed public keys without prefix 0x04 (Noah Lanson)

Pull request description:

  Was following #520 and through it was a quick fix that I could do:

  #### Changes:
  - If an uncompressed public key doesn't have prefix 0x04 in `PublicKey::from_slice()`, an error is returned.

  <br>

  I was wondering if `PublicKey::from_str()` should also enforce the same rules, however I have not incuded this in the PR.

  Please let me know if any changes need to be made.

  Thanks

ACKs for top commit:
  Kixunil:
    ACK c0d36ef
  apoelstra:
    ACK c0d36ef
  sanket1729:
    utACK c0d36ef. Not thrilled about the error message expecting len 66, when it can be both 66/130. But can live with it

Tree-SHA512: cfbcd569691c9a7f69ee775ec530605f42e988470a2ff9c28b4c881cec6b259053bb2288818e00b6f6b20316b1fb30fecc0b9a240ebbe7618f202ef6b5efeb9b

src/util/base58.rs

@@ -22,7 +22,7 @@ use prelude::*;
 
 use core::{fmt, str, iter, slice};
 
-use hashes::{sha256d, Hash};
+use hashes::{sha256d, Hash, hex};
 use secp256k1;
 
 use util::{endian, key};
@@ -46,6 +46,9 @@ pub enum Error {
     TooShort(usize),
     /// Secp256k1 error while parsing a secret key
     Secp256k1(secp256k1::Error),
+    /// Hex decoding error
+    // TODO: Remove this as part of crate-smashing, there should not be any key related errors in this module
+    Hex(hex::Error)
 }
 
 impl fmt::Display for Error {
@@ -58,6 +61,7 @@ impl fmt::Display for Error {
             Error::InvalidExtendedKeyVersion(ref v) => write!(f, "extended key version {:#04x?} is invalid for this base58 type", v),
             Error::TooShort(_) => write!(f, "base58ck data not even long enough for a checksum"),
             Error::Secp256k1(ref e) => fmt::Display::fmt(&e, f),
+            Error::Hex(ref e) => write!(f, "Hexadecimal decoding error: {}", e)
         }
     }
 }
@@ -255,6 +259,8 @@ impl From<key::Error> for Error {
         match e {
             key::Error::Secp256k1(e) => Error::Secp256k1(e),
             key::Error::Base58(e) => e,
+            key::Error::InvalidKeyPrefix(_) => Error::Secp256k1(secp256k1::Error::InvalidPublicKey),
+            key::Error::Hex(e) => Error::Hex(e)
         }
     }
 }

src/util/bip32.rs

@@ -25,7 +25,7 @@ use core::{fmt, str::FromStr, default::Default};
 #[cfg(feature = "serde")] use serde;
 
 use hash_types::XpubIdentifier;
-use hashes::{sha512, Hash, HashEngine, Hmac, HmacEngine};
+use hashes::{sha512, Hash, HashEngine, Hmac, HmacEngine, hex};
 use secp256k1::{self, Secp256k1, XOnlyPublicKey};
 
 use network::constants::Network;
@@ -461,7 +461,9 @@ pub enum Error {
     /// Encoded extended key data has wrong length
     WrongExtendedKeyLength(usize),
     /// Base58 encoding error
-    Base58(base58::Error)
+    Base58(base58::Error),
+    /// Hexadecimal decoding error
+    Hex(hex::Error)
 }
 
 impl fmt::Display for Error {
@@ -475,6 +477,7 @@ impl fmt::Display for Error {
             Error::UnknownVersion(ref bytes) => write!(f, "unknown version magic bytes: {:?}", bytes),
             Error::WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data has wrong length {}", len),
             Error::Base58(ref err) => write!(f, "base58 encoding error: {}", err),
+            Error::Hex(ref e) => write!(f, "Hexadecimal decoding error: {}", e)
         }
     }
 }
@@ -496,6 +499,8 @@ impl From<key::Error> for Error {
         match err {
             key::Error::Base58(e) => Error::Base58(e),
             key::Error::Secp256k1(e) => Error::Secp256k1(e),
+            key::Error::InvalidKeyPrefix(_) => Error::Secp256k1(secp256k1::Error::InvalidPublicKey),
+            key::Error::Hex(e) => Error::Hex(e)
         }
     }
 }

src/util/key.rs

@@ -27,7 +27,7 @@ use io;
 
 use secp256k1::{self, Secp256k1};
 use network::constants::Network;
-use hashes::{Hash, hash160};
+use hashes::{Hash, hash160, hex, hex::FromHex};
 use hash_types::{PubkeyHash, WPubkeyHash};
 use util::base58;
 
@@ -39,6 +39,10 @@ pub enum Error {
     Base58(base58::Error),
     /// secp256k1-related error
     Secp256k1(secp256k1::Error),
+    /// Invalid key prefix error
+    InvalidKeyPrefix(u8),
+    /// Hex decoding error
+    Hex(hex::Error)
 }
 
 
@@ -47,6 +51,8 @@ impl fmt::Display for Error {
         match *self {
             Error::Base58(ref e) => write!(f, "Key base58 error: {}", e),
             Error::Secp256k1(ref e) => write!(f, "Key secp256k1 error: {}", e),
+            Error::InvalidKeyPrefix(ref e) => write!(f, "Key prefix invalid: {}", e),
+            Error::Hex(ref e) => write!(f, "Key hex decoding error: {}", e)
         }
     }
 }
@@ -58,6 +64,8 @@ impl ::std::error::Error for Error {
         match *self {
             Error::Base58(ref e) => Some(e),
             Error::Secp256k1(ref e) => Some(e),
+            Error::InvalidKeyPrefix(_) => None,
+            Error::Hex(ref e) => Some(e)
         }
     }
 }
@@ -76,6 +84,13 @@ impl From<secp256k1::Error> for Error {
     }
 }
 
+#[doc(hidden)]
+impl From<hex::Error> for Error {
+    fn from(e: hex::Error) -> Self {
+        Error::Hex(e)
+    }
+}
+
 
 /// A Bitcoin ECDSA public key
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -158,6 +173,8 @@ impl PublicKey {
             let reason = match e {
                 Error::Base58(_) => "base58 error",
                 Error::Secp256k1(_) => "secp256k1 error",
+                Error::InvalidKeyPrefix(_) => "invalid key prefix",
+                Error::Hex(_) => "hex decoding error"
             };
             io::Error::new(io::ErrorKind::InvalidData, reason)
         })
@@ -178,6 +195,10 @@ impl PublicKey {
             len =>  { return Err(base58::Error::InvalidLength(len).into()); },
         };
 
+        if !compressed && data[0] != 0x04 {
+            return Err(Error::InvalidKeyPrefix(data[0]))
+        }
+
         Ok(PublicKey {
             compressed,
             inner: secp256k1::PublicKey::from_slice(data)?,
@@ -208,11 +229,11 @@ impl fmt::Display for PublicKey {
 impl FromStr for PublicKey {
     type Err = Error;
     fn from_str(s: &str) -> Result<PublicKey, Error> {
-        let key = secp256k1::PublicKey::from_str(s)?;
-        Ok(PublicKey {
-            inner: key,
-            compressed: s.len() == 66
-        })
+        match s.len() {
+            66 => PublicKey::from_slice(&<[u8; 33]>::from_hex(s)?),
+            130 => PublicKey::from_slice(&<[u8; 65]>::from_hex(s)?),
+            len => return Err(Error::Hex(hex::Error::InvalidLength(66, len)))
+        }
     }
 }