From cd483f25146a0dfe3a7efa64d0388d42c81705aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 7 Jul 2020 21:26:50 +0200 Subject: [PATCH] Isolate crypto stuff and avoid dealing with leaking `SecretKey` (#365) * Extract SecretKey usage and hide it behind a trait. * Isolate secp256k1 to one module. * cargo fmt --all --- Cargo.toml | 1 - src/api/accounts.rs | 174 +++++++++--------------------------------- src/contract/mod.rs | 4 +- src/error.rs | 13 ++-- src/lib.rs | 1 + src/signing.rs | 167 ++++++++++++++++++++++++++++++++++++++++ src/types/recovery.rs | 53 +++++++------ 7 files changed, 240 insertions(+), 173 deletions(-) create mode 100644 src/signing.rs diff --git a/Cargo.toml b/Cargo.toml index 31da23f4..a30231bc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,6 @@ secp256k1 = { version = "0.17", features = ["recovery"] } serde = { version = "1.0.90", features = ["derive"] } serde_json = "1.0.39" tiny-keccak = { version = "2.0.1", features = ["keccak"] } -zeroize = "1.1.0" # Optional deps ## HTTP base64 = { version = "0.12.0", optional = true } diff --git a/src/api/accounts.rs b/src/api/accounts.rs index d2c5f74d..a8c117ce 100644 --- a/src/api/accounts.rs +++ b/src/api/accounts.rs @@ -3,6 +3,7 @@ use crate::api::{Namespace, Web3}; use crate::error; use crate::helpers::CallFuture; +use crate::signing::{self, Signature}; use crate::types::{ Address, Bytes, Recovery, RecoveryMessage, SignedData, SignedTransaction, TransactionParameters, H256, U256, }; @@ -13,14 +14,9 @@ use futures::{ Future, FutureExt, }; use rlp::RlpStream; -use secp256k1::key::ONE_KEY; -use secp256k1::{Message, PublicKey, Secp256k1, SecretKey}; use std::convert::TryInto; use std::mem; -use std::ops::Deref; use std::pin::Pin; -use tiny_keccak::{Hasher, Keccak}; -use zeroize::{DefaultIsZeroes, Zeroize}; /// `Accounts` namespace #[derive(Debug, Clone)] @@ -48,7 +44,7 @@ impl Accounts { } /// Signs an Ethereum transaction with a given private key. - pub fn sign_transaction(&self, tx: TransactionParameters, key: &SecretKey) -> SignTransactionFuture { + pub fn sign_transaction(&self, tx: TransactionParameters, key: K) -> SignTransactionFuture { SignTransactionFuture::new(self, tx, key) } @@ -66,7 +62,7 @@ impl Accounts { let mut eth_message = format!("\x19Ethereum Signed Message:\n{}", message.len()).into_bytes(); eth_message.extend_from_slice(message); - keccak256(ð_message).into() + signing::keccak256(ð_message).into() } /// Sign arbitrary string data. @@ -76,15 +72,16 @@ impl Accounts { /// notation, that is the recovery value `v` is either `27` or `28` (as /// opposed to the standard notation where `v` is either `0` or `1`). This /// is important to consider when using this signature with other crates. - pub fn sign(&self, message: S, key: &SecretKey) -> SignedData + pub fn sign(&self, message: S, key: impl signing::Key) -> SignedData where S: AsRef<[u8]>, { let message = message.as_ref(); let message_hash = self.hash_message(message); - let sig_message = Message::from_slice(message_hash.as_bytes()).expect("hash is non-zero 32-bytes; qed"); - let signature = sign(&sig_message, key, None); + let signature = key + .sign(&message_hash.as_bytes(), None) + .expect("hash is non-zero 32-bytes; qed"); let v = signature .v .try_into() @@ -124,47 +121,14 @@ impl Accounts { RecoveryMessage::Data(ref message) => self.hash_message(message), RecoveryMessage::Hash(hash) => hash, }; - let signature = recovery.as_signature()?; - - let message = Message::from_slice(message_hash.as_bytes())?; - let public_key = Secp256k1::verification_only().recover(&message, &signature)?; - - Ok(public_key_address(&public_key)) + let (signature, recovery_id) = recovery + .as_signature() + .ok_or_else(|| error::Error::Recovery(signing::RecoveryError::InvalidSignature))?; + let address = signing::recover(message_hash.as_bytes(), &signature, recovery_id)?; + Ok(address) } } -/// Compute the Keccak-256 hash of input bytes. -pub fn keccak256(bytes: &[u8]) -> [u8; 32] { - let mut output = [0u8; 32]; - let mut hasher = Keccak::v256(); - hasher.update(bytes); - hasher.finalize(&mut output); - output -} - -/// Gets the public address of a private key. -fn secret_key_address(key: &SecretKey) -> Address { - let secp = Secp256k1::signing_only(); - let public_key = PublicKey::from_secret_key(&secp, key); - public_key_address(&public_key) -} - -/// Gets the address of a public key. -/// -/// The public address is defined as the low 20 bytes of the keccak hash of -/// the public key. Note that the public key returned from the `secp256k1` -/// crate is 65 bytes long, that is because it is prefixed by `0x04` to -/// indicate an uncompressed public key; this first byte is ignored when -/// computing the hash. -fn public_key_address(public_key: &PublicKey) -> Address { - let public_key = public_key.serialize_uncompressed(); - - debug_assert_eq!(public_key[0], 0x04); - let hash = keccak256(&public_key[1..]); - - Address::from_slice(&hash[12..]) -} - type MaybeReady = Either>, CallFuture::Out>>; type TxParams = Join3, MaybeReady, MaybeReady>; @@ -175,15 +139,15 @@ type TxParams = Join3, MaybeReady, MaybeReady { +pub struct SignTransactionFuture { tx: TransactionParameters, - key: Box, + key: Option, inner: TxParams, } -impl SignTransactionFuture { +impl SignTransactionFuture { /// Creates a new SignTransactionFuture with accounts and transaction data. - pub fn new(accounts: &Accounts, tx: TransactionParameters, key: &SecretKey) -> SignTransactionFuture { + pub fn new(accounts: &Accounts, tx: TransactionParameters, key: K) -> SignTransactionFuture { macro_rules! maybe { ($o: expr, $f: expr) => { match $o { @@ -193,7 +157,7 @@ impl SignTransactionFuture { }; } - let from = secret_key_address(key); + let from = key.address(); let inner = future::join3( maybe!(tx.nonce, accounts.web3().eth().transaction_count(from, None)), maybe!(tx.gas_price, accounts.web3().eth().gas_price()), @@ -202,13 +166,13 @@ impl SignTransactionFuture { SignTransactionFuture { tx, - key: ZeroizeSecretKey::boxed(*key), + key: Some(key), inner, } } } -impl Future for SignTransactionFuture { +impl Future for SignTransactionFuture { type Output = error::Result; fn poll(mut self: Pin<&mut Self>, ctx: &mut Context) -> Poll { @@ -224,49 +188,17 @@ impl Future for SignTransactionFuture { value: self.tx.value, data: data.0, }; - let signed = tx.sign(&self.key, chain_id); + let signed = tx.sign( + self.key + .take() + .expect("SignTransactionFuture can't be polled after Ready; qed"), + chain_id, + ); Poll::Ready(Ok(signed)) } } -impl Drop for SignTransactionFuture { - fn drop(&mut self) { - self.key.zeroize(); - } -} - -/// A struct that represents a the components of a secp256k1 signature. -struct Signature { - v: u64, - r: H256, - s: H256, -} - -/// Sign a message with a secret key and optional chain ID. -/// -/// When a chain ID is provided, the `Signature`'s V-value will have chain relay -/// protection added (as per EIP-155). Otherwise, the V-value will be in -/// 'Electrum' notation. -fn sign(message: &Message, key: &SecretKey, chain_id: Option) -> Signature { - let (recovery_id, signature) = Secp256k1::signing_only() - .sign_recoverable(message, key) - .serialize_compact(); - - let standard_v = recovery_id.to_i32() as u64; - let v = if let Some(chain_id) = chain_id { - // When signing with a chain ID, add chain replay protection. - standard_v + 35 + chain_id * 2 - } else { - // Otherwise, convert to 'Electrum' notation. - standard_v + 27 - }; - let r = H256::from_slice(&signature[..32]); - let s = H256::from_slice(&signature[32..]); - - Signature { v, r, s } -} - /// A transaction used for RLP encoding, hashing and signing. struct Transaction { to: Option
, @@ -315,18 +247,19 @@ impl Transaction { } /// Sign and return a raw signed transaction. - fn sign(self, key: &SecretKey, chain_id: u64) -> SignedTransaction { + fn sign(self, sign: impl signing::Key, chain_id: u64) -> SignedTransaction { let mut rlp = RlpStream::new(); self.rlp_append_unsigned(&mut rlp, chain_id); - let hash = keccak256(rlp.as_raw()); - let message = Message::from_slice(&hash).expect("hash is non-zero 32-bytes; qed"); - let signature = sign(&message, key, Some(chain_id)); + let hash = signing::keccak256(rlp.as_raw()); + let signature = sign + .sign(&hash, Some(chain_id)) + .expect("hash is non-zero 32-bytes; qed"); rlp.clear(); self.rlp_append_signed(&mut rlp, &signature); - let transaction_hash = keccak256(rlp.as_raw()).into(); + let transaction_hash = signing::keccak256(rlp.as_raw()).into(); let raw_transaction = rlp.out().into(); SignedTransaction { @@ -340,43 +273,11 @@ impl Transaction { } } -/// A wrapper type around `SecretKey` to prevent leaking secret key data. This -/// type will properly zeroize the secret key to `ONE_KEY` in a way that will -/// not get optimized away by the compiler nor be prone to leaks that take -/// advantage of access reordering. -/// -/// This is required since the `SignTransactionFuture` needs to retain a copy -/// of the `SecretKey`. -#[derive(Clone, Copy)] -struct ZeroizeSecretKey(SecretKey); - -impl ZeroizeSecretKey { - /// Create new boxed instance to make sure we don't leak any copies around. - pub fn boxed(key: SecretKey) -> Box { - Box::new(Self(key)) - } -} - -impl Default for ZeroizeSecretKey { - fn default() -> Self { - ZeroizeSecretKey(ONE_KEY) - } -} - -impl Deref for ZeroizeSecretKey { - type Target = SecretKey; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl DefaultIsZeroes for ZeroizeSecretKey {} - #[cfg(test)] mod tests { use super::*; use crate::helpers::tests::TestTransport; + use crate::signing::{SecretKey, SecretKeyRef}; use crate::types::Bytes; use rustc_hex::FromHex; use serde_json::json; @@ -398,7 +299,7 @@ mod tests { let nonce = U256::zero(); let gas_price = U256::from(21_000_000_000u128); let chain_id = "0x1"; - let from: Address = secret_key_address(&key); + let from: Address = signing::secret_key_address(&key); let mut transport = TestTransport::default(); transport.add_response(json!(nonce)); @@ -493,7 +394,7 @@ mod tests { let key: SecretKey = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" .parse() .unwrap(); - let signed = accounts.sign("Some data", &key); + let signed = accounts.sign("Some data", SecretKeyRef::new(&key)); assert_eq!( signed.message_hash, @@ -542,7 +443,7 @@ mod tests { let key: SecretKey = "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" .parse() .unwrap(); - let address: Address = secret_key_address(&key); + let address: Address = signing::secret_key_address(&key); let accounts = Accounts::new(TestTransport::default()); @@ -580,11 +481,12 @@ mod tests { value: 1_000_000_000.into(), data: Vec::new(), }; - let key: SecretKey = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" + let skey: SecretKey = "4c0883a69102937d6231471b5dbb6204fe5129617082792ae468d01a3f362318" .parse() .unwrap(); + let key = SecretKeyRef::new(&skey); - let signed = tx.sign(&key, 1); + let signed = tx.sign(key, 1); let expected = SignedTransaction { message_hash: "6893a6ee8df79b0f5d64a180cd1ef35d030f3e296a5361cf04d02ce720d32ec5" diff --git a/src/contract/mod.rs b/src/contract/mod.rs index 209ea109..7c5a3855 100644 --- a/src/contract/mod.rs +++ b/src/contract/mod.rs @@ -3,6 +3,7 @@ use crate::api::{Accounts, Eth, Namespace}; use crate::confirm; use crate::contract::tokens::{Detokenize, Tokenize}; +use crate::signing; use crate::types::{ Address, BlockId, Bytes, CallRequest, FilterBuilder, TransactionCondition, TransactionParameters, TransactionReceipt, TransactionRequest, H256, U256, @@ -12,7 +13,6 @@ use futures::{ future::{self, Either}, Future, FutureExt, TryFutureExt, }; -use secp256k1::key::SecretKey; use std::{collections::HashMap, hash::Hash, time}; pub mod deploy; @@ -159,7 +159,7 @@ impl Contract { params: impl Tokenize, options: Options, confirmations: usize, - key: &'a SecretKey, + key: impl signing::Key + 'a, ) -> impl Future> + 'a { let poll_interval = time::Duration::from_secs(1); diff --git a/src/error.rs b/src/error.rs index 155e0e2b..83138955 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,7 +1,6 @@ //! Web3 Error use crate::rpc::error::Error as RPCError; use derive_more::{Display, From}; -use secp256k1::Error as Secp256k1Error; use serde_json::Error as SerdeError; use std::io::Error as IoError; @@ -31,9 +30,9 @@ pub enum Error { /// io error #[display(fmt = "IO error: {}", _0)] Io(IoError), - /// signing error - #[display(fmt = "Signing error: {}", _0)] - Signing(Secp256k1Error), + /// recovery error + #[display(fmt = "Recovery error: {}", _0)] + Recovery(crate::signing::RecoveryError), /// web3 internal error #[display(fmt = "Internal Web3 error")] Internal, @@ -46,7 +45,7 @@ impl std::error::Error for Error { Unreachable | Decoder(_) | InvalidResponse(_) | Transport(_) | Internal => None, Rpc(ref e) => Some(e), Io(ref e) => Some(e), - Signing(ref e) => Some(e), + Recovery(ref e) => Some(e), } } } @@ -67,7 +66,7 @@ impl Clone for Error { Transport(s) => Transport(s.clone()), Rpc(e) => Rpc(e.clone()), Io(e) => Io(IoError::from(e.kind())), - Signing(e) => Signing(*e), + Recovery(e) => Recovery(e.clone()), Internal => Internal, } } @@ -84,7 +83,7 @@ impl PartialEq for Error { } (Rpc(a), Rpc(b)) => a == b, (Io(a), Io(b)) => a.kind() == b.kind(), - (Signing(a), Signing(b)) => a == b, + (Recovery(a), Recovery(b)) => a == b, _ => false, } } diff --git a/src/lib.rs b/src/lib.rs index b7de709a..5640b8e8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,6 +27,7 @@ pub mod api; pub mod confirm; pub mod contract; pub mod error; +pub mod signing; pub mod transports; pub mod types; diff --git a/src/signing.rs b/src/signing.rs new file mode 100644 index 00000000..b83d6116 --- /dev/null +++ b/src/signing.rs @@ -0,0 +1,167 @@ +//! Signing capabilities and utilities. + +use crate::types::{Address, H256}; +use secp256k1::recovery::{RecoverableSignature, RecoveryId}; +use secp256k1::{Message, PublicKey, Secp256k1}; +use std::ops::Deref; + +pub(crate) use secp256k1::SecretKey; + +/// Error during signing. +#[derive(Debug, derive_more::Display, PartialEq, Clone)] +pub enum SigningError { + /// A message to sign is invalid. Has to be a non-zero 32-bytes slice. + #[display(fmt = "Message has to be a non-zero 32-bytes slice.")] + InvalidMessage, +} +impl std::error::Error for SigningError {} + +/// Error during sender recovery. +#[derive(Debug, derive_more::Display, PartialEq, Clone)] +pub enum RecoveryError { + /// A message to recover is invalid. Has to be a non-zero 32-bytes slice. + #[display(fmt = "Message has to be a non-zero 32-bytes slice.")] + InvalidMessage, + /// A signature is invalid and the sender could not be recovered. + #[display(fmt = "Signature is invalid (check recovery id).")] + InvalidSignature, +} +impl std::error::Error for RecoveryError {} + +/// A trait representing ethereum-compatible key with signing capabilities. +/// +/// The purpose of this trait is to prevent leaking `secp256k1::SecretKey` struct +/// in stack or memory. +/// To use secret keys securely, they should be wrapped in a struct that prevents +/// leaving copies in memory (both when it's moved or dropped). Please take a look +/// at: +/// - https://github.com/graphprotocol/solidity-bindgen/blob/master/solidity-bindgen/src/secrets.rs +/// - or https://crates.io/crates/zeroize +/// if you care enough about your secrets to be used securely. +/// +/// If it's enough to pass a reference to `SecretKey` (lifetimes) than you can use `SecretKeyRef` +/// wrapper. +pub trait Key: std::marker::Unpin { + /// Sign given message and include chain-id replay protection. + /// + /// When a chain ID is provided, the `Signature`'s V-value will have chain relay + /// protection added (as per EIP-155). Otherwise, the V-value will be in + /// 'Electrum' notation. + fn sign(&self, message: &[u8], chain_id: Option) -> Result; + + /// Get public address that this key represents. + fn address(&self) -> Address; +} + +/// A `SecretKey` reference wrapper. +/// +/// A wrapper around `secp256k1::SecretKey` reference, which enables it to be used in methods expecting +/// `Key` capabilities. +pub struct SecretKeyRef<'a> { + key: &'a SecretKey, +} + +impl<'a> SecretKeyRef<'a> { + /// A simple wrapper around a reference to `SecretKey` which allows it to be usable for signing. + pub fn new(key: &'a SecretKey) -> Self { + Self { key } + } +} + +impl<'a> From<&'a SecretKey> for SecretKeyRef<'a> { + fn from(key: &'a SecretKey) -> Self { + Self::new(key) + } +} + +impl<'a> Deref for SecretKeyRef<'a> { + type Target = SecretKey; + + fn deref(&self) -> &Self::Target { + &self.key + } +} + +impl + std::marker::Unpin> Key for T { + fn sign(&self, message: &[u8], chain_id: Option) -> Result { + let message = Message::from_slice(&message).map_err(|_| SigningError::InvalidMessage)?; + let (recovery_id, signature) = Secp256k1::signing_only() + .sign_recoverable(&message, self) + .serialize_compact(); + + let standard_v = recovery_id.to_i32() as u64; + let v = if let Some(chain_id) = chain_id { + // When signing with a chain ID, add chain replay protection. + standard_v + 35 + chain_id * 2 + } else { + // Otherwise, convert to 'Electrum' notation. + standard_v + 27 + }; + let r = H256::from_slice(&signature[..32]); + let s = H256::from_slice(&signature[32..]); + + Ok(Signature { v, r, s }) + } + + fn address(&self) -> Address { + secret_key_address(self) + } +} + +/// A struct that represents the components of a secp256k1 signature. +pub struct Signature { + /// V component in electrum format with chain-id replay protection. + pub v: u64, + /// R component of the signature. + pub r: H256, + /// S component of the signature. + pub s: H256, +} + +/// Recover a sender, given message and the signature. +/// +/// Signature and `recovery_id` can be obtained from `types::Recovery` type. +pub fn recover(message: &[u8], signature: &[u8], recovery_id: i32) -> Result { + let message = Message::from_slice(message).map_err(|_| RecoveryError::InvalidMessage)?; + let recovery_id = RecoveryId::from_i32(recovery_id).map_err(|_| RecoveryError::InvalidSignature)?; + let signature = + RecoverableSignature::from_compact(&signature, recovery_id).map_err(|_| RecoveryError::InvalidSignature)?; + let public_key = Secp256k1::verification_only() + .recover(&message, &signature) + .map_err(|_| RecoveryError::InvalidSignature)?; + + Ok(public_key_address(&public_key)) +} + +/// Gets the address of a public key. +/// +/// The public address is defined as the low 20 bytes of the keccak hash of +/// the public key. Note that the public key returned from the `secp256k1` +/// crate is 65 bytes long, that is because it is prefixed by `0x04` to +/// indicate an uncompressed public key; this first byte is ignored when +/// computing the hash. +pub(crate) fn public_key_address(public_key: &PublicKey) -> Address { + let public_key = public_key.serialize_uncompressed(); + + debug_assert_eq!(public_key[0], 0x04); + let hash = keccak256(&public_key[1..]); + + Address::from_slice(&hash[12..]) +} + +/// Gets the public address of a private key. +pub(crate) fn secret_key_address(key: &SecretKey) -> Address { + let secp = Secp256k1::signing_only(); + let public_key = PublicKey::from_secret_key(&secp, key); + public_key_address(&public_key) +} + +/// Compute the Keccak-256 hash of input bytes. +pub fn keccak256(bytes: &[u8]) -> [u8; 32] { + use tiny_keccak::{Hasher, Keccak}; + let mut output = [0u8; 32]; + let mut hasher = Keccak::v256(); + hasher.update(bytes); + hasher.finalize(&mut output); + output +} diff --git a/src/types/recovery.rs b/src/types/recovery.rs index da860a1c..1ebbf386 100644 --- a/src/types/recovery.rs +++ b/src/types/recovery.rs @@ -1,6 +1,4 @@ use crate::types::{SignedData, SignedTransaction, H256}; -use secp256k1::recovery::{RecoverableSignature, RecoveryId}; -use secp256k1::Error as Secp256k1Error; use std::error::Error; use std::fmt::{Display, Formatter, Result as FmtResult}; @@ -58,20 +56,21 @@ impl Recovery { Ok(Recovery::new(message, v as _, r, s)) } - /// Retrieve the recovery ID. - pub fn recovery_id(&self) -> Result { - let standard_v = match self.v { - 27 => 0, - 28 => 1, - v if v >= 35 => ((v - 1) % 2) as _, - _ => 4, - }; - - RecoveryId::from_i32(standard_v) + /// Retrieve the Recovery Id ("Standard V") + /// + /// Returns `None` if `v` value is invalid + /// (equivalent of returning `4` in some implementaions). + pub fn recovery_id(&self) -> Option { + match self.v { + 27 => Some(0), + 28 => Some(1), + v if v >= 35 => Some(((v - 1) % 2) as _), + _ => None, + } } - /// Retrieves the recovery signature. - pub fn as_signature(&self) -> Result { + /// Retrieves the recovery id & compact signature in it's raw form. + pub fn as_signature(&self) -> Option<([u8; 64], i32)> { let recovery_id = self.recovery_id()?; let signature = { let mut sig = [0u8; 64]; @@ -80,7 +79,7 @@ impl Recovery { sig }; - RecoverableSignature::from_compact(&signature, recovery_id) + Some((signature, recovery_id)) } } @@ -188,20 +187,20 @@ mod tests { .unwrap() ), }; - let expected_signature = RecoverableSignature::from_compact( - &"b91467e570a6466aa9e9876cbcd013baba02900b8979d43fe208a4a4f339f5fd6007e74cd82e037b800186422fc2da167c747ef045e5d18a5f5d4300f8e1a029" + let expected_signature = ( + "b91467e570a6466aa9e9876cbcd013baba02900b8979d43fe208a4a4f339f5fd6007e74cd82e037b800186422fc2da167c747ef045e5d18a5f5d4300f8e1a029" .from_hex::>() .unwrap(), - RecoveryId::from_i32(1).unwrap(), - ); - - assert_eq!(Recovery::from(&signed).as_signature(), expected_signature); - assert_eq!(Recovery::new(message, v as _, r, s).as_signature(), expected_signature); - assert_eq!( - Recovery::from_raw_signature(message, &signed.signature.0) - .unwrap() - .as_signature(), - expected_signature + 1 ); + let (sig, id) = Recovery::from(&signed).as_signature().unwrap(); + assert_eq!((sig.to_vec(), id), expected_signature); + let (sig, id) = Recovery::new(message, v as _, r, s).as_signature().unwrap(); + assert_eq!((sig.to_vec(), id), expected_signature); + let (sig, id) = Recovery::from_raw_signature(message, &signed.signature.0) + .unwrap() + .as_signature() + .unwrap(); + assert_eq!((sig.to_vec(), id), expected_signature); } }