Remove control characters instead of returning an error

sigp · Nov 25, 2020 · c5daab7 · c5daab7
1 parent 7f6e5f9
commit c5daab7
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 78 deletions.
diff --git a/crypto/eth2_keystore/src/keystore.rs b/crypto/eth2_keystore/src/keystore.rs
@@ -65,7 +65,6 @@ pub enum Error {
     InvalidSecretKeyLen { len: usize, expected: usize },
     InvalidPassword,
     InvalidPasswordBytes,
-    InvalidPasswordCharacter { character: u8, index: usize },
     InvalidSecretKeyBytes(bls::Error),
     PublicKeyMismatch,
     EmptyPassword,
@@ -166,8 +165,6 @@ 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)?;
@@ -337,6 +334,8 @@ pub fn encrypt(
     validate_parameters(kdf)?;
     let mut password = normalize(password)?;
 
+    password.retain(|c| !is_control_character(c));
+
     let derived_key = derive_key(&password.as_bytes(), &kdf)?;
 
     // TODO: remove if using `ZeroizeString`
@@ -371,6 +370,8 @@ pub fn encrypt(
 pub fn decrypt(password: &[u8], crypto: &Crypto) -> Result<PlainText, Error> {
     let mut password = normalize(password)?;
 
+    password.retain(|c| !is_control_character(c));
+
     validate_parameters(&crypto.kdf.params)?;
 
     let cipher_message = &crypto.cipher.message;
@@ -404,34 +405,10 @@ 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(())
+/// Returns true if the given char is a UTF-8 control character and false otherwise.
+pub fn is_control_character(c: char) -> bool {
+    // 0x00 - 0x1F + 0x80 - 0x9F + 0x7F
+    c.is_control()
 }
 
 /// Takes a slice of bytes and returns a NKFD normalized string representation.

diff --git a/crypto/eth2_keystore/tests/tests.rs b/crypto/eth2_keystore/tests/tests.rs
@@ -250,57 +250,29 @@ 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);
 }
 
 fn print(s: &str) {
@@ -335,7 +307,7 @@ fn normalization() {
         .build()
         .unwrap();
 
-    assert_eq!(keystore_nfc, keystore_nfkd);
+    assert_eq!(keystore_nfc.pubkey(), keystore_nfkd.pubkey());
 
     // Decode same keystore with nfc and nfkd form passwords
     let decoded_nfc = keystore_nfc