Improve bls::SecretKey privacy (#1164)

* Improve bls::SecretKey privacy

* Add missed file

* Remove more methods from bls::SecretKey

* Add as_bytes() to SecretKey, remove as_raw

* Remove as_raw

* Add back as_raw

* Address review comments

crypto/bls/Cargo.toml

@@ -16,6 +16,7 @@ eth2_ssz = "0.1.2"
 eth2_ssz_types = { path = "../../consensus/ssz_types" }
 tree_hash = "0.1.0"
 arbitrary = { version = "0.4.4", features = ["derive"], optional = true }
+zeroize = { version = "1.0.0", features = ["zeroize_derive"] }
 
 [features]
 fake_crypto = []

crypto/bls/src/keypair.rs

@@ -1,9 +1,8 @@
 use super::{PublicKey, SecretKey};
-use serde_derive::{Deserialize, Serialize};
 use std::fmt;
 use std::hash::{Hash, Hasher};
 
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Clone)]
 pub struct Keypair {
     pub sk: SecretKey,
     pub pk: PublicKey,

crypto/bls/src/lib.rs

@@ -4,6 +4,7 @@ extern crate ssz;
 #[macro_use]
 mod macros;
 mod keypair;
+mod plain_text;
 mod public_key_bytes;
 mod secret_key;
 mod signature_bytes;
@@ -14,6 +15,7 @@ pub use crate::public_key_bytes::PublicKeyBytes;
 pub use crate::secret_key::SecretKey;
 pub use crate::signature_bytes::SignatureBytes;
 pub use milagro_bls::{compress_g2, hash_to_curve_g2};
+pub use plain_text::PlainText;
 pub use signature_set::{verify_signature_sets, SignatureSet};
 
 #[cfg(feature = "arbitrary")]

crypto/bls/src/plain_text.rs

@@ -32,3 +32,9 @@ impl From<Vec<u8>> for PlainText {
         Self(vec)
     }
 }
+
+impl AsRef<[u8]> for PlainText {
+    fn as_ref(&self) -> &[u8] {
+        &self.0
+    }
+}

crypto/bls/src/secret_key.rs

@@ -1,21 +1,18 @@
 extern crate rand;
 
-use super::BLS_SECRET_KEY_BYTE_SIZE;
-use hex::encode as hex_encode;
+use crate::PlainText;
 use milagro_bls::SecretKey as RawSecretKey;
-use serde::de::{Deserialize, Deserializer};
-use serde::ser::{Serialize, Serializer};
-use serde_hex::PrefixedHexVisitor;
-use ssz::{ssz_encode, Decode, DecodeError, Encode};
+use ssz::DecodeError;
 
 /// A single BLS signature.
 ///
 /// This struct is a wrapper upon a base type and provides helper functions (e.g., SSZ
 /// serialization).
-#[derive(Debug, PartialEq, Clone, Eq)]
+#[derive(Clone)]
 pub struct SecretKey(RawSecretKey);
 
 impl SecretKey {
+    /// Generate a new `Self` using `rand::thread_rng`.
     pub fn random() -> Self {
         SecretKey(RawSecretKey::random(&mut rand::thread_rng()))
     }
@@ -24,9 +21,13 @@ impl SecretKey {
         Self(raw)
     }
 
-    /// Returns the underlying point as compressed bytes.
-    fn as_bytes(&self) -> Vec<u8> {
-        self.as_raw().as_bytes()
+    /// Returns the secret key as a byte array (wrapped in `PlainText` wrapper so it is zeroized on
+    /// `Drop`).
+    ///
+    /// Extreme care should be taken not to leak these bytes as they are the unencrypted secret
+    /// key.
+    pub fn as_bytes(&self) -> PlainText {
+        self.as_raw().as_bytes().into()
     }
 
     /// Instantiate a SecretKey from existing bytes.
@@ -42,40 +43,14 @@ impl SecretKey {
     }
 
     /// Returns the underlying secret key.
-    pub fn as_raw(&self) -> &RawSecretKey {
+    pub(crate) fn as_raw(&self) -> &RawSecretKey {
         &self.0
     }
 }
 
-impl_ssz!(SecretKey, BLS_SECRET_KEY_BYTE_SIZE, "SecretKey");
-
-impl_tree_hash!(SecretKey, BLS_SECRET_KEY_BYTE_SIZE);
-
-impl Serialize for SecretKey {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        serializer.serialize_str(&hex_encode(ssz_encode(self)))
-    }
-}
-
-impl<'de> Deserialize<'de> for SecretKey {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: Deserializer<'de>,
-    {
-        let bytes = deserializer.deserialize_str(PrefixedHexVisitor)?;
-        let secret_key = SecretKey::from_ssz_bytes(&bytes[..])
-            .map_err(|e| serde::de::Error::custom(format!("invalid ssz ({:?})", e)))?;
-        Ok(secret_key)
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
-    use ssz::ssz_encode;
 
     #[test]
     pub fn test_ssz_round_trip() {
@@ -85,9 +60,9 @@ mod tests {
         ];
         let original = SecretKey::from_bytes(&byte_key).unwrap();
 
-        let bytes = ssz_encode(&original);
-        let decoded = SecretKey::from_ssz_bytes(&bytes).unwrap();
+        let bytes = original.as_bytes();
+        let decoded = SecretKey::from_bytes(bytes.as_ref()).unwrap();
 
-        assert_eq!(original, decoded);
+        assert!(original.as_bytes() == decoded.as_bytes());
     }
 }

crypto/eth2_keystore/src/keystore.rs

@@ -6,7 +6,7 @@ use crate::json_keystore::{
     Aes128Ctr, ChecksumModule, Cipher, CipherModule, Crypto, EmptyMap, EmptyString, JsonKeystore,
     Kdf, KdfModule, Scrypt, Sha256Checksum, Version,
 };
-use crate::plain_text::PlainText;
+use crate::PlainText;
 use crate::Uuid;
 use bls::{Keypair, PublicKey, SecretKey};
 use crypto::{digest::Digest, sha2::Sha256};
@@ -130,7 +130,7 @@ impl Keystore {
         uuid: Uuid,
         path: String,
     ) -> Result<Self, Error> {
-        let secret = PlainText::from(keypair.sk.as_raw().as_bytes());
+        let secret: PlainText = keypair.sk.as_bytes();
 
         let (cipher_text, checksum) = encrypt(secret.as_bytes(), password, &kdf, &cipher)?;
 

crypto/eth2_keystore/src/lib.rs

@@ -3,13 +3,12 @@
 
 mod derived_key;
 mod keystore;
-mod plain_text;
 
 pub mod json_keystore;
 
+pub use bls::PlainText;
 pub use keystore::{
     decrypt, default_kdf, encrypt, keypair_from_secret, Error, Keystore, KeystoreBuilder, DKLEN,
     HASH_SIZE, IV_SIZE, SALT_SIZE,
 };
-pub use plain_text::PlainText;
 pub use uuid::Uuid;

crypto/eth2_keystore/tests/eip2335_vectors.rs

@@ -14,7 +14,7 @@ pub fn decode_and_check_sk(json: &str) -> Keystore {
     let keystore = Keystore::from_json_str(json).expect("should decode keystore json");
     let expected_sk = hex::decode(EXPECTED_SECRET).unwrap();
     let keypair = keystore.decrypt_keypair(PASSWORD.as_bytes()).unwrap();
-    assert_eq!(keypair.sk.as_raw().as_bytes(), expected_sk);
+    assert_eq!(keypair.sk.as_bytes().as_ref(), &expected_sk[..]);
     keystore
 }
 

crypto/eth2_keystore/tests/tests.rs

@@ -38,8 +38,8 @@ fn string_round_trip() {
     );
 
     assert_eq!(
-        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(),
-        keypair,
+        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk,
+        keypair.pk,
         "should decrypt with good password"
     );
 }
@@ -77,8 +77,8 @@ fn file() {
     );
 
     assert_eq!(
-        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(),
-        keypair,
+        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk,
+        keypair.pk,
         "should decrypt with good password"
     );
 }
@@ -102,8 +102,8 @@ fn scrypt_params() {
     );
 
     assert_eq!(
-        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(),
-        keypair,
+        decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk,
+        keypair.pk,
         "should decrypt with good password"
     );
 }

crypto/eth2_wallet/Cargo.toml

@@ -18,5 +18,4 @@ tiny-bip39 = "0.7.3"
 
 [dev-dependencies]
 hex = "0.3"
-eth2_ssz = { path = "../../consensus/ssz" }
 tempfile = "3.1.0"

crypto/eth2_wallet/tests/tests.rs

@@ -4,7 +4,6 @@ use eth2_wallet::{
     bip39::{Language, Mnemonic, Seed},
     recover_validator_secret, DerivedKey, Error, KeyType, KeystoreError, Wallet, WalletBuilder,
 };
-use ssz::Encode;
 use std::fs::OpenOptions;
 use tempfile::tempdir;
 
@@ -243,14 +242,14 @@ fn key_derivation_from_seed() {
             .expect("should decrypt voting keypair");
 
         assert_eq!(
-            voting_keypair.sk.as_ssz_bytes(),
-            manually_derived_voting_key(i),
+            voting_keypair.sk.as_bytes().as_ref(),
+            &manually_derived_voting_key(i)[..],
             "voting secret should match manually derived"
         );
 
         assert_eq!(
-            voting_keypair.sk.as_ssz_bytes(),
-            recovered_voting_key(&wallet, i),
+            voting_keypair.sk.as_bytes().as_ref(),
+            &recovered_voting_key(&wallet, i)[..],
             "voting secret should match recovered"
         );
 
@@ -260,20 +259,20 @@ fn key_derivation_from_seed() {
             .expect("should decrypt withdrawal keypair");
 
         assert_eq!(
-            withdrawal_keypair.sk.as_ssz_bytes(),
-            manually_derived_withdrawal_key(i),
+            withdrawal_keypair.sk.as_bytes().as_ref(),
+            &manually_derived_withdrawal_key(i)[..],
             "withdrawal secret should match manually derived"
         );
 
         assert_eq!(
-            withdrawal_keypair.sk.as_ssz_bytes(),
-            recovered_withdrawal_key(&wallet, i),
+            withdrawal_keypair.sk.as_bytes().as_ref(),
+            &recovered_withdrawal_key(&wallet, i)[..],
             "withdrawal secret should match recovered"
         );
 
         assert_ne!(
-            withdrawal_keypair.sk.as_ssz_bytes(),
-            voting_keypair.sk.as_ssz_bytes(),
+            withdrawal_keypair.sk.as_bytes().as_ref(),
+            voting_keypair.sk.as_bytes().as_bytes(),
             "voting and withdrawal keypairs should be distinct"
         );