From e1353088e020e6805f10918786c6d54bfa1c2897 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 3 Dec 2020 22:07:09 +0000 Subject: [PATCH] Normalize keystore passwords (#1972) ## Issue Addressed Resolves #1879 ## Proposed Changes Do NFKD normalization for keystore passwords. --- Cargo.lock | 1 + crypto/eth2_keystore/Cargo.toml | 1 + crypto/eth2_keystore/src/keystore.rs | 104 ++++++++++++++++++--------- crypto/eth2_keystore/tests/tests.rs | 100 ++++++++++++++------------ 4 files changed, 127 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7eab338aa..94d59a9cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2087,6 +2087,7 @@ dependencies = [ "serde_repr", "sha2 0.9.2", "tempfile", + "unicode-normalization", "uuid", "zeroize", ] diff --git a/crypto/eth2_keystore/Cargo.toml b/crypto/eth2_keystore/Cargo.toml index 4619f8175..d761b6e1e 100644 --- a/crypto/eth2_keystore/Cargo.toml +++ b/crypto/eth2_keystore/Cargo.toml @@ -22,6 +22,7 @@ bls = { path = "../bls" } eth2_ssz = "0.1.2" serde_json = "1.0.58" eth2_key_derivation = { path = "../eth2_key_derivation" } +unicode-normalization = "0.1.16" [dev-dependencies] tempfile = "3.1.0" diff --git a/crypto/eth2_keystore/src/keystore.rs b/crypto/eth2_keystore/src/keystore.rs index 3f1a8d47b..e16a565bf 100644 --- a/crypto/eth2_keystore/src/keystore.rs +++ b/crypto/eth2_keystore/src/keystore.rs @@ -23,7 +23,11 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::fs::OpenOptions; use std::io::{Read, Write}; +use std::iter::FromIterator; use std::path::Path; +use std::str; +use unicode_normalization::UnicodeNormalization; +use zeroize::Zeroize; /// The byte-length of a BLS secret key. const SECRET_KEY_LEN: usize = 32; @@ -57,11 +61,50 @@ pub const HASH_SIZE: usize = 32; /// The default iteraction count, `c`, for PBKDF2. pub const DEFAULT_PBKDF2_C: u32 = 262_144; +/// Provides a new-type wrapper around `String` that is zeroized on `Drop`. +/// +/// Useful for ensuring that password memory is zeroed-out on drop. +#[derive(Clone, PartialEq, Serialize, Deserialize, Zeroize)] +#[zeroize(drop)] +#[serde(transparent)] +struct ZeroizeString(String); + +impl From for ZeroizeString { + fn from(s: String) -> Self { + Self(s) + } +} + +impl AsRef<[u8]> for ZeroizeString { + fn as_ref(&self) -> &[u8] { + self.0.as_bytes() + } +} + +impl std::ops::Deref for ZeroizeString { + type Target = String; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::ops::DerefMut for ZeroizeString { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl FromIterator for ZeroizeString { + fn from_iter>(iter: T) -> Self { + ZeroizeString(String::from_iter(iter)) + } +} + #[derive(Debug, PartialEq)] pub enum Error { InvalidSecretKeyLen { len: usize, expected: usize }, InvalidPassword, - InvalidPasswordCharacter { character: u8, index: usize }, + InvalidPasswordBytes, InvalidSecretKeyBytes(bls::Error), PublicKeyMismatch, EmptyPassword, @@ -162,8 +205,6 @@ impl Keystore { path: String, description: String, ) -> Result { - validate_password_utf8_characters(password)?; - let secret: ZeroizeHash = keypair.sk.serialize(); let (cipher_text, checksum) = encrypt(secret.as_bytes(), password, &kdf, &cipher)?; @@ -319,11 +360,12 @@ pub fn default_kdf(salt: Vec) -> Kdf { /// Returns `(cipher_text, checksum)` for the given `plain_text` encrypted with `Cipher` using a /// key derived from `password` via the `Kdf` (key derivation function). +/// Normalizes the password into NFKD form and removes control characters as specified in EIP-2335 +/// before encryption. /// /// ## Errors /// /// - If `kdf` is badly formed (e.g., has some values set to zero). -/// - If `password` uses utf-8 control characters. pub fn encrypt( plain_text: &[u8], password: &[u8], @@ -331,8 +373,11 @@ pub fn encrypt( cipher: &Cipher, ) -> Result<(Vec, [u8; HASH_SIZE]), Error> { validate_parameters(kdf)?; + let mut password = normalize(password)?; - let derived_key = derive_key(&password, &kdf)?; + password.retain(|c| !is_control_character(c)); + + let derived_key = derive_key(&password.as_ref(), &kdf)?; // Encrypt secret. let mut cipher_text = plain_text.to_vec(); @@ -355,18 +400,24 @@ pub fn encrypt( } /// Regenerate some `plain_text` from the given `password` and `crypto`. +/// Normalizes the password into NFKD form and removes control characters as specified in EIP-2335 +/// before decryption. /// /// ## Errors /// /// - The provided password is incorrect. /// - The `crypto.kdf` is badly formed (e.g., has some values set to zero). pub fn decrypt(password: &[u8], crypto: &Crypto) -> Result { + let mut password = normalize(password)?; + + password.retain(|c| !is_control_character(c)); + validate_parameters(&crypto.kdf.params)?; let cipher_message = &crypto.cipher.message; // Generate derived key - let derived_key = derive_key(password, &crypto.kdf.params)?; + let derived_key = derive_key(password.as_ref(), &crypto.kdf.params)?; // Mismatching checksum indicates an invalid password. if &generate_checksum(&derived_key, cipher_message.as_bytes())[..] @@ -391,34 +442,21 @@ pub fn decrypt(password: &[u8], crypto: &Crypto) -> Result { Ok(plain_text) } -/// Verifies that a password does not contain UTF-8 control characters. -pub fn validate_password_utf8_characters(password: &[u8]) -> Result<(), Error> { - for (i, char) in password.iter().enumerate() { - // C0 - 0x00 to 0x1F - if *char <= 0x1F { - return Err(Error::InvalidPasswordCharacter { - character: *char, - index: i, - }); - } +/// Returns true if the given char is a control character as specified by EIP 2335 and false otherwise. +fn is_control_character(c: char) -> bool { + // Note: The control codes specified in EIP 2335 are same as the unicode control characters. + // (0x00 to 0x1F) + (0x80 to 0x9F) + 0x7F + c.is_control() +} - // C1 - 0x80 to 0x9F - if *char >= 0x80 && *char <= 0x9F { - return Err(Error::InvalidPasswordCharacter { - character: *char, - index: i, - }); - } - - // Backspace - if *char == 0x7F { - return Err(Error::InvalidPasswordCharacter { - character: *char, - index: i, - }); - } - } - Ok(()) +/// Takes a slice of bytes and returns a NFKD normalized string representation. +/// +/// Returns an error if the bytes are not valid utf8. +fn normalize(bytes: &[u8]) -> Result { + Ok(str::from_utf8(bytes) + .map_err(|_| Error::InvalidPasswordBytes)? + .nfkd() + .collect::()) } /// Generates a checksum to indicate that the `derived_key` is associated with the diff --git a/crypto/eth2_keystore/tests/tests.rs b/crypto/eth2_keystore/tests/tests.rs index 365c9410d..3472ee6f1 100644 --- a/crypto/eth2_keystore/tests/tests.rs +++ b/crypto/eth2_keystore/tests/tests.rs @@ -250,55 +250,63 @@ fn custom_pbkdf2_kdf() { fn utf8_control_characters() { let keypair = Keypair::random(); - let invalid_character = 0u8; - let invalid_password = [invalid_character]; - let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) - .unwrap() - .build(); - assert_eq!( - keystore, - Err(Error::InvalidPasswordCharacter { - character: invalid_character, - index: 0 - }) - ); + let password = vec![42, 42, 42]; + let password_with_control_chars = vec![0x7Fu8, 42, 42, 42]; - let invalid_character = 0x1Fu8; - let invalid_password = [50, invalid_character, 50]; - let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) + let keystore1 = KeystoreBuilder::new(&keypair, &password_with_control_chars, "".into()) .unwrap() - .build(); - assert_eq!( - keystore, - Err(Error::InvalidPasswordCharacter { - character: invalid_character, - index: 1 - }) - ); + .build() + .unwrap(); - let invalid_character = 0x80u8; - let invalid_password = [50, 50, invalid_character]; - let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) + let keystore2 = KeystoreBuilder::new(&keypair, &password, "".into()) .unwrap() - .build(); - assert_eq!( - keystore, - Err(Error::InvalidPasswordCharacter { - character: invalid_character, - index: 2 - }) - ); + .build() + .unwrap(); - let invalid_character = 0x7Fu8; - let invalid_password = [50, 50, 50, 50, 50, 50, invalid_character]; - let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) - .unwrap() - .build(); - assert_eq!( - keystore, - Err(Error::InvalidPasswordCharacter { - character: invalid_character, - index: 6 - }) - ); + assert_eq!(keystore1.pubkey(), keystore2.pubkey()); + + // Decode same keystore with nfc and nfkd form passwords + let decoded1 = keystore1 + .decrypt_keypair(&password_with_control_chars) + .unwrap(); + let decoded2 = keystore1.decrypt_keypair(&password).unwrap(); + + assert_eq!(decoded1.pk, keypair.pk); + assert_eq!(decoded2.pk, keypair.pk); +} + +#[test] +fn normalization() { + use unicode_normalization::UnicodeNormalization; + + let keypair = Keypair::random(); + let password_str = "Zoë"; + + let password_nfc: String = password_str.nfc().collect(); + let password_nfkd: String = password_str.nfkd().collect(); + + assert_ne!(password_nfc, password_nfkd); + + let keystore_nfc = KeystoreBuilder::new(&keypair, password_nfc.as_bytes(), "".into()) + .unwrap() + .build() + .unwrap(); + + let keystore_nfkd = KeystoreBuilder::new(&keypair, password_nfkd.as_bytes(), "".into()) + .unwrap() + .build() + .unwrap(); + + assert_eq!(keystore_nfc.pubkey(), keystore_nfkd.pubkey()); + + // Decode same keystore with nfc and nfkd form passwords + let decoded_nfc = keystore_nfc + .decrypt_keypair(password_nfc.as_bytes()) + .unwrap(); + let decoded_nfkd = keystore_nfc + .decrypt_keypair(password_nfkd.as_bytes()) + .unwrap(); + + assert_eq!(decoded_nfc.pk, keypair.pk); + assert_eq!(decoded_nfkd.pk, keypair.pk); }