From 3db9072fee1b3096bd8749df4a3bbf8daf4c91ca Mon Sep 17 00:00:00 2001 From: Kirk Baird Date: Thu, 19 Nov 2020 00:37:43 +0000 Subject: [PATCH] Reject invalid utf-8 characters during encryption (#1928) ## Issue Addressed Closes #1889 ## Proposed Changes - Error when passwords which use invalid UTF-8 characters during encryption. - Add some tests ## Additional Info I've decided to error when bad characters are used to create/encrypt a keystore but think we should allow them during decryption since either the keystore was created - with invalid UTF-8 characters (possibly by another client or someone whose password is random bytes) in which case we'd want them to be able to decrypt their keystore using the right key. - without invalid characters then the password checksum would almost certainly fail. Happy to add them to decryption if we want to make the decryption more trigger happy :yum: , it would only be a one line change and would tell the user which character index is causing the issue. See https://eips.ethereum.org/EIPS/eip-2335#password-requirements --- .../eth2_wallet_manager/src/wallet_manager.rs | 2 +- common/validator_dir/src/insecure_keys.rs | 4 +- common/validator_dir/tests/tests.rs | 2 +- crypto/eth2_keystore/src/keystore.rs | 36 +++++++++++- crypto/eth2_keystore/tests/tests.rs | 57 +++++++++++++++++++ validator_client/src/http_api/tests.rs | 2 +- 6 files changed, 97 insertions(+), 6 deletions(-) diff --git a/common/eth2_wallet_manager/src/wallet_manager.rs b/common/eth2_wallet_manager/src/wallet_manager.rs index d1bef6734..df30cfe4c 100644 --- a/common/eth2_wallet_manager/src/wallet_manager.rs +++ b/common/eth2_wallet_manager/src/wallet_manager.rs @@ -299,7 +299,7 @@ mod tests { ); for i in 1..3 { - w.next_validator(WALLET_PASSWORD, &[1], &[0]) + w.next_validator(WALLET_PASSWORD, &[50; 32], &[51; 32]) .expect("should create validator"); assert_eq!( load_wallet_raw(&base_dir, &uuid).nextaccount(), diff --git a/common/validator_dir/src/insecure_keys.rs b/common/validator_dir/src/insecure_keys.rs index 8043db749..f8cc51da6 100644 --- a/common/validator_dir/src/insecure_keys.rs +++ b/common/validator_dir/src/insecure_keys.rs @@ -13,7 +13,7 @@ use std::path::PathBuf; use types::test_utils::generate_deterministic_keypair; /// A very weak password with which to encrypt the keystores. -pub const INSECURE_PASSWORD: &[u8] = &[30; 32]; +pub const INSECURE_PASSWORD: &[u8] = &[50; 51]; impl<'a> Builder<'a> { /// Generate the voting keystore using a deterministic, well-known, **unsafe** keypair. @@ -59,7 +59,7 @@ fn insecure_kdf() -> Kdf { n: 2, p: 1, r: 8, - salt: vec![1, 3, 3, 5].into(), + salt: vec![1; 32].into(), }) } diff --git a/common/validator_dir/tests/tests.rs b/common/validator_dir/tests/tests.rs index fd1b79f14..3a54c22f5 100644 --- a/common/validator_dir/tests/tests.rs +++ b/common/validator_dir/tests/tests.rs @@ -11,7 +11,7 @@ use validator_dir::{ }; /// A very weak password with which to encrypt the keystores. -pub const INSECURE_PASSWORD: &[u8] = &[30; 32]; +pub const INSECURE_PASSWORD: &[u8] = &[50; 51]; /// Helper struct for configuring tests. struct BuildConfig { diff --git a/crypto/eth2_keystore/src/keystore.rs b/crypto/eth2_keystore/src/keystore.rs index c1997680d..2518ff0fb 100644 --- a/crypto/eth2_keystore/src/keystore.rs +++ b/crypto/eth2_keystore/src/keystore.rs @@ -59,6 +59,7 @@ pub const HASH_SIZE: usize = 32; pub enum Error { InvalidSecretKeyLen { len: usize, expected: usize }, InvalidPassword, + InvalidPasswordCharacter { character: u8, index: usize }, InvalidSecretKeyBytes(bls::Error), PublicKeyMismatch, EmptyPassword, @@ -158,6 +159,8 @@ 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)?; @@ -322,7 +325,8 @@ pub fn default_kdf(salt: Vec) -> Kdf { /// /// ## Errors /// -/// - The `kdf` is badly formed (e.g., has some values set to zero). +/// - 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], @@ -384,6 +388,36 @@ 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, + }); + } + + // 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(()) +} + /// Generates a checksum to indicate that the `derived_key` is associated with the /// `cipher_message`. fn generate_checksum(derived_key: &DerivedKey, cipher_message: &[u8]) -> [u8; HASH_SIZE] { diff --git a/crypto/eth2_keystore/tests/tests.rs b/crypto/eth2_keystore/tests/tests.rs index 911086919..7311b04fa 100644 --- a/crypto/eth2_keystore/tests/tests.rs +++ b/crypto/eth2_keystore/tests/tests.rs @@ -160,3 +160,60 @@ fn custom_pbkdf2_kdf() { assert_eq!(keystore.kdf(), &my_kdf); } + +#[test] +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 invalid_character = 0x1Fu8; + let invalid_password = [50, invalid_character, 50]; + let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) + .unwrap() + .build(); + assert_eq!( + keystore, + Err(Error::InvalidPasswordCharacter { + character: invalid_character, + index: 1 + }) + ); + + let invalid_character = 0x80u8; + let invalid_password = [50, 50, invalid_character]; + let keystore = KeystoreBuilder::new(&keypair, &invalid_password, "".into()) + .unwrap() + .build(); + assert_eq!( + keystore, + Err(Error::InvalidPasswordCharacter { + character: invalid_character, + index: 2 + }) + ); + + 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 + }) + ); +} diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index eef3aa8ae..fd7f3d6ed 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -25,7 +25,7 @@ use std::sync::Arc; use tempfile::{tempdir, TempDir}; use tokio::sync::oneshot; -const PASSWORD_BYTES: &[u8] = &[42, 13, 37]; +const PASSWORD_BYTES: &[u8] = &[42, 50, 37]; type E = MainnetEthSpec;