[minor API BREAK] Add unit tests to cover all error cases

This comes with a couple bugfixes and the following API changes:

  - Secp256k1::sign and ::sign_compact no longer return Result;
    it is impossible to trigger their failure modes with safe
    code since the `Message` and `SecretKey` types validate when
    they are created.

  - constants::MAX_COMPACT_SIGNATURE_SIZE loses the MAX_; signatures
    are always constant size

  - the Debug output for everything is now hex-encoded rather than
    being a list of base-10 ints. It's just easier to read this way.

kcov v26 now reports 100% test coverage; however, this does not
guarantee that test coverage is actually complete. Patches are
always welcome for improved unit tests.

src/constants.rs

@@ -31,7 +31,7 @@ pub const COMPRESSED_PUBLIC_KEY_SIZE: usize = 33;
 pub const MAX_SIGNATURE_SIZE: usize = 72;
 
 /// The maximum size of a compact signature
-pub const MAX_COMPACT_SIGNATURE_SIZE: usize = 64;
+pub const COMPACT_SIGNATURE_SIZE: usize = 64;
 
 /// The order of the secp256k1 curve
 pub const CURVE_ORDER: [u8; 32] = [

src/key.rs

@@ -158,6 +158,10 @@ impl PublicKey {
             constants::UNCOMPRESSED_PUBLIC_KEY_SIZE => {
                 let mut ret = [0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE];
                 unsafe {
+                    if ffi::secp256k1_ec_pubkey_verify(secp.ctx, data.as_ptr(),
+                                                       data.len() as ::libc::c_int) == 0 {
+                        return Err(InvalidPublicKey);
+                    }
                     copy_nonoverlapping(data.as_ptr(),
                                         ret.as_mut_ptr(),
                                         data.len());
@@ -242,7 +246,10 @@ impl PartialEq for PublicKeyData {
 
 impl fmt::Debug for PublicKeyData {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        (&self[..]).fmt(f)
+        for i in self[..].iter().cloned() {
+            try!(write!(f, "{:02x}", i));
+        }
+        Ok(())
     }
 }
 
@@ -451,7 +458,11 @@ impl Serialize for PublicKey {
 
 impl fmt::Debug for SecretKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
-        (&self[..]).fmt(f)
+        try!(write!(f, "SecretKey("));
+        for i in self[..].iter().cloned() {
+            try!(write!(f, "{:02x}", i));
+        }
+        write!(f, ")")
     }
 }
 
@@ -460,6 +471,9 @@ mod test {
     use super::super::Secp256k1;
     use super::super::Error::{InvalidPublicKey, InvalidSecretKey};
     use super::{PublicKey, SecretKey};
+    use super::super::constants;
+
+    use rand::Rng;
 
     #[test]
     fn skey_from_slice() {
@@ -520,6 +534,41 @@ mod test {
                                         0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x41]).is_err());
     }
 
+    #[test]
+    fn test_bad_deserialize() {
+        use std::io::Cursor;
+        use serialize::{json, Decodable};
+
+        let zero31 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let json31 = json::Json::from_reader(&mut Cursor::new(zero31)).unwrap();
+        let zero32 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let json32 = json::Json::from_reader(&mut Cursor::new(zero32)).unwrap();
+        let zero65 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let json65 = json::Json::from_reader(&mut Cursor::new(zero65)).unwrap();
+        let string = "\"my key\"".as_bytes();
+        let json = json::Json::from_reader(&mut Cursor::new(string)).unwrap();
+
+        // Invalid length
+        let mut decoder = json::Decoder::new(json31.clone());
+        assert!(<PublicKey as Decodable>::decode(&mut decoder).is_err());
+        let mut decoder = json::Decoder::new(json31.clone());
+        assert!(<SecretKey as Decodable>::decode(&mut decoder).is_err());
+        let mut decoder = json::Decoder::new(json32.clone());
+        assert!(<PublicKey as Decodable>::decode(&mut decoder).is_err());
+        let mut decoder = json::Decoder::new(json32.clone());
+        assert!(<SecretKey as Decodable>::decode(&mut decoder).is_ok());
+        let mut decoder = json::Decoder::new(json65.clone());
+        assert!(<PublicKey as Decodable>::decode(&mut decoder).is_ok());
+        let mut decoder = json::Decoder::new(json65.clone());
+        assert!(<SecretKey as Decodable>::decode(&mut decoder).is_err());
+
+        // Syntax error
+        let mut decoder = json::Decoder::new(json.clone());
+        assert!(<PublicKey as Decodable>::decode(&mut decoder).is_err());
+        let mut decoder = json::Decoder::new(json.clone());
+        assert!(<SecretKey as Decodable>::decode(&mut decoder).is_err());
+    }
+
     #[test]
     fn test_serialize() {
         use std::io::Cursor;
@@ -551,6 +600,38 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_bad_serde_deserialize() {
+        use serde::{json, Deserialize};
+
+        // Invalid length
+        let zero31 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let mut json = json::de::Deserializer::new(zero31.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<PublicKey as Deserialize>::deserialize(&mut json).is_err());
+        let mut json = json::de::Deserializer::new(zero31.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<SecretKey as Deserialize>::deserialize(&mut json).is_err());
+
+        let zero32 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let mut json = json::de::Deserializer::new(zero32.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<PublicKey as Deserialize>::deserialize(&mut json).is_err());
+        let mut json = json::de::Deserializer::new(zero32.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<SecretKey as Deserialize>::deserialize(&mut json).is_ok());
+
+        let zero65 = "[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]".as_bytes();
+        let mut json = json::de::Deserializer::new(zero65.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<PublicKey as Deserialize>::deserialize(&mut json).is_ok());
+        let mut json = json::de::Deserializer::new(zero65.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<SecretKey as Deserialize>::deserialize(&mut json).is_err());
+
+        // Syntax error
+        let string = "\"my key\"".as_bytes();
+        let mut json = json::de::Deserializer::new(string.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<PublicKey as Deserialize>::deserialize(&mut json).is_err());
+        let mut json = json::de::Deserializer::new(string.iter().map(|c| Ok(*c))).unwrap();
+        assert!(<SecretKey as Deserialize>::deserialize(&mut json).is_err());
+    }
+
+
     #[test]
     fn test_serialize_serde() {
         use serde::{json, Serialize, Deserialize};
@@ -580,6 +661,83 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_out_of_range() {
+
+        struct BadRng(u8);
+        impl Rng for BadRng {
+            fn next_u32(&mut self) -> u32 { unimplemented!() }
+            // This will set a secret key to a little over the
+            // group order, then decrement with repeated calls
+            // until it returns a valid key
+            fn fill_bytes(&mut self, data: &mut [u8]) {
+                let group_order: [u8; 32] = [
+                    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe,
+                    0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b,
+                    0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41];
+                assert_eq!(data.len(), 32);
+                unsafe {
+                    use std::intrinsics::copy_nonoverlapping;
+                    copy_nonoverlapping(group_order.as_ptr(),
+                                        data.as_mut_ptr(),
+                                        32);
+                }
+                data[31] = self.0;
+                self.0 -= 1;
+            }
+        }
+
+        let mut s = Secp256k1::with_rng(BadRng(0xff));
+        s.generate_keypair(false);
+        let mut s = Secp256k1::with_rng(BadRng(0xff));
+        s.generate_keypair(true);
+    }
+
+    #[test]
+    fn test_pubkey_from_bad_slice() {
+        let s = Secp256k1::new_deterministic();
+        // Bad sizes
+        assert_eq!(PublicKey::from_slice(&s, &[0; constants::COMPRESSED_PUBLIC_KEY_SIZE - 1]),
+                   Err(InvalidPublicKey));
+        assert_eq!(PublicKey::from_slice(&s, &[0; constants::COMPRESSED_PUBLIC_KEY_SIZE + 1]),
+                   Err(InvalidPublicKey));
+        assert_eq!(PublicKey::from_slice(&s, &[0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE - 1]),
+                   Err(InvalidPublicKey));
+        assert_eq!(PublicKey::from_slice(&s, &[0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE + 1]),
+                   Err(InvalidPublicKey));
+
+        // Bad parse
+        assert_eq!(PublicKey::from_slice(&s, &[0xff; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE]),
+                   Err(InvalidPublicKey));
+        assert_eq!(PublicKey::from_slice(&s, &[0x55; constants::COMPRESSED_PUBLIC_KEY_SIZE]),
+                   Err(InvalidPublicKey));
+    }
+
+    #[test]
+    fn test_debug_output() {
+        struct DumbRng(u32);
+        impl Rng for DumbRng {
+            fn next_u32(&mut self) -> u32 {
+                self.0 = self.0.wrapping_add(1);
+                self.0
+            }
+        }
+
+        let mut s = Secp256k1::with_rng(DumbRng(0));
+        let (sk1, pk1) = s.generate_keypair(false);
+        let (sk2, pk2) = s.generate_keypair(true);
+
+        assert_eq!(&format!("{:?}", sk1),
+                   "SecretKey(0200000001000000040000000300000006000000050000000800000007000000)");
+        assert_eq!(&format!("{:?}", pk1),
+                   "PublicKey(049510c48c265cefb3413be0e6b75beef02ebafcaf6634f962b27b4832abc4feec01bd8ff2e31057f7b7a244ed8c5ccd9781a63a6f607b40b493330cd159ecd5ce)");
+        assert_eq!(&format!("{:?}", sk2),
+                   "SecretKey(0a000000090000000c0000000b0000000e0000000d000000100000000f000000)");
+        assert_eq!(&format!("{:?}", pk2),
+                   "PublicKey(024889f1f4a9407f8588b55358c2b392a6d9662872d5b9fff98b6f68c5e290a866)");
+    }
+
     #[test]
     fn test_addition() {
         let mut s = Secp256k1::new().unwrap();

src/lib.rs

@@ -221,8 +221,6 @@ pub enum Error {
     InvalidSignature,
     /// Bad secret key
     InvalidSecretKey,
-    /// Signing failed: bad nonce, bad privkey or signature was too small
-    SignFailed,
     /// Boolean-returning function returned the wrong boolean
     Unknown
 }
@@ -317,36 +315,36 @@ impl<R> Secp256k1<R> {
 
     /// Constructs a signature for `msg` using the secret key `sk` and nonce `nonce`
     pub fn sign(&self, msg: &Message, sk: &key::SecretKey)
-                -> Result<Signature, Error> {
+                -> Signature {
         let mut sig = [0; constants::MAX_SIGNATURE_SIZE];
         let mut len = constants::MAX_SIGNATURE_SIZE as c_int;
         unsafe {
-            if ffi::secp256k1_ecdsa_sign(self.ctx, msg.as_ptr(), sig.as_mut_ptr(),
+            // We can assume the return value because it's not possible to construct
+            // an invalid signature from a valid `Message` and `SecretKey`
+            assert_eq!(ffi::secp256k1_ecdsa_sign(self.ctx, msg.as_ptr(), sig.as_mut_ptr(),
                                                  &mut len, sk.as_ptr(),
                                                  ffi::secp256k1_nonce_function_rfc6979,
-                                         ptr::null()) != 1 {
+                                                 ptr::null()), 1);
-                return Err(Error::SignFailed);
-            }
             // This assertation is probably too late :)
             debug_assert!(len as usize <= constants::MAX_SIGNATURE_SIZE);
-        };
+        }
-        Ok(Signature(len as usize, sig))
+        Signature(len as usize, sig)
     }
 
     /// Constructs a compact signature for `msg` using the secret key `sk`
     pub fn sign_compact(&self, msg: &Message, sk: &key::SecretKey)
-                        -> Result<(Signature, RecoveryId), Error> {
+                        -> (Signature, RecoveryId) {
         let mut sig = [0; constants::MAX_SIGNATURE_SIZE];
         let mut recid = 0;
         unsafe {
-            if ffi::secp256k1_ecdsa_sign_compact(self.ctx, msg.as_ptr(),
+            // We can assume the return value because it's not possible to construct
+            // an invalid signature from a valid `Message` and `SecretKey`
+            assert_eq!(ffi::secp256k1_ecdsa_sign_compact(self.ctx, msg.as_ptr(),
                                                          sig.as_mut_ptr(), sk.as_ptr(),
                                                          ffi::secp256k1_nonce_function_default,
-                                                 ptr::null(), &mut recid) != 1 {
+                                                         ptr::null(), &mut recid), 1);
-                return Err(Error::SignFailed);
         }
-        };
+        (Signature(constants::COMPACT_SIGNATURE_SIZE, sig), RecoveryId(recid))
-        Ok((Signature(constants::MAX_COMPACT_SIGNATURE_SIZE, sig), RecoveryId(recid)))
     }
 
     /// Determines the public key for which `sig` is a valid signature for
@@ -357,6 +355,9 @@ impl<R> Secp256k1<R> {
         let mut pk = key::PublicKey::new(compressed);
         let RecoveryId(recid) = recid;
 
+        if sig.len() != constants::COMPACT_SIGNATURE_SIZE {
+            return Err(Error::InvalidSignature);
+        }
         unsafe {
             let mut len = 0;
             if ffi::secp256k1_ecdsa_recover_compact(self.ctx, msg.as_ptr(),
@@ -400,14 +401,14 @@ impl<R> Secp256k1<R> {
 
 #[cfg(test)]
 mod tests {
-    use std::iter::repeat;
     use rand::{Rng, thread_rng};
 
     use test::{Bencher, black_box};
 
-    use key::PublicKey;
+    use key::{SecretKey, PublicKey};
-    use super::{Secp256k1, Signature, Message};
+    use super::constants;
-    use super::Error::{InvalidPublicKey, IncorrectSignature, InvalidSignature};
+    use super::{Secp256k1, Signature, Message, RecoveryId};
+    use super::Error::{InvalidMessage, InvalidPublicKey, IncorrectSignature, InvalidSignature};
 
     #[test]
     fn invalid_pubkey() {
@@ -450,31 +451,40 @@ mod tests {
 
     #[test]
     fn sign() {
-        let mut s = Secp256k1::new().unwrap();
+        let s = Secp256k1::new_deterministic();
+        let one = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1];
 
-        let mut msg = [0u8; 32];
+        let sk = SecretKey::from_slice(&s, &one).unwrap();
-        thread_rng().fill_bytes(&mut msg);
+        let msg = Message::from_slice(&one).unwrap();
-        let msg = Message::from_slice(&msg).unwrap();
 
-        let (sk, _) = s.generate_keypair(false);
+        let sig = s.sign(&msg, &sk);
-
+        assert_eq!(sig, Signature(70, [
-        s.sign(&msg, &sk).unwrap();
+            0x30, 0x44, 0x02, 0x20, 0x66, 0x73, 0xff, 0xad,
+            0x21, 0x47, 0x74, 0x1f, 0x04, 0x77, 0x2b, 0x6f,
+            0x92, 0x1f, 0x0b, 0xa6, 0xaf, 0x0c, 0x1e, 0x77,
+            0xfc, 0x43, 0x9e, 0x65, 0xc3, 0x6d, 0xed, 0xf4,
+            0x09, 0x2e, 0x88, 0x98, 0x02, 0x20, 0x4c, 0x1a,
+            0x97, 0x16, 0x52, 0xe0, 0xad, 0xa8, 0x80, 0x12,
+            0x0e, 0xf8, 0x02, 0x5e, 0x70, 0x9f, 0xff, 0x20,
+            0x80, 0xc4, 0xa3, 0x9a, 0xae, 0x06, 0x8d, 0x12,
+            0xee, 0xd0, 0x09, 0xb6, 0x8c, 0x89, 0x00, 0x00]))
     }
 
     #[test]
     fn sign_and_verify() {
         let mut s = Secp256k1::new().unwrap();
 
-        let mut msg: Vec<u8> = repeat(0).take(32).collect();
+        let mut msg = [0; 32];
+        for _ in 0..100 {
             thread_rng().fill_bytes(&mut msg);
             let msg = Message::from_slice(&msg).unwrap();
 
             let (sk, pk) = s.generate_keypair(false);
-
+            let sig = s.sign(&msg, &sk);
-        let sig = s.sign(&msg, &sk).unwrap();
-
             assert_eq!(s.verify(&msg, &sig, &pk), Ok(()));
          }
+    }
 
     #[test]
     fn sign_and_verify_fail() {
@@ -486,12 +496,16 @@ mod tests {
 
         let (sk, pk) = s.generate_keypair(false);
 
-        let sig = s.sign(&msg, &sk).unwrap();
+        let sig = s.sign(&msg, &sk);
+        let (sig_compact, recid) = s.sign_compact(&msg, &sk);
 
         let mut msg = [0u8; 32];
         thread_rng().fill_bytes(&mut msg);
         let msg = Message::from_slice(&msg).unwrap();
         assert_eq!(s.verify(&msg, &sig, &pk), Err(IncorrectSignature));
+
+        let recovered_key = s.recover_compact(&msg, &sig_compact[..], false, recid).unwrap();
+        assert!(recovered_key != pk);
     }
 
     #[test]
@@ -504,11 +518,55 @@ mod tests {
 
         let (sk, pk) = s.generate_keypair(false);
 
-        let (sig, recid) = s.sign_compact(&msg, &sk).unwrap();
+        let (sig, recid) = s.sign_compact(&msg, &sk);
 
         assert_eq!(s.recover_compact(&msg, &sig[..], false, recid), Ok(pk));
     }
 
+    #[test]
+    fn bad_recovery() {
+        let s = Secp256k1::new().unwrap();
+
+        let msg = Message::from_slice(&[0x55; 32]).unwrap();
+
+        // Bad length
+        assert_eq!(s.recover_compact(&msg, &[1; 63], false, RecoveryId(0)), Err(InvalidSignature));
+        assert_eq!(s.recover_compact(&msg, &[1; 65], false, RecoveryId(0)), Err(InvalidSignature));
+        // Zero is not a valid sig
+        assert_eq!(s.recover_compact(&msg, &[0; 64], false, RecoveryId(0)), Err(InvalidSignature));
+        // ...but 111..111 is
+        assert!(s.recover_compact(&msg, &[1; 64], false, RecoveryId(0)).is_ok());
+    }
+
+    #[test]
+    fn test_bad_slice() {
+        assert_eq!(Signature::from_slice(&[0; constants::MAX_SIGNATURE_SIZE + 1]),
+                   Err(InvalidSignature));
+        assert!(Signature::from_slice(&[0; constants::MAX_SIGNATURE_SIZE]).is_ok());
+
+        assert_eq!(Message::from_slice(&[0; constants::MESSAGE_SIZE - 1]),
+                   Err(InvalidMessage));
+        assert_eq!(Message::from_slice(&[0; constants::MESSAGE_SIZE + 1]),
+                   Err(InvalidMessage));
+        assert!(Signature::from_slice(&[0; constants::MESSAGE_SIZE]).is_ok());
+    }
+
+    #[test]
+    fn test_debug_output() {
+        let sig = Signature(0, [4; 72]);
+        assert_eq!(&format!("{:?}", sig), "Signature()");
+        let sig = Signature(10, [5; 72]);
+        assert_eq!(&format!("{:?}", sig), "Signature(05050505050505050505)");
+        let sig = Signature(72, [6; 72]);
+        assert_eq!(&format!("{:?}", sig), "Signature(060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606060606)");
+
+        let msg = Message([1, 2, 3, 4, 5, 6, 7, 8,
+                           9, 10, 11, 12, 13, 14, 15, 16,
+                           17, 18, 19, 20, 21, 22, 23, 24,
+                           25, 26, 27, 28, 29, 30, 31, 255]);
+        assert_eq!(&format!("{:?}", msg), "Message(0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1fff)");
+    }
+
     #[bench]
     pub fn generate_compressed(bh: &mut Bencher) {
         let mut s = Secp256k1::new().unwrap();