From c5e97b9bf7758991fd224bc5b77db602cfe17eac Mon Sep 17 00:00:00 2001 From: Kirk Baird Date: Thu, 19 Nov 2020 08:52:51 +0000 Subject: [PATCH] Add validation to kdf parameters (#1930) ## Issue Addressed Closes #1906 Closes #1907 ## Proposed Changes - Emits warnings when the KDF parameters are two low. - Returns errors when the KDF parameters are high enough to pose a potential DoS threat. - Validates AES IV length is 128 bits, errors if empty, warnings otherwise. ## Additional Info NIST advice used for PBKDF2 ranges https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-132.pdf. Scrypt ranges are based on the maximum value of the `u32` (i.e 4GB of memory) The minimum range has been set to anything below the default fields. --- crypto/eth2_keystore/Cargo.toml | 1 + .../src/json_keystore/kdf_module.rs | 13 ++ crypto/eth2_keystore/src/keystore.rs | 206 +++++++++++++----- crypto/eth2_keystore/tests/tests.rs | 95 +++++++- 4 files changed, 260 insertions(+), 55 deletions(-) diff --git a/crypto/eth2_keystore/Cargo.toml b/crypto/eth2_keystore/Cargo.toml index 63508b5e5..4619f8175 100644 --- a/crypto/eth2_keystore/Cargo.toml +++ b/crypto/eth2_keystore/Cargo.toml @@ -22,5 +22,6 @@ bls = { path = "../bls" } eth2_ssz = "0.1.2" serde_json = "1.0.58" eth2_key_derivation = { path = "../eth2_key_derivation" } + [dev-dependencies] tempfile = "3.1.0" diff --git a/crypto/eth2_keystore/src/json_keystore/kdf_module.rs b/crypto/eth2_keystore/src/json_keystore/kdf_module.rs index 89fdd219e..c537e4d69 100644 --- a/crypto/eth2_keystore/src/json_keystore/kdf_module.rs +++ b/crypto/eth2_keystore/src/json_keystore/kdf_module.rs @@ -4,6 +4,7 @@ //! data structures. Specifically, there should not be any actual crypto logic in this file. use super::hex_bytes::HexBytes; +use crate::DKLEN; use hmac::{Hmac, Mac, NewMac}; use serde::{Deserialize, Serialize}; use sha2::Sha256; @@ -128,3 +129,15 @@ pub struct Scrypt { pub p: u32, pub salt: HexBytes, } + +impl Scrypt { + pub fn default_scrypt(salt: Vec) -> Self { + Self { + dklen: DKLEN, + n: 262144, + p: 1, + r: 8, + salt: salt.into(), + } + } +} diff --git a/crypto/eth2_keystore/src/keystore.rs b/crypto/eth2_keystore/src/keystore.rs index 2518ff0fb..3f1a8d47b 100644 --- a/crypto/eth2_keystore/src/keystore.rs +++ b/crypto/eth2_keystore/src/keystore.rs @@ -54,6 +54,8 @@ pub const DKLEN: u32 = 32; pub const IV_SIZE: usize = 16; /// The byte size of a SHA256 hash. pub const HASH_SIZE: usize = 32; +/// The default iteraction count, `c`, for PBKDF2. +pub const DEFAULT_PBKDF2_C: u32 = 262_144; #[derive(Debug, PartialEq)] pub enum Error { @@ -69,6 +71,7 @@ pub enum Error { ReadError(String), InvalidPbkdf2Param, InvalidScryptParam, + InvalidSaltLength, IncorrectIvSize { expected: usize, len: usize }, ScryptInvalidParams(InvalidParams), ScryptInvaidOutputLen(InvalidOutputLen), @@ -311,13 +314,7 @@ pub fn keypair_from_secret(secret: &[u8]) -> Result { /// /// Currently this is set to scrypt due to its memory hardness properties. pub fn default_kdf(salt: Vec) -> Kdf { - Kdf::Scrypt(Scrypt { - dklen: DKLEN, - n: 262144, - p: 1, - r: 8, - salt: salt.into(), - }) + Kdf::Scrypt(Scrypt::default_scrypt(salt)) } /// Returns `(cipher_text, checksum)` for the given `plain_text` encrypted with `Cipher` using a @@ -333,12 +330,18 @@ pub fn encrypt( kdf: &Kdf, cipher: &Cipher, ) -> Result<(Vec, [u8; HASH_SIZE]), Error> { + validate_parameters(kdf)?; + let derived_key = derive_key(&password, &kdf)?; // Encrypt secret. let mut cipher_text = plain_text.to_vec(); match &cipher { Cipher::Aes128Ctr(params) => { + // Validate IV + validate_aes_iv(params.iv.as_bytes())?; + + // AES Encrypt let key = GenericArray::from_slice(&derived_key.as_bytes()[0..16]); let nonce = GenericArray::from_slice(params.iv.as_bytes()); let mut cipher = AesCtr::new(&key, &nonce); @@ -358,6 +361,8 @@ pub fn encrypt( /// - 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 { + validate_parameters(&crypto.kdf.params)?; + let cipher_message = &crypto.cipher.message; // Generate derived key @@ -373,13 +378,11 @@ pub fn decrypt(password: &[u8], crypto: &Crypto) -> Result { let mut plain_text = PlainText::from(cipher_message.as_bytes().to_vec()); match &crypto.cipher.params { Cipher::Aes128Ctr(params) => { + // Validate IV + validate_aes_iv(params.iv.as_bytes())?; + + // AES Decrypt let key = GenericArray::from_slice(&derived_key.as_bytes()[0..16]); - // NOTE: we do not check the size of the `iv` as there is no guidance about - // this on EIP-2335. - // - // Reference: - // - // - https://github.com/ethereum/EIPs/issues/2339#issuecomment-623865023 let nonce = GenericArray::from_slice(params.iv.as_bytes()); let mut cipher = AesCtr::new(&key, &nonce); cipher.apply_keystream(plain_text.as_mut_bytes()); @@ -436,21 +439,6 @@ fn derive_key(password: &[u8], kdf: &Kdf) -> Result { match &kdf { Kdf::Pbkdf2(params) => { - // RFC2898 declares that `c` must be a "positive integer" and the `crypto` crate panics - // if it is `0`. - // - // Both of these seem fairly convincing that it shouldn't be 0. - // - // Reference: - // - // https://www.ietf.org/rfc/rfc2898.txt - // - // Additionally, we always compute a derived key of 32 bytes so reject anything that - // says otherwise. - if params.c == 0 || params.dklen != DKLEN { - return Err(Error::InvalidPbkdf2Param); - } - pbkdf2::>( password, params.salt.as_bytes(), @@ -459,27 +447,6 @@ fn derive_key(password: &[u8], kdf: &Kdf) -> Result { ); } Kdf::Scrypt(params) => { - // RFC7914 declares that all these parameters must be greater than 1: - // - // - `N`: costParameter. - // - `r`: blockSize. - // - `p`: parallelizationParameter - // - // Reference: - // - // https://tools.ietf.org/html/rfc7914 - // - // Additionally, we always compute a derived key of 32 bytes so reject anything that - // says otherwise. - if params.n <= 1 || params.r == 0 || params.p == 0 || params.dklen != DKLEN { - return Err(Error::InvalidScryptParam); - } - - // Ensure that `n` is power of 2. - if params.n != 2u32.pow(log2_int(params.n)) { - return Err(Error::InvalidScryptParam); - } - scrypt( password, params.salt.as_bytes(), @@ -494,10 +461,149 @@ fn derive_key(password: &[u8], kdf: &Kdf) -> Result { Ok(dk) } -/// Compute floor of log2 of a u32. +// Compute floor of log2 of a u32. fn log2_int(x: u32) -> u32 { if x == 0 { return 0; } 31 - x.leading_zeros() } + +// We only check the size of the `iv` is non-zero as there is no guidance about +// this on EIP-2335. +// +// Reference: +// +// - https://github.com/ethereum/EIPs/issues/2339#issuecomment-623865023 +fn validate_aes_iv(iv: &[u8]) -> Result<(), Error> { + if iv.is_empty() { + return Err(Error::IncorrectIvSize { + expected: IV_SIZE, + len: iv.len(), + }); + } else if iv.len() != IV_SIZE { + eprintln!( + "WARN: AES IV length incorrect is {}, should be {}", + iv.len(), + IV_SIZE + ); + } + Ok(()) +} + +// Validates the kdf parameters to ensure they are sufficiently secure, in addition to +// preventing DoS attacks from excessively large parameters. +fn validate_parameters(kdf: &Kdf) -> Result<(), Error> { + match kdf { + Kdf::Pbkdf2(params) => { + // We always compute a derived key of 32 bytes so reject anything that + // says otherwise. + if params.dklen != DKLEN { + return Err(Error::InvalidPbkdf2Param); + } + + // NIST Recommends suggests potential use cases where `c` of 10,000,000 is desireable. + // As it is 10 years old this has been increased to 80,000,000. Larger values will + // take over 1 minute to execute on an average machine. + // + // Reference: + // + // https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-132.pdf + if params.c > 80_000_000 { + return Err(Error::InvalidPbkdf2Param); + } + + // RFC2898 declares that `c` must be a "positive integer" and the `crypto` crate panics + // if it is `0`. + // + // Reference: + // + // https://www.ietf.org/rfc/rfc2898.txt + if params.c < DEFAULT_PBKDF2_C { + if params.c == 0 { + return Err(Error::InvalidPbkdf2Param); + } + eprintln!( + "WARN: PBKDF2 parameters are too weak, 'c' is {}, we recommend using {}", + params.c, DEFAULT_PBKDF2_C, + ); + } + + // Validate `salt` length. + validate_salt(params.salt.as_bytes())?; + + Ok(()) + } + Kdf::Scrypt(params) => { + // RFC7914 declares that all these parameters must be greater than 1: + // + // - `N`: costParameter. + // - `r`: blockSize. + // - `p`: parallelizationParameter + // + // Reference: + // + // https://tools.ietf.org/html/rfc7914 + if params.n <= 1 || params.r == 0 || params.p == 0 { + return Err(Error::InvalidScryptParam); + } + + // We always compute a derived key of 32 bytes so reject anything that + // says otherwise. + if params.dklen != DKLEN { + return Err(Error::InvalidScryptParam); + } + + // Ensure that `n` is power of 2. + if params.n != 2u32.pow(log2_int(params.n)) { + return Err(Error::InvalidScryptParam); + } + + // Maximum Parameters + // + // Uses a u32 to store value thus maximum memory usage is 4GB. + // + // Note: Memory requirements = 128*n*p*r + let mut npr: u32 = params + .n + .checked_mul(params.p) + .ok_or(Error::InvalidScryptParam)?; + npr = npr.checked_mul(params.r).ok_or(Error::InvalidScryptParam)?; + npr = npr.checked_mul(128).ok_or(Error::InvalidScryptParam)?; + + // Minimum Parameters + let default_kdf = Scrypt::default_scrypt(vec![0u8; 32]); + let default_npr = 128 * default_kdf.n * default_kdf.p * default_kdf.r; + if npr < default_npr { + eprintln!("WARN: Scrypt parameters are too weak (n: {}, p: {}, r: {}), we recommend (n: {}, p: {}, r: {})", params.n, params.p, params.r, default_kdf.n, default_kdf.p, default_kdf.r); + } + + // Validate `salt` length. + validate_salt(params.salt.as_bytes())?; + + Ok(()) + } + } +} + +// Validates that the salt is non-zero in length. +// Emits a warning if the salt is outside reasonable bounds. +fn validate_salt(salt: &[u8]) -> Result<(), Error> { + // Validate `salt` length + if salt.is_empty() { + return Err(Error::InvalidSaltLength); + } else if salt.len() < SALT_SIZE / 2 { + eprintln!( + "WARN: Salt is too short {}, we recommend {}", + salt.len(), + SALT_SIZE + ); + } else if salt.len() > SALT_SIZE * 2 { + eprintln!( + "WARN: Salt is too long {}, we recommend {}", + salt.len(), + SALT_SIZE + ); + } + Ok(()) +} diff --git a/crypto/eth2_keystore/tests/tests.rs b/crypto/eth2_keystore/tests/tests.rs index 7311b04fa..365c9410d 100644 --- a/crypto/eth2_keystore/tests/tests.rs +++ b/crypto/eth2_keystore/tests/tests.rs @@ -90,33 +90,118 @@ fn file() { #[test] fn scrypt_params() { let keypair = Keypair::random(); + let salt = vec![42; 32]; let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) .unwrap() .build() .unwrap(); - let json = keystore.to_json_string().unwrap(); let decoded = Keystore::from_json_str(&json).unwrap(); - assert_eq!( decoded.decrypt_keypair(BAD_PASSWORD).err().unwrap(), Error::InvalidPassword, "should not decrypt with bad password" ); - assert_eq!( decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk, keypair.pk, "should decrypt with good password" ); + + // n <= 1 + let my_kdf = Kdf::Scrypt(Scrypt { + dklen: DKLEN, + n: 1, + p: 1, + r: 8, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidScryptParam)); + + // p != 0 + let my_kdf = Kdf::Scrypt(Scrypt { + dklen: DKLEN, + n: 16, + p: 0, + r: 8, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidScryptParam)); + + // r != 0 + let my_kdf = Kdf::Scrypt(Scrypt { + dklen: DKLEN, + n: 16, + p: 1, + r: 0, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidScryptParam)); + + // 128 * n * p * r overflow + let my_kdf = Kdf::Scrypt(Scrypt { + dklen: DKLEN, + n: 1 << 31, + p: 1 << 31, + r: 1 << 31, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidScryptParam)); +} + +#[test] +fn pbkdf2_params() { + let keypair = Keypair::random(); + + let salt = vec![42; 32]; + + let my_kdf = Kdf::Pbkdf2(Pbkdf2 { + dklen: DKLEN, + c: 80_000_001, + prf: Prf::HmacSha256, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidPbkdf2Param)); + + let my_kdf = Kdf::Pbkdf2(Pbkdf2 { + dklen: DKLEN + 1, + c: 4, + prf: Prf::HmacSha256, + salt: salt.clone().into(), + }); + let keystore = KeystoreBuilder::new(&keypair, GOOD_PASSWORD, "".into()) + .unwrap() + .kdf(my_kdf.clone()) + .build(); + assert_eq!(keystore, Err(Error::InvalidPbkdf2Param)); } #[test] fn custom_scrypt_kdf() { let keypair = Keypair::random(); - let salt = vec![42]; + let salt = vec![42; 32]; let my_kdf = Kdf::Scrypt(Scrypt { dklen: DKLEN, @@ -141,7 +226,7 @@ fn custom_scrypt_kdf() { fn custom_pbkdf2_kdf() { let keypair = Keypair::random(); - let salt = vec![42]; + let salt = vec![42; 32]; let my_kdf = Kdf::Pbkdf2(Pbkdf2 { dklen: DKLEN,