Skip to content

Commit

Permalink
Validate minimum KDF settings (#1100)
Browse files Browse the repository at this point in the history
## 🎟️ 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
  • Loading branch information
dani-garcia committed Sep 30, 2024
1 parent a6fd484 commit c33f82a
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 12 deletions.
3 changes: 3 additions & 0 deletions crates/bitwarden-crypto/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down
79 changes: 67 additions & 12 deletions crates/bitwarden-crypto/src/keys/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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"
);
}
}
}

0 comments on commit c33f82a

Please sign in to comment.