fix!: update argon2 and improve key handling (#4892)

Description --- Updates `argon2` for the gRPC and wallet use cases. Improves handling of keys and secret data. Fixes [issue 4882](#4882). Motivation and Context --- [Issue 4882](#4882) notes that different versions of `argon2` are used throughout the codebase. The newer minor version `0.4` changes the API significantly, and the older version `0.2` is [no longer supported](https://crates.io/crates/argon2/0.2.0). Additionally, gRPC and wallet implementations use the default parameter set from the crate, which is not in line with the [OWASP recommendations](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id) that we use elsewhere in the key manager. Further, it's recommended that a [specific function](https://docs.rs/argon2/0.4.1/argon2/struct.Argon2.html#method.hash_password_into) be used when performing KDF functionality, as this binds the parameter set into the output. While not a major security issue, it's worth updating to this function where appropriate. Finally, secret data is kept in memory in many places through the codebase, and this area is no exception. As part of good practice, we should try to zeroize such data wherever possible. This PR addresses these issues. It updates the gRPC and wallet `argon2` versions to `0.4` (the key manager is addressed in [PR 4860](#4860) and makes the necessary API changes. It updates the parameter set to be consistent with the linked recommendations. It also adds some improved handling of secret data (but does not do so comprehensively, limiting the scope to the updated code). How Has This Been Tested? --- Existing tests pass. BREAKING: This changes how wallet passphrase-based hashes and keys are derived.
tari-project · Nov 7, 2022 · 9aa9087 · 9aa9087
1 parent 44ed0c8
commit 9aa9087
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 28 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/applications/tari_app_grpc/Cargo.toml b/applications/tari_app_grpc/Cargo.toml
@@ -15,7 +15,7 @@ tari_crypto = { git = "https://github.com/tari-project/tari-crypto.git", tag = "
 tari_script = { path = "../../infrastructure/tari_script" }
 tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", tag="v0.4.7" }
 
-argon2 = { version = "0.4.1", features = ["std"] }
+argon2 = { version = "0.4.1", features = ["std", "password-hash"] }
 base64 = "0.13.0"
 chrono = { version = "0.4.19", default-features = false }
 digest = "0.9"

diff --git a/applications/tari_app_grpc/src/authentication/salted_password.rs b/applications/tari_app_grpc/src/authentication/salted_password.rs
@@ -20,17 +20,36 @@
 // CAUSED AND ON ANY THEORY OF LIABILITY,  WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
 // OTHERWISE) ARISING IN ANY WAY OUT OF THE  USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
 // DAMAGE.
-use argon2::{password_hash::SaltString, Argon2, PasswordHasher};
+use argon2::{
+    password_hash::{Decimal, SaltString},
+    Argon2,
+    PasswordHasher,
+};
 use rand::rngs::OsRng;
 use zeroize::Zeroizing;
 
 pub fn create_salted_hashed_password(password: &[u8]) -> argon2::password_hash::Result<Zeroizing<String>> {
-    let argon2 = Argon2::default();
-    // Generate a random salt
+    // Generate a 16-byte random salt
     let passphrase_salt = SaltString::generate(&mut OsRng);
 
-    // Hash the password, this is placed in the configuration file
-    let hashed_password = argon2.hash_password(password, &passphrase_salt)?;
+    // Use the recommended OWASP parameters, which are not the default:
+    // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
+    let params = argon2::Params::new(
+        37 * 1024, // m-cost: 37 MiB, converted to KiB
+        1,         // t-cost
+        1,         // p-cost
+        None,      // output length: default
+    )?;
+
+    // Hash the password; this is placed in the configuration file
+    // We explicitly use the recommended algorithm and version due to the API
+    let hashed_password = Argon2::default().hash_password_customized(
+        password,
+        Some(argon2::Algorithm::Argon2id.ident()),
+        Some(argon2::Version::V0x13 as Decimal), // for some reason we need to use the numerical representation here
+        params,
+        &passphrase_salt,
+    )?;
 
     Ok(Zeroizing::new(hashed_password.to_string()))
 }
diff --git a/base_layer/wallet/Cargo.toml b/base_layer/wallet/Cargo.toml
@@ -29,7 +29,7 @@ tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", t
 tokio = { version = "1.20", features = ["sync", "macros"] }
 
 async-trait = "0.1.50"
-argon2 = "0.2"
+argon2 = "0.4.1"
 bincode = "1.3.1"
 blake2 = "0.9.0"
 sha2 = "0.9.5"
@@ -55,6 +55,7 @@ tower = "0.4"
 prost = "0.9"
 itertools = "0.10.3"
 chacha20poly1305 = "0.9.1"
+zeroize = "1.3"
 
 [dev-dependencies]
 tari_p2p = { version = "^0.38", path = "../p2p", features = ["test-mocks"] }

diff --git a/base_layer/wallet/src/storage/sqlite_db/wallet.rs b/base_layer/wallet/src/storage/sqlite_db/wallet.rs
@@ -28,7 +28,7 @@ use std::{
 };
 
 use argon2::{
-    password_hash::{rand_core::OsRng, PasswordHash, PasswordHasher, PasswordVerifier, SaltString},
+    password_hash::{rand_core::OsRng, Decimal, PasswordHash, PasswordHasher, PasswordVerifier, SaltString},
     Argon2,
 };
 use chacha20poly1305::{aead::NewAead, Key, Tag, XChaCha20Poly1305, XNonce};
@@ -47,6 +47,7 @@ use tari_utilities::{
     SafePassword,
 };
 use tokio::time::Instant;
+use zeroize::Zeroizing;
 
 use crate::{
     error::WalletStorageError,
@@ -408,23 +409,53 @@ impl WalletBackend for WalletSqliteDatabase {
             return Err(WalletStorageError::AlreadyEncrypted);
         }
 
-        let argon2 = Argon2::default();
-        let passphrase_salt = SaltString::generate(&mut OsRng);
+        // Use the recommended OWASP parameters, which are not the default:
+        // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
+
+        // These are the parameters for the passphrase hash
+        let params_passphrase = argon2::Params::new(
+            37 * 1024, // m-cost: 37 MiB, converted to KiB
+            1,         // t-cost
+            1,         // p-cost
+            None,      // output length: default is fine for this use
+        )
+        .map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
+
+        // These are the parameters for the encryption key
+        // We set them separately to ensure a proper key size
+        let params_encryption = argon2::Params::new(
+            37 * 1024,              // m-cost: 37 MiB, converted to KiB
+            1,                      // t-cost
+            1,                      // p-cost
+            Some(size_of::<Key>()), // output length: ChaCha20-Poly1305 key size
+        )
+        .map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
 
-        let passphrase_hash = argon2
-            .hash_password_simple(passphrase.reveal(), &passphrase_salt)
+        // Hash the passphrase to a PHC string for later verification
+        let passphrase_salt = SaltString::generate(&mut OsRng);
+        let passphrase_hash = argon2::Argon2::default()
+            .hash_password_customized(
+                passphrase.reveal(),
+                Some(argon2::Algorithm::Argon2id.ident()),
+                Some(argon2::Version::V0x13 as Decimal), // the API requires the numerical version representation
+                params_passphrase,
+                &passphrase_salt,
+            )
             .map_err(|e| WalletStorageError::AeadError(e.to_string()))?
             .to_string();
-        let encryption_salt = SaltString::generate(&mut OsRng);
 
-        let derived_encryption_key = argon2
-            .hash_password_simple(passphrase.reveal(), encryption_salt.as_str())
-            .map_err(|e| WalletStorageError::AeadError(e.to_string()))?
-            .hash
-            .ok_or_else(|| WalletStorageError::AeadError("Problem generating encryption key hash".to_string()))?;
+        // Hash the passphrase to produce a ChaCha20-Poly1305 key
+        let encryption_salt = SaltString::generate(&mut OsRng);
+        let mut derived_encryption_key = Zeroizing::new([0u8; size_of::<Key>()]);
+        argon2::Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params_encryption)
+            .hash_password_into(
+                passphrase.reveal(),
+                encryption_salt.as_bytes(),
+                derived_encryption_key.as_mut(),
+            )
+            .map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
 
-        let key = Key::from_slice(derived_encryption_key.as_bytes());
-        let cipher = XChaCha20Poly1305::new(key);
+        let cipher = XChaCha20Poly1305::new(Key::from_slice(derived_encryption_key.as_ref()));
 
         WalletSettingSql::new(DbKey::PassphraseHash.to_string(), passphrase_hash).set(&conn)?;
         WalletSettingSql::new(DbKey::EncryptionSalt.to_string(), encryption_salt.as_str().to_string()).set(&conn)?;
@@ -608,18 +639,33 @@ fn check_db_encryption_status(
             let stored_hash =
                 PasswordHash::new(&db_passphrase_hash).map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
 
+            // Check the passphrase PHC string against the provided passphrase
             if let Err(e) = argon2.verify_password(passphrase.reveal(), &stored_hash) {
                 error!(target: LOG_TARGET, "Incorrect passphrase ({})", e);
                 return Err(WalletStorageError::InvalidPassphrase);
             }
 
-            let derived_encryption_key = argon2
-                .hash_password_simple(passphrase.reveal(), encryption_salt.as_str())
-                .map_err(|e| WalletStorageError::AeadError(e.to_string()))?
-                .hash
-                .ok_or_else(|| WalletStorageError::AeadError("Problem generating encryption key hash".to_string()))?;
-            let key = Key::from_slice(derived_encryption_key.as_bytes());
-            Some(XChaCha20Poly1305::new(key))
+            // Use the recommended OWASP parameters, which are not the default:
+            // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
+            let params_encryption = argon2::Params::new(
+                37 * 1024,              // m-cost: 37 MiB, converted to KiB
+                1,                      // t-cost
+                1,                      // p-cost
+                Some(size_of::<Key>()), // output length: ChaCha20-Poly1305 key size
+            )
+            .map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
+
+            // Hash the passphrase to produce a ChaCha20-Poly1305 key
+            let mut derived_encryption_key = Zeroizing::new([0u8; size_of::<Key>()]);
+            argon2::Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params_encryption)
+                .hash_password_into(
+                    passphrase.reveal(),
+                    encryption_salt.as_bytes(),
+                    derived_encryption_key.as_mut(),
+                )
+                .map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
+
+            Some(XChaCha20Poly1305::new(Key::from_slice(derived_encryption_key.as_ref())))
         },
         _ => None,
     };