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 😋 , 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

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(),

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(),
     })
 }
 

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 {

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<Self, Error> {
+        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<u8>) -> 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<PlainText, Error> {
     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] {

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
+        })
+    );
+}

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;