From c33f82aec8e26e861830a4f65a7352dfc69f349c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Garc=C3=ADa?=
 <dani-garcia@users.noreply.github.com>
Date: Mon, 30 Sep 2024 17:27:01 +0200
Subject: [PATCH] Validate minimum KDF settings (#1100)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## 🎟️ Tracking

<!-- Paste the link to the Jira or GitHub issue or otherwise describe /
point to where this change is coming from. -->

## 📔 Objective

Previously we weren't validating the KDF settings that we got from the
server prelogin message.

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes
---
 crates/bitwarden-crypto/src/error.rs      |  3 +
 crates/bitwarden-crypto/src/keys/utils.rs | 79 +++++++++++++++++++----
 2 files changed, 70 insertions(+), 12 deletions(-)

diff --git a/crates/bitwarden-crypto/src/error.rs b/crates/bitwarden-crypto/src/error.rs
index cd2ef9ac0..2f9a58b8b 100644
--- a/crates/bitwarden-crypto/src/error.rs
+++ b/crates/bitwarden-crypto/src/error.rs
@@ -24,6 +24,9 @@ pub enum CryptoError {
     #[error("The item was missing a required field: {0}")]
     MissingField(&'static str),
 
+    #[error("Insufficient KDF parameters")]
+    InsufficientKdfParameters,
+
     #[error("EncString error, {0}")]
     EncString(#[from] EncStringParseError),
 
diff --git a/crates/bitwarden-crypto/src/keys/utils.rs b/crates/bitwarden-crypto/src/keys/utils.rs
index 8564a7c9f..4b2c4c8cd 100644
--- a/crates/bitwarden-crypto/src/keys/utils.rs
+++ b/crates/bitwarden-crypto/src/keys/utils.rs
@@ -3,30 +3,45 @@ use std::pin::Pin;
 use generic_array::{typenum::U32, GenericArray};
 use sha2::Digest;
 
-use crate::{util::hkdf_expand, Kdf, Result, SymmetricCryptoKey};
+use crate::{util::hkdf_expand, CryptoError, Kdf, Result, SymmetricCryptoKey};
+
+const PBKDF2_MIN_ITERATIONS: u32 = 5000;
+
+const ARGON2ID_MIN_MEMORY: u32 = 16 * 1024;
+const ARGON2ID_MIN_ITERATIONS: u32 = 2;
+const ARGON2ID_MIN_PARALLELISM: u32 = 1;
 
 /// Derive a generic key from a secret and salt using the provided KDF.
 pub(super) fn derive_kdf_key(secret: &[u8], salt: &[u8], kdf: &Kdf) -> Result<SymmetricCryptoKey> {
     let mut hash = match kdf {
-        Kdf::PBKDF2 { iterations } => crate::util::pbkdf2(secret, salt, iterations.get()),
+        Kdf::PBKDF2 { iterations } => {
+            let iterations = iterations.get();
+            if iterations < PBKDF2_MIN_ITERATIONS {
+                return Err(CryptoError::InsufficientKdfParameters);
+            }
 
+            crate::util::pbkdf2(secret, salt, iterations)
+        }
         Kdf::Argon2id {
             iterations,
             memory,
             parallelism,
         } => {
+            let memory = memory.get() * 1024; // Convert MiB to KiB;
+            let iterations = iterations.get();
+            let parallelism = parallelism.get();
+
+            if memory < ARGON2ID_MIN_MEMORY
+                || iterations < ARGON2ID_MIN_ITERATIONS
+                || parallelism < ARGON2ID_MIN_PARALLELISM
+            {
+                return Err(CryptoError::InsufficientKdfParameters);
+            }
+
             use argon2::*;
 
-            let argon = Argon2::new(
-                Algorithm::Argon2id,
-                Version::V0x13,
-                Params::new(
-                    memory.get() * 1024, // Convert MiB to KiB
-                    iterations.get(),
-                    parallelism.get(),
-                    Some(32),
-                )?,
-            );
+            let params = Params::new(memory, iterations, parallelism, Some(32))?;
+            let argon = Argon2::new(Algorithm::Argon2id, Version::V0x13, params);
 
             let salt_sha = sha2::Sha256::new().chain_update(salt).finalize();
 
@@ -57,6 +72,8 @@ pub(super) fn stretch_kdf_key(k: &SymmetricCryptoKey) -> Result<SymmetricCryptoK
 
 #[cfg(test)]
 mod tests {
+    use std::num::NonZero;
+
     use super::*;
 
     #[test]
@@ -89,4 +106,42 @@ mod tests {
             stretched.mac_key.as_ref().unwrap().as_slice()
         );
     }
+
+    #[test]
+    fn test_derive_kdf_minimums() {
+        fn nz(n: u32) -> NonZero<u32> {
+            NonZero::new(n).unwrap()
+        }
+
+        let secret = [0u8; 32];
+        let salt = [0u8; 32];
+
+        for kdf in [
+            Kdf::PBKDF2 {
+                iterations: nz(4999),
+            },
+            Kdf::Argon2id {
+                iterations: nz(1),
+                memory: nz(16),
+                parallelism: nz(1),
+            },
+            Kdf::Argon2id {
+                iterations: nz(2),
+                memory: nz(15),
+                parallelism: nz(1),
+            },
+            Kdf::Argon2id {
+                iterations: nz(1),
+                memory: nz(15),
+                parallelism: nz(1),
+            },
+        ] {
+            assert_eq!(
+                derive_kdf_key(&secret, &salt, &kdf)
+                    .unwrap_err()
+                    .to_string(),
+                "Insufficient KDF parameters"
+            );
+        }
+    }
 }