diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index 10ca6e370..3c8921555 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -2,6 +2,7 @@ use crate::common::{initiate_validator_exit, slash_validator}; use errors::{BlockInvalid as Invalid, BlockProcessingError as Error, IntoWithIndex}; use rayon::prelude::*; use std::collections::HashSet; +use std::convert::TryInto; use std::iter::FromIterator; use tree_hash::{SignedRoot, TreeHash}; use types::*; @@ -396,9 +397,13 @@ pub fn process_deposit( // depositing validator already exists in the registry. state.update_pubkey_cache()?; + let pubkey: PublicKey = match (&deposit.data.pubkey).try_into() { + Err(_) => return Ok(()), //bad public key => return early + Ok(k) => k, + }; // Get an `Option` where `u64` is the validator index if this deposit public key // already exists in the beacon_state. - let validator_index = get_existing_validator_index(state, deposit) + let validator_index = get_existing_validator_index(state, &pubkey) .map_err(|e| e.into_with_index(deposit_index))?; let amount = deposit.data.amount; @@ -409,13 +414,13 @@ pub fn process_deposit( } else { // The signature should be checked for new validators. Return early for a bad // signature. - if verify_deposit_signature(state, deposit, spec).is_err() { + if verify_deposit_signature(state, deposit, spec, &pubkey).is_err() { return Ok(()); } // Create a new validator. let validator = Validator { - pubkey: deposit.data.pubkey.clone(), + pubkey, withdrawal_credentials: deposit.data.withdrawal_credentials, activation_eligibility_epoch: spec.far_future_epoch, activation_epoch: spec.far_future_epoch, diff --git a/eth2/state_processing/src/per_block_processing/errors.rs b/eth2/state_processing/src/per_block_processing/errors.rs index e2b908c73..65179167c 100644 --- a/eth2/state_processing/src/per_block_processing/errors.rs +++ b/eth2/state_processing/src/per_block_processing/errors.rs @@ -349,6 +349,8 @@ pub enum DepositInvalid { BadIndex { state: u64, deposit: u64 }, /// The signature (proof-of-possession) does not match the given pubkey. BadSignature, + /// The signature does not represent a valid BLS signature. + BadSignatureBytes, /// The specified `branch` and `index` did not form a valid proof that the deposit is included /// in the eth1 deposit root. BadMerkleProof, diff --git a/eth2/state_processing/src/per_block_processing/verify_deposit.rs b/eth2/state_processing/src/per_block_processing/verify_deposit.rs index 5642c7a5f..0ce25a0b2 100644 --- a/eth2/state_processing/src/per_block_processing/verify_deposit.rs +++ b/eth2/state_processing/src/per_block_processing/verify_deposit.rs @@ -1,5 +1,6 @@ use super::errors::{DepositInvalid as Invalid, DepositValidationError as Error}; use merkle_proof::verify_merkle_proof; +use std::convert::TryInto; use tree_hash::{SignedRoot, TreeHash}; use types::*; @@ -10,15 +11,17 @@ pub fn verify_deposit_signature( state: &BeaconState, deposit: &Deposit, spec: &ChainSpec, + pubkey: &PublicKey, ) -> Result<(), Error> { // Note: Deposits are valid across forks, thus the deposit domain is computed // with the fork zeroed. let domain = spec.get_domain(state.current_epoch(), Domain::Deposit, &Fork::default()); + let signature: Signature = (&deposit.data.signature) + .try_into() + .map_err(|_| Error::Invalid(Invalid::BadSignatureBytes))?; + verify!( - deposit - .data - .signature - .verify(&deposit.data.signed_root(), domain, &deposit.data.pubkey,), + signature.verify(&deposit.data.signed_root(), domain, pubkey), Invalid::BadSignature ); @@ -33,9 +36,9 @@ pub fn verify_deposit_signature( /// Errors if the state's `pubkey_cache` is not current. pub fn get_existing_validator_index( state: &BeaconState, - deposit: &Deposit, + pub_key: &PublicKey, ) -> Result, Error> { - let validator_index = state.get_validator_index(&deposit.data.pubkey)?; + let validator_index = state.get_validator_index(pub_key)?; Ok(validator_index.map(|idx| idx as u64)) } diff --git a/eth2/types/src/deposit_data.rs b/eth2/types/src/deposit_data.rs index 8e5088889..4dc7689cd 100644 --- a/eth2/types/src/deposit_data.rs +++ b/eth2/types/src/deposit_data.rs @@ -1,6 +1,7 @@ use crate::test_utils::TestRandom; use crate::*; -use bls::{PublicKey, Signature}; +use bls::{PublicKeyBytes, SignatureBytes}; +use std::convert::From; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -25,11 +26,11 @@ use tree_hash_derive::{CachedTreeHash, SignedRoot, TreeHash}; TestRandom, )] pub struct DepositData { - pub pubkey: PublicKey, + pub pubkey: PublicKeyBytes, pub withdrawal_credentials: Hash256, pub amount: u64, #[signed_root(skip_hashing)] - pub signature: Signature, + pub signature: SignatureBytes, } impl DepositData { @@ -42,11 +43,11 @@ impl DepositData { epoch: Epoch, fork: &Fork, spec: &ChainSpec, - ) -> Signature { + ) -> SignatureBytes { let msg = self.signed_root(); let domain = spec.get_domain(epoch, Domain::Deposit, fork); - Signature::new(msg.as_slice(), domain, secret_key) + SignatureBytes::from(Signature::new(msg.as_slice(), domain, secret_key)) } } diff --git a/eth2/types/src/test_utils/builders/testing_deposit_builder.rs b/eth2/types/src/test_utils/builders/testing_deposit_builder.rs index df3dcffa1..ed08571a7 100644 --- a/eth2/types/src/test_utils/builders/testing_deposit_builder.rs +++ b/eth2/types/src/test_utils/builders/testing_deposit_builder.rs @@ -1,5 +1,5 @@ use crate::*; -use bls::get_withdrawal_credentials; +use bls::{get_withdrawal_credentials, PublicKeyBytes, SignatureBytes}; /// Builds an deposit to be used for testing purposes. /// @@ -14,10 +14,10 @@ impl TestingDepositBuilder { let deposit = Deposit { proof: vec![].into(), data: DepositData { - pubkey, + pubkey: PublicKeyBytes::from(pubkey), withdrawal_credentials: Hash256::zero(), amount, - signature: Signature::empty_signature(), + signature: SignatureBytes::empty(), }, }; @@ -34,7 +34,7 @@ impl TestingDepositBuilder { &get_withdrawal_credentials(&keypair.pk, spec.bls_withdrawal_prefix_byte)[..], ); - self.deposit.data.pubkey = keypair.pk.clone(); + self.deposit.data.pubkey = PublicKeyBytes::from(keypair.pk.clone()); self.deposit.data.withdrawal_credentials = withdrawal_credentials; self.deposit.data.signature = diff --git a/eth2/types/src/test_utils/test_random.rs b/eth2/types/src/test_utils/test_random.rs index 3598fa79c..fa1a41815 100644 --- a/eth2/types/src/test_utils/test_random.rs +++ b/eth2/types/src/test_utils/test_random.rs @@ -7,8 +7,10 @@ mod aggregate_signature; mod bitfield; mod hash256; mod public_key; +mod public_key_bytes; mod secret_key; mod signature; +mod signature_bytes; pub trait TestRandom { fn random_for_test(rng: &mut impl RngCore) -> Self; @@ -99,3 +101,5 @@ macro_rules! impl_test_random_for_u8_array { impl_test_random_for_u8_array!(4); impl_test_random_for_u8_array!(32); +impl_test_random_for_u8_array!(48); +impl_test_random_for_u8_array!(96); diff --git a/eth2/types/src/test_utils/test_random/public_key_bytes.rs b/eth2/types/src/test_utils/test_random/public_key_bytes.rs new file mode 100644 index 000000000..e801e30d2 --- /dev/null +++ b/eth2/types/src/test_utils/test_random/public_key_bytes.rs @@ -0,0 +1,19 @@ +use std::convert::From; + +use bls::{PublicKeyBytes, BLS_PUBLIC_KEY_BYTE_SIZE}; + +use super::*; + +impl TestRandom for PublicKeyBytes { + fn random_for_test(rng: &mut impl RngCore) -> Self { + //50-50 chance for signature to be "valid" or invalid + if bool::random_for_test(rng) { + //valid signature + PublicKeyBytes::from(PublicKey::random_for_test(rng)) + } else { + //invalid signature, just random bytes + PublicKeyBytes::from_bytes(&<[u8; BLS_PUBLIC_KEY_BYTE_SIZE]>::random_for_test(rng)) + .unwrap() + } + } +} diff --git a/eth2/types/src/test_utils/test_random/signature_bytes.rs b/eth2/types/src/test_utils/test_random/signature_bytes.rs new file mode 100644 index 000000000..cae2a8225 --- /dev/null +++ b/eth2/types/src/test_utils/test_random/signature_bytes.rs @@ -0,0 +1,17 @@ +use bls::{SignatureBytes, BLS_SIG_BYTE_SIZE}; + +use super::*; +use std::convert::From; + +impl TestRandom for SignatureBytes { + fn random_for_test(rng: &mut impl RngCore) -> Self { + //50-50 chance for signature to be "valid" or invalid + if bool::random_for_test(rng) { + //valid signature + SignatureBytes::from(Signature::random_for_test(rng)) + } else { + //invalid signature, just random bytes + SignatureBytes::from_bytes(&<[u8; BLS_SIG_BYTE_SIZE]>::random_for_test(rng)).unwrap() + } + } +} diff --git a/eth2/utils/bls/src/lib.rs b/eth2/utils/bls/src/lib.rs index e8d71ebeb..5067b1aba 100644 --- a/eth2/utils/bls/src/lib.rs +++ b/eth2/utils/bls/src/lib.rs @@ -4,10 +4,14 @@ extern crate ssz; #[macro_use] mod macros; mod keypair; +mod public_key_bytes; mod secret_key; +mod signature_bytes; pub use crate::keypair::Keypair; +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_on_g2}; #[cfg(feature = "fake_crypto")] diff --git a/eth2/utils/bls/src/macros.rs b/eth2/utils/bls/src/macros.rs index 4f41bac1d..5a84bb61a 100644 --- a/eth2/utils/bls/src/macros.rs +++ b/eth2/utils/bls/src/macros.rs @@ -84,3 +84,111 @@ macro_rules! impl_cached_tree_hash { } }; } + +macro_rules! bytes_struct { + ($name: ident, $type: ty, $byte_size: expr, $small_name: expr, $ssz_type_size: ident, + $type_str: expr, $byte_size_str: expr) => { + #[doc = "Stores `"] + #[doc = $byte_size_str] + #[doc = "` bytes which may or may not represent a valid BLS "] + #[doc = $small_name] + #[doc = ".\n\nThe `"] + #[doc = $type_str] + #[doc = "` struct performs validation when it is instantiated, where as this struct does \ + not. This struct is suitable where we may wish to store bytes that are \ + potentially not a valid "] + #[doc = $small_name] + #[doc = " (e.g., from the deposit contract)."] + #[derive(Clone)] + pub struct $name([u8; $byte_size]); + }; + ($name: ident, $type: ty, $byte_size: expr, $small_name: expr, $ssz_type_size: ident) => { + bytes_struct!($name, $type, $byte_size, $small_name, $ssz_type_size, stringify!($type), + stringify!($byte_size)); + + impl $name { + pub fn from_bytes(bytes: &[u8]) -> Result { + Ok(Self(Self::get_bytes(bytes)?)) + } + + pub fn empty() -> Self { + Self([0; $byte_size]) + } + + pub fn as_bytes(&self) -> Vec { + self.0.to_vec() + } + + fn get_bytes(bytes: &[u8]) -> Result<[u8; $byte_size], ssz::DecodeError> { + let mut result = [0; $byte_size]; + if bytes.len() != $byte_size { + Err(ssz::DecodeError::InvalidByteLength { + len: bytes.len(), + expected: $byte_size, + }) + } else { + result[..].copy_from_slice(bytes); + Ok(result) + } + } + } + + impl std::fmt::Debug for $name { + fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + self.0[..].fmt(formatter) + } + } + + impl PartialEq for $name { + fn eq(&self, other: &Self) -> bool { + &self.0[..] == &other.0[..] + } + } + + impl Eq for $name {} + + impl std::convert::TryInto<$type> for &$name { + type Error = ssz::DecodeError; + + fn try_into(self) -> Result<$type, Self::Error> { + <$type>::from_bytes(&self.0[..]) + } + } + + impl std::convert::From<$type> for $name { + fn from(obj: $type) -> Self { + // We know that obj.as_bytes() always has exactly $byte_size many bytes. + Self::from_bytes(obj.as_ssz_bytes().as_slice()).unwrap() + } + } + + impl_ssz!($name, $byte_size, "$type"); + + impl_tree_hash!($name, $ssz_type_size); + + impl_cached_tree_hash!($name, $ssz_type_size); + + impl serde::ser::Serialize for $name { + /// Serde serialization is compliant the Ethereum YAML test format. + fn serialize(&self, serializer: S) -> Result + where + S: serde::ser::Serializer, + { + serializer.serialize_str(&hex::encode(ssz::ssz_encode(self))) + } + } + + impl<'de> serde::de::Deserialize<'de> for $name { + /// Serde serialization is compliant the Ethereum YAML test format. + fn deserialize(deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + let bytes = deserializer.deserialize_str(serde_hex::HexVisitor)?; + let signature = Self::from_ssz_bytes(&bytes[..]) + .map_err(|e| serde::de::Error::custom(format!("invalid ssz ({:?})", e)))?; + Ok(signature) + } + } + }; +} diff --git a/eth2/utils/bls/src/public_key.rs b/eth2/utils/bls/src/public_key.rs index d78b5869b..5924baa4c 100644 --- a/eth2/utils/bls/src/public_key.rs +++ b/eth2/utils/bls/src/public_key.rs @@ -150,6 +150,15 @@ mod tests { assert_eq!(original, decoded); } + #[test] + pub fn test_byte_size() { + let sk = SecretKey::random(); + let original = PublicKey::from_secret_key(&sk); + + let bytes = ssz_encode(&original); + assert_eq!(bytes.len(), BLS_PUBLIC_KEY_BYTE_SIZE); + } + #[test] // TODO: once `CachedTreeHash` is fixed, this test should _not_ panic. #[should_panic] diff --git a/eth2/utils/bls/src/public_key_bytes.rs b/eth2/utils/bls/src/public_key_bytes.rs new file mode 100644 index 000000000..f75735140 --- /dev/null +++ b/eth2/utils/bls/src/public_key_bytes.rs @@ -0,0 +1,43 @@ +use ssz::{Decode, DecodeError, Encode}; + +use super::{PublicKey, BLS_PUBLIC_KEY_BYTE_SIZE}; + +bytes_struct!( + PublicKeyBytes, + PublicKey, + BLS_PUBLIC_KEY_BYTE_SIZE, + "public key", + U48 +); + +#[cfg(test)] +mod tests { + use std::convert::TryInto; + + use ssz::ssz_encode; + + use super::super::Keypair; + use super::*; + + #[test] + pub fn test_valid_public_key() { + let keypair = Keypair::random(); + + let bytes = ssz_encode(&keypair.pk); + let public_key_bytes = PublicKeyBytes::from_bytes(&bytes).unwrap(); + let public_key: Result = (&public_key_bytes).try_into(); + assert!(public_key.is_ok()); + assert_eq!(keypair.pk, public_key.unwrap()); + } + + #[test] + pub fn test_invalid_public_key() { + let mut public_key_bytes = [0; BLS_PUBLIC_KEY_BYTE_SIZE]; + public_key_bytes[0] = 255; //a_flag1 == b_flag1 == c_flag1 == 1 and x1 = 0 shouldn't be allowed + let public_key_bytes = PublicKeyBytes::from_bytes(&public_key_bytes[..]); + assert!(public_key_bytes.is_ok()); + + let public_key: Result = public_key_bytes.as_ref().unwrap().try_into(); + assert!(public_key.is_err()); + } +} diff --git a/eth2/utils/bls/src/signature.rs b/eth2/utils/bls/src/signature.rs index 20240039b..aba5fc1da 100644 --- a/eth2/utils/bls/src/signature.rs +++ b/eth2/utils/bls/src/signature.rs @@ -155,6 +155,15 @@ mod tests { assert_eq!(original, decoded); } + #[test] + pub fn test_byte_size() { + let keypair = Keypair::random(); + + let signature = Signature::new(&[42, 42], 0, &keypair.sk); + let bytes = ssz_encode(&signature); + assert_eq!(bytes.len(), BLS_SIG_BYTE_SIZE); + } + #[test] // TODO: once `CachedTreeHash` is fixed, this test should _not_ panic. #[should_panic] diff --git a/eth2/utils/bls/src/signature_bytes.rs b/eth2/utils/bls/src/signature_bytes.rs new file mode 100644 index 000000000..a30cecb4d --- /dev/null +++ b/eth2/utils/bls/src/signature_bytes.rs @@ -0,0 +1,44 @@ +use ssz::{Decode, DecodeError, Encode}; + +use super::{Signature, BLS_SIG_BYTE_SIZE}; + +bytes_struct!( + SignatureBytes, + Signature, + BLS_SIG_BYTE_SIZE, + "signature", + U96 +); + +#[cfg(test)] +mod tests { + use std::convert::TryInto; + + use ssz::ssz_encode; + + use super::super::Keypair; + use super::*; + + #[test] + pub fn test_valid_signature() { + let keypair = Keypair::random(); + let original = Signature::new(&[42, 42], 0, &keypair.sk); + + let bytes = ssz_encode(&original); + let signature_bytes = SignatureBytes::from_bytes(&bytes).unwrap(); + let signature: Result = (&signature_bytes).try_into(); + assert!(signature.is_ok()); + assert_eq!(original, signature.unwrap()); + } + + #[test] + pub fn test_invalid_signature() { + let mut signature_bytes = [0; BLS_SIG_BYTE_SIZE]; + signature_bytes[0] = 255; //a_flag1 == b_flag1 == c_flag1 == 1 and x1 = 0 shouldn't be allowed + let signature_bytes = SignatureBytes::from_bytes(&signature_bytes[..]); + assert!(signature_bytes.is_ok()); + + let signature: Result = signature_bytes.as_ref().unwrap().try_into(); + assert!(signature.is_err()); + } +}