diff --git a/.github/workflows/bindings.yaml b/.github/workflows/bindings.yaml index bdb2e4fb..c6e915f0 100644 --- a/.github/workflows/bindings.yaml +++ b/.github/workflows/bindings.yaml @@ -94,25 +94,6 @@ jobs: name: mfkdf2-web-mochawesome-report path: mfkdf2-web/test-results/mochawesome/ - - - name: Generate TypeScript bindings for differential tests - working-directory: mfkdf2-web - run: npm run ubrn:web:differential:release - - - name: Copy index.web.ts implementation - run: cp mfkdf2-web/src/index.ts mfkdf2-web/src/index.web.ts - - - name: Run differential tests with reports - working-directory: mfkdf2-web - run: npm run test:differential:report - - - name: Upload differential HTML test report - if: always() - uses: actions/upload-artifact@v4 - with: - name: mfkdf2-web-differential-report - path: mfkdf2-web/test-results/mochawesome-differential/ - - name: Run TypeScript type checking working-directory: mfkdf2-web run: npm run typecheck diff --git a/.github/workflows/differential.yaml b/.github/workflows/differential.yaml new file mode 100644 index 00000000..5d284e13 --- /dev/null +++ b/.github/workflows/differential.yaml @@ -0,0 +1,82 @@ +name: Differential Tests +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +on: + workflow_dispatch: + +env: + CARGO_TERM_COLOR: always + +jobs: + differential-tests: + name: differential + runs-on: ubuntu-latest + steps: + - name: Checkout pinned mfkdf2.rs commit + uses: actions/checkout@v4 + with: + ref: 7c33c7164d6e40a26c0899f19b8f9ad9b9f0c029 + + - name: Install Rust + uses: dtolnay/rust-toolchain@master + with: + toolchain: stable + targets: wasm32-unknown-unknown + + - name: Rust Cache + uses: Swatinem/rust-cache@v2 + with: + key: typescript/differential + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: "20" + cache: "npm" + cache-dependency-path: mfkdf2-web/package-lock.json + + - name: Install wasm-bindgen-cli + uses: taiki-e/install-action@v2 + with: + tool: wasm-bindgen-cli + + - name: Cache node_modules + id: cache-node-modules + uses: actions/cache@v4 + with: + path: mfkdf2-web/node_modules + key: ${{ runner.os }}-node-modules-${{ hashFiles('mfkdf2-web/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-node-modules- + + - name: Install mfkdf2-web dependencies + if: steps.cache-node-modules.outputs.cache-hit != 'true' + working-directory: mfkdf2-web + run: npm ci + + - name: Generate TypeScript bindings for differential tests + working-directory: mfkdf2-web + run: npm run ubrn:web:differential:release + + - name: Copy index.web.ts implementation + run: cp mfkdf2-web/src/index.ts mfkdf2-web/src/index.web.ts + + - name: Verify bindings were generated + run: | + if [ ! -d "mfkdf2-web/src/generated" ] || [ -z "$(ls -A mfkdf2-web/src/generated)" ]; then + echo "Error: mfkdf2-web/src/generated does not exist or is empty" + exit 1 + fi + if [ ! -d "mfkdf2-web/rust_modules" ]; then + echo "Error: mfkdf2-web/rust_modules does not exist" + exit 1 + fi + echo "âś“ TypeScript bindings verified" + + - name: Run differential tests + working-directory: mfkdf2-web + run: npm run test:differential + + diff --git a/.github/workflows/rust.yaml b/.github/workflows/rust.yaml index 43f2a0c6..67a48ee3 100644 --- a/.github/workflows/rust.yaml +++ b/.github/workflows/rust.yaml @@ -119,6 +119,9 @@ jobs: - name: Run tests (JUnit) run: cargo nextest run --release --profile ci + + - name: Run doctests + run: cargo test --release --doc - name: Publish JUnit test report if: always() @@ -184,7 +187,7 @@ jobs: uses: taiki-e/install-action@cargo-llvm-cov - name: Run cargo-llvm-cov - run: cargo llvm-cov --all-features --workspace --html --output-dir target/coverage + run: cargo llvm-cov --workspace --html --output-dir target/coverage - name: Upload coverage to Artifacts uses: actions/upload-artifact@v4 diff --git a/Cargo.lock b/Cargo.lock index bd5c82fb..c5d61042 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -240,6 +240,15 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37b2a672a2cb129a2e41c10b1224bb368f9f37a2b16b612598138befd7b37eb5" +[[package]] +name = "cbc" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26b52a9543ae338f279b96b0b9fed9c8093744685043739079ce85cd58f289a6" +dependencies = [ + "cipher", +] + [[package]] name = "cc" version = "1.2.41" @@ -459,6 +468,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1bfb12502f3fc46cca1bb51ac28df9d618d813cdc3d2f25b9fe775a34af26bb3" dependencies = [ "generic-array", + "rand_core", "typenum", ] @@ -964,6 +974,7 @@ dependencies = [ "aes", "argon2", "base64", + "cbc", "cipher", "console_log", "criterion", diff --git a/SECURITY.md b/SECURITY.md index 2cb12f3c..a25eda6d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,6 +12,7 @@ Please check here before filing new reports. | Affected Version(s) | Description | Status | CVE / Advisory | | ------------------- | ---------------------------------------------------------------------- | ------------ | -------------- | | 0.0.1 | RSA Marvin Attack: potential key recovery through timing sidechannels. | đź”´ Unresolved | 2023-49092 | +| 0.0.1 | Generic-Array: v0.14.9 is deprecated but used by aes-v0.8.4 | đź”´ Unresolved | - | Legend: - 🟢 Fixed diff --git a/docs/src/differential-tests.md b/docs/src/differential-tests.md index 05a57d15..034027b5 100644 --- a/docs/src/differential-tests.md +++ b/docs/src/differential-tests.md @@ -31,19 +31,26 @@ See: [multifactor/MFKDF#27](https://github.com/multifactor/MFKDF/pull/27) See: [multifactor/MFKDF2.rs#43](https://github.com/multifactor/MFKDF2.rs/pull/43) - Add a `differential-test` feature flag providing a global deterministic RNG equivalent to the reference. - Provide utility methods in the TypeScript bindings facade for nested parameter parsing and stringification (read/write inner params) to match reference structures. +- Pin the versions used for differential testing to: + - `MFKDF2.rs` commit `7c33c7164d6e40a26c0899f19b8f9ad9b9f0c029` + - `MFKDF` commit `3d5bf73b4ce42b23da113b4be6d35e7d941fadf8` ## How to reproduce -Run the differential tests using the bindings workflow. From the repository root: +You can run the differential tests **locally** or via the **GitHub Actions workflow**. + +### Locally + +From the repository root: ```bash # Ensure the WASM target is present (one-time) rustup target add wasm32-unknown-unknown -# Generate differential-release bindings (optimized) +# Generate differential-release bindings (includes the `differential-test` feature) just gen-ts-bindings-differential -# Run the TypeScript test suite (includes differential tests) +# Run only the differential TypeScript test suite just test-bindings-differential ``` diff --git a/mfkdf2-web/package-lock.json b/mfkdf2-web/package-lock.json index 18635b1d..04e1aa62 100644 --- a/mfkdf2-web/package-lock.json +++ b/mfkdf2-web/package-lock.json @@ -19,7 +19,7 @@ "ajv": "^8.17.1", "chai": "^4.3.0", "chai-as-promised": "^7.1.1", - "mfkdf": "github:multifactor/MFKDF#test/differential-testing", + "mfkdf": "github:multifactor/MFKDF#3d5bf7", "mocha": "^10.0.0", "mocha-junit-reporter": "^2.2.1", "mocha-multi-reporters": "^1.5.1", diff --git a/mfkdf2-web/package.json b/mfkdf2-web/package.json index b72a3e4e..b797b2f3 100644 --- a/mfkdf2-web/package.json +++ b/mfkdf2-web/package.json @@ -61,6 +61,6 @@ "tsconfig-paths": "^4.2.0", "tsx": "^4.19.0", "typescript": "^5.6.2", - "mfkdf": "github:multifactor/MFKDF#test/differential-testing" + "mfkdf": "github:multifactor/MFKDF#3d5bf7" } } \ No newline at end of file diff --git a/mfkdf2-web/test/factors/hmacsha1.test.ts b/mfkdf2-web/test/factors/hmacsha1.test.ts index d36e2e45..6e926684 100644 --- a/mfkdf2-web/test/factors/hmacsha1.test.ts +++ b/mfkdf2-web/test/factors/hmacsha1.test.ts @@ -73,7 +73,7 @@ suite('factors/hmacsha1', () => { derive.key .toString('hex') .should.equal( - '2747ebf65219aee6630a758e40fd05ccbb39ab465745ea1c9a6c5adb6673d2d3' + 'e1e67a0a2118867d8baf660d87500e650211855d2eff4c557ef2c8ae26ab5b6f' ); }); diff --git a/mfkdf2/Cargo.toml b/mfkdf2/Cargo.toml index 55195fdf..61324f07 100644 --- a/mfkdf2/Cargo.toml +++ b/mfkdf2/Cargo.toml @@ -14,8 +14,11 @@ required-features = ["bindings"] [dependencies] # Cryptography aes = { version = "0.8", default-features = false } +cbc = { version = "0.1.2", default-features = false } cipher = { version = "0.4", default-features = false, features = [ "block-padding", + "rand_core", + "std", ] } ecb = { version = "0.1", default-features = false } hkdf = { version = "0.12", default-features = false } @@ -84,6 +87,7 @@ data-encoding = { version = "2.9.0", default-features = false, features = [ ] } regex = { version = "1.11.3", default-features = false } + [target.'cfg(target_arch = "wasm32")'.dependencies] console_log = { version = "1.0", default-features = false } getrandom = { version = "0.2", default-features = false, features = ["js"] } diff --git a/mfkdf2/README.md b/mfkdf2/README.md index 8d38aa45..6da2e374 100644 --- a/mfkdf2/README.md +++ b/mfkdf2/README.md @@ -276,6 +276,8 @@ a Shamir‑style secret sharing scheme, one share per factor. During derive, any that supplies at least `threshold` valid shares can reconstruct the same secret and therefore the same derived key. +**Note**: MFKDF2 provides no mechanism to invalidate old policies. When threshold is increased via [reconstitution](`crate::definitions::mfkdf_derived_key::reconstitution`), old policies can still be used to derive keys. + ## Setup: configuring a 2‑of‑3 recovery policy The snippet below constructs a 2‑of‑3 key from a password, an HOTP soft token, and a UUID @@ -463,6 +465,13 @@ let derived = derive::key( The same outer key can also be derived with only `password3` by supplying a single password factor keyed by `"password3"` to [setup key](`crate::derive::key`). +# Integrity Protetion + + +MFKDF2 allows policy integrity to be enforced between each subsequent derives, and is enabled by default. An honest client will only accept a state if the key it derives from that state correctly validates the state’s integrity. Before deriving the final key, current policy's self-referential tag is checked. This is enabled using `verify` flag in [setup](`crate::setup::key`) and [derive](`crate::derive::key`). If any mismatch is detected, the [PolicyIntegrityCheckFailed](`crate::error::MFKDF2Error::PolicyIntegrityCheckFailed`) error is returned. + +When integrity is disabled, adversary can modify factor public state like threshold, factor parameters, encrypted shares. This may expose underlying keys and factor secrets, reducing the overall entropy of the key. + # Feature Flags - `bindings`: Generate FFI bindings of the library to other languages. diff --git a/mfkdf2/src/crypto.rs b/mfkdf2/src/crypto.rs index df31bfd2..c9cbd759 100644 --- a/mfkdf2/src/crypto.rs +++ b/mfkdf2/src/crypto.rs @@ -1,12 +1,16 @@ //! Cryptographic functions for the MFKDF2 library. use aes::Aes256; -use cipher::{BlockDecryptMut, BlockEncryptMut, KeyInit, block_padding::NoPadding}; +use cipher::{ + BlockDecryptMut, BlockEncryptMut, Iv, Key, KeyInit, KeyIvInit, block_padding::NoPadding, +}; use ecb::{Decryptor, Encryptor}; use hkdf::Hkdf; use hmac::{Hmac, Mac}; use sha1::Sha1; use sha2::Sha256; +use crate::error::MFKDF2Error; + /// Derives a 32-byte key using HKDF-SHA256 with the given salt and info. pub(crate) fn hkdf_sha256_with_info(input: &[u8], salt: &[u8], info: &[u8]) -> [u8; 32] { let hk = Hkdf::::new(Some(salt), input); @@ -33,6 +37,30 @@ pub(crate) fn encrypt(data: &[u8], key: &[u8; 32]) -> Vec { buf } +pub(crate) fn encrypt_cbc( + data: &[u8], + key: &Key>, + iv: &Iv>, +) -> Result, MFKDF2Error> +where + C: cipher::BlockCipher + cipher::BlockEncrypt, + cbc::Encryptor: KeyIvInit + cipher::IvSizeUser + BlockEncryptMut, +{ + let mut buf = { + let mut v = data.to_vec(); + let rem = v.len() % 16; + if rem != 0 { + v.extend(vec![0u8; 16 - rem]); + } + v + }; + let padded_len = buf.len(); + + let ct = + cbc::Encryptor::::new(key, iv).encrypt_padded_mut::(&mut buf, padded_len)?; + Ok(ct.to_vec()) +} + /// Decrypts a buffer using AES256-ECB with the given 32-byte key. // TODO (@lonerapier): check every use of decrypt and unpad properly or use assert. pub(crate) fn decrypt(mut data: Vec, key: &[u8; 32]) -> Vec { @@ -41,6 +69,20 @@ pub(crate) fn decrypt(mut data: Vec, key: &[u8; 32]) -> Vec { data } +pub(crate) fn decrypt_cbc( + data: &[u8], + key: &Key>, + iv: &Iv>, +) -> Result, MFKDF2Error> +where + C: cipher::BlockCipher + cipher::BlockDecrypt, + cbc::Decryptor: KeyIvInit + cipher::IvSizeUser + BlockDecryptMut, +{ + let mut buf = data.to_vec(); + let ct = cbc::Decryptor::::new(key, iv).decrypt_padded_mut::(&mut buf)?; + Ok(ct.to_vec()) +} + /// Computes an HMAC-SHA1 over the given challenge using the provided secret. pub(crate) fn hmacsha1(secret: &[u8], challenge: &[u8]) -> [u8; 20] { let mut mac = as Mac>::new_from_slice(secret).unwrap(); diff --git a/mfkdf2/src/definitions/factor.rs b/mfkdf2/src/definitions/factor.rs index f57488b6..81351524 100644 --- a/mfkdf2/src/definitions/factor.rs +++ b/mfkdf2/src/definitions/factor.rs @@ -35,7 +35,7 @@ pub(crate) trait FactorMetadata: Send + Sync + std::fmt::Debug { /// /// ```rust /// use mfkdf2::{ -/// definitions::{FactorMetadata, FactorType}, +/// definitions::FactorType, /// derive::factors::password as derive_password, /// setup::factors::password::{PasswordOptions, password}, /// }; @@ -48,7 +48,6 @@ pub(crate) trait FactorMetadata: Send + Sync + std::fmt::Debug { /// _ => panic!("Wrong factor type"), /// }; /// assert_eq!(p.password, "password123"); -/// assert_eq!(p.bytes(), "password123".as_bytes()); /// /// // derive a key using the password factor /// let derive = derive_password("password123")?; diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/crypto.rs b/mfkdf2/src/definitions/mfkdf_derived_key/crypto.rs index 901cc7d3..27e1fa56 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/crypto.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/crypto.rs @@ -1,9 +1,15 @@ +use crate::error::MFKDF2Result; + impl crate::definitions::MFKDF2DerivedKey { /// Returns an HKDF-SHA256 derived key for the given purpose and salt. - pub fn get_subkey(&self, purpose: Option<&str>, salt: Option<&[u8]>) -> [u8; 32] { + pub fn get_subkey(&self, purpose: Option<&str>, salt: Option<&[u8]>) -> MFKDF2Result<[u8; 32]> { let salt = salt.unwrap_or(&[]); let purpose = purpose.unwrap_or(""); - crate::crypto::hkdf_sha256_with_info(&self.key, salt, purpose.as_bytes()) + + // derive internal key + let internal_key = self.derive_internal_key()?; + // derive subkey + Ok(crate::crypto::hkdf_sha256_with_info(&internal_key, salt, purpose.as_bytes())) } } @@ -13,8 +19,8 @@ fn derived_key_get_subkey( derived_key: &crate::definitions::MFKDF2DerivedKey, purpose: Option, salt: Option>, -) -> Vec { +) -> MFKDF2Result> { let purpose = purpose.as_deref(); let salt = salt.as_deref(); - derived_key.get_subkey(purpose, salt).to_vec() + Ok(derived_key.get_subkey(purpose, salt)?.to_vec()) } diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/hints.rs b/mfkdf2/src/definitions/mfkdf_derived_key/hints.rs index 0c113eaa..3eeddd6b 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/hints.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/hints.rs @@ -10,9 +10,13 @@ //! correct before attempting a full derivation. use std::fmt::Write; -use base64::engine::general_purpose; +use base64::{Engine, engine::general_purpose}; -use crate::{definitions::MFKDF2DerivedKey, error::MFKDF2Error}; +use crate::{ + crypto::{hkdf_sha256_with_info, hmacsha256}, + definitions::MFKDF2DerivedKey, + error::MFKDF2Error, +}; impl MFKDF2DerivedKey { /// Compute a probabilistic hint for a single factor. @@ -37,6 +41,9 @@ impl MFKDF2DerivedKey { return Err(MFKDF2Error::InvalidHintLength("bits must be greater than 0")); } + // derive internal key + let internal_key = self.derive_internal_key()?; + let factor_data = self .policy .factors @@ -46,7 +53,7 @@ impl MFKDF2DerivedKey { let pad = base64::Engine::decode(&general_purpose::STANDARD, factor_data.secret.as_bytes())?; let salt = base64::Engine::decode(&general_purpose::STANDARD, factor_data.salt.as_bytes())?; let secret_key = crate::crypto::hkdf_sha256_with_info( - &self.key, + &internal_key, &salt, format!("mfkdf2:factor:secret:{factor_id}").as_bytes(), ); @@ -135,10 +142,39 @@ impl MFKDF2DerivedKey { /// # Ok::<(), mfkdf2::error::MFKDF2Error>(()) /// ``` pub fn add_hint(&mut self, factor_id: &str, bits: Option) -> Result<(), MFKDF2Error> { + // derive internal key + let internal_key = self.derive_internal_key()?; + + // verify policy integrity + if !self.policy.hmac.is_empty() { + let integrity_data = self.policy.extract(); + let salt = general_purpose::STANDARD.decode(&self.policy.salt)?; + let integrity_key = + hkdf_sha256_with_info(&internal_key, &salt, "mfkdf2:integrity".as_bytes()); + let digest = hmacsha256(&integrity_key, &integrity_data); + let hmac = general_purpose::STANDARD.encode(digest); + if self.policy.hmac != hmac { + return Err(MFKDF2Error::PolicyIntegrityCheckFailed); + } + } + let bits = bits.unwrap_or(7); let hint = self.get_hint(factor_id, bits); let factor_data = self.policy.factors.iter_mut().find(|f| f.id == factor_id).unwrap(); factor_data.hint = Some(hint?); + + // update the hmac of the policy + if !self.policy.hmac.is_empty() { + // compute the new hmac of the policy + let integrity_data = self.policy.extract(); + let salt = general_purpose::STANDARD.decode(&self.policy.salt)?; + let integrity_key = + hkdf_sha256_with_info(&internal_key, &salt, "mfkdf2:integrity".as_bytes()); + let digest = hmacsha256(&integrity_key, &integrity_data); + let hmac = general_purpose::STANDARD.encode(digest); + self.policy.hmac = hmac; + } + Ok(()) } } @@ -269,7 +305,7 @@ mod tests { &[crate::setup::factors::password("password1", PasswordOptions { id: Some("password1".to_string()), })?], - MFKDF2Options { integrity: Some(false), ..Default::default() }, + MFKDF2Options { integrity: Some(true), ..Default::default() }, )?; let result = setup_key.get_hint("password1", 0); @@ -277,4 +313,123 @@ mod tests { Ok(()) } + + // Below tests demonstrate the entropy leakage of the hint feature. With integrity turned off, an + // adversary can modify the hint and derive the key successfully. If hint mismatches, then + // derivation fails with `HintMismatch` error. This leaks 1 bit of information for each + // derivation. The adversary can repeatedly guess the hint, launching a brute-force attack on the + // factor. + // If integrity is turned on, then the adversary cannot modify the hint and derive the key + // successfully. If hint mismatches, then derivation fails with `PolicyIntegrityCheckFailed` + // error. This does not leak any information about the factor. + #[test] + fn hint_entropy_leakage_no_integrity() -> Result<(), error::MFKDF2Error> { + let mut setup_key = setup::key( + &[crate::setup::factors::password("password1", PasswordOptions { + id: Some("password1".to_string()), + })?], + MFKDF2Options { integrity: Some(false), ..Default::default() }, + )?; + + // 7 bit hint added to the policy + setup_key.add_hint("password1", None)?; + + // derive the key with the correct password + let derive_key = derive::key( + &setup_key.policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1")?)]), + false, + false, + )?; + + assert_eq!(derive_key.key, setup_key.key); + + // modify hint + let mut hint = setup_key.get_hint("password1", 7)?; + hint.insert(0, '0'); + + let mut modified_setup_key_policy = setup_key.policy.clone(); + modified_setup_key_policy.factors[0].hint = Some(hint); + + // derive the key with the modified hint + let mut modified_derive_key = derive::key( + &modified_setup_key_policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1")?)]), + false, + false, + ); + + if matches!(modified_derive_key, Err(error::MFKDF2Error::HintMismatch(_))) { + let mut hint = setup_key.get_hint("password1", 7)?; + hint.insert(0, '1'); + modified_setup_key_policy.factors[0].hint = Some(hint); + modified_derive_key = derive::key( + &modified_setup_key_policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1")?)]), + false, + false, + ); + } + + assert_eq!(modified_derive_key.unwrap().key, derive_key.key); + + Ok(()) + } + + #[test] + fn hint_entropy_leakage_with_integrity() { + let mut setup_key = setup::key( + &[crate::setup::factors::password("password1", PasswordOptions { + id: Some("password1".to_string()), + }) + .unwrap()], + MFKDF2Options { integrity: Some(true), ..Default::default() }, + ) + .unwrap(); + + // 7 bit hint added to the policy + setup_key.add_hint("password1", None).unwrap(); + + // derive the key with the correct password + let derive_key = derive::key( + &setup_key.policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1").unwrap())]), + true, + false, + ) + .unwrap(); + + assert_eq!(derive_key.key, setup_key.key); + + // modify hint + let mut hint = setup_key.get_hint("password1", 7).unwrap(); + hint.insert(0, '0'); + + let mut modified_setup_key_policy = setup_key.policy.clone(); + modified_setup_key_policy.factors[0].hint = Some(hint); + + // derive the key with the modified hint + let modified_derive_key1 = derive::key( + &modified_setup_key_policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1").unwrap())]), + true, + false, + ); + + let mut modified_hint = setup_key.get_hint("password1", 7).unwrap(); + modified_hint.insert(0, '1'); + modified_setup_key_policy.factors[0].hint = Some(modified_hint); + + let modified_derive_key2 = derive::key( + &modified_setup_key_policy, + HashMap::from([("password1".to_string(), derive_factors::password("password1").unwrap())]), + true, + false, + ); + + assert!( + matches!(modified_derive_key1, Err(error::MFKDF2Error::PolicyIntegrityCheckFailed)) + || matches!(modified_derive_key2, Err(error::MFKDF2Error::PolicyIntegrityCheckFailed)) + ); + } } diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/mfdpg.rs b/mfkdf2/src/definitions/mfkdf_derived_key/mfdpg.rs index 7edb09ec..cf5d3b20 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/mfdpg.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/mfdpg.rs @@ -11,7 +11,7 @@ use rand::{SeedableRng, distributions::Distribution}; use rand_regex::Regex; -use crate::error::MFKDF2Error; +use crate::error::MFKDF2Result; impl crate::definitions::MFKDF2DerivedKey { /// Derives a deterministic, policy-compliant password from an `MFKDF2DerivedKey` @@ -64,8 +64,8 @@ impl crate::definitions::MFKDF2DerivedKey { purpose: Option<&str>, salt: Option<&[u8]>, regex: &str, - ) -> Result { - let password_key = self.get_subkey(purpose, salt); + ) -> MFKDF2Result { + let password_key = self.get_subkey(purpose, salt)?; // seed and rng with password_key let mut rng = rand_chacha::ChaCha20Rng::from_seed(password_key); let dfa = Regex::compile(regex, 10000)?; @@ -80,7 +80,7 @@ fn derived_key_derive_password( purpose: Option, salt: Option>, regex: &str, -) -> Result { +) -> MFKDF2Result { let purpose = purpose.as_deref(); let salt = salt.as_deref(); derived_key.derive_password(purpose, salt, regex) diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/mod.rs b/mfkdf2/src/definitions/mfkdf_derived_key/mod.rs index 8827d5ac..698995ad 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/mod.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/mod.rs @@ -11,12 +11,15 @@ //! underlying multi-factor construction. use std::collections::HashMap; +use argon2::Argon2; use base64::{Engine, engine::general_purpose}; use serde::{Deserialize, Serialize}; use serde_json::Value; use crate::{ + crypto::decrypt, definitions::{bytearray::Key, entropy::MFKDF2Entropy}, + error::MFKDF2Result, policy::Policy, }; @@ -64,3 +67,28 @@ impl std::fmt::Display for MFKDF2DerivedKey { ) } } + +impl MFKDF2DerivedKey { + /// Derive an internal key for deriving separate keys for parameters, secret, and integrity + /// using the policy salt and secret. + fn derive_internal_key(&self) -> MFKDF2Result { + let salt = general_purpose::STANDARD.decode(&self.policy.salt)?; + + let mut kek = [0u8; 32]; + Argon2::new( + argon2::Algorithm::Argon2id, + argon2::Version::default(), + argon2::Params::new( + argon2::Params::DEFAULT_M_COST + self.policy.memory, + argon2::Params::DEFAULT_T_COST + self.policy.time, + 1, + Some(32), + )?, + ) + .hash_password_into(&self.secret, &salt, &mut kek)?; + + let policy_key = general_purpose::STANDARD.decode(&self.policy.key)?; + let key = decrypt(policy_key, &kek); + key.try_into() + } +} diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/reconstitution.rs b/mfkdf2/src/definitions/mfkdf_derived_key/reconstitution.rs index a5c4eb4d..b1156af3 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/reconstitution.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/reconstitution.rs @@ -6,6 +6,9 @@ //! Consider a key derived from a password, a TOTP factor, and a UUID factor. Using threshold //! recovery, the user can derive the key with only a subset of factors inside the policy. //! +//! **Note**: MFKDF2 provides no mechanism to invalidate old policies. When threshold is increased +//! via reconstitution, old policies can still be used to derive keys. +//! //! ```rust //! # use std::collections::HashMap; //! # use uuid::Uuid; @@ -159,13 +162,16 @@ impl MFKDF2DerivedKey { let threshold = threshold.unwrap_or(self.policy.threshold); + // derive internal key for deriving separate keys for parameters, secret, and integrity + let internal_key = self.derive_internal_key()?; + for factor in &self.policy.factors { factors.insert(factor.id.clone(), factor.clone()); let pad = general_purpose::STANDARD.decode(&factor.secret)?; let salt = general_purpose::STANDARD.decode(&factor.salt)?; let secret_key = hkdf_sha256_with_info( - &self.key, + &internal_key, &salt, format!("mfkdf2:factor:secret:{}", factor.id).as_bytes(), ); @@ -192,8 +198,11 @@ impl MFKDF2DerivedKey { let id = factor.id.clone().ok_or(MFKDF2Error::MissingFactorId)?; - let params_key = - hkdf_sha256_with_info(&self.key, &salt, format!("mfkdf2:factor:params:{id}").as_bytes()); + let params_key = hkdf_sha256_with_info( + &internal_key, + &salt, + format!("mfkdf2:factor:params:{id}").as_bytes(), + ); let params = factor.factor_type.setup().params(params_key.into())?; let new_factor = PolicyFactor { @@ -253,7 +262,7 @@ impl MFKDF2DerivedKey { }; let secret_key = hkdf_sha256_with_info( - &self.key, + &internal_key, &salt, format!("mfkdf2:factor:secret:{}", factor.id).as_bytes(), ); @@ -271,7 +280,8 @@ impl MFKDF2DerivedKey { if !self.policy.hmac.is_empty() { let salt = general_purpose::STANDARD.decode(&self.policy.salt)?; - let integrity_key = hkdf_sha256_with_info(&self.key, &salt, "mfkdf2:integrity".as_bytes()); + let integrity_key = + hkdf_sha256_with_info(&internal_key, &salt, "mfkdf2:integrity".as_bytes()); let integrity_data = self.policy.extract(); let digest = hmacsha256(&integrity_key, &integrity_data); self.policy.hmac = general_purpose::STANDARD.encode(digest); diff --git a/mfkdf2/src/definitions/mfkdf_derived_key/strengthening.rs b/mfkdf2/src/definitions/mfkdf_derived_key/strengthening.rs index b526255a..b28450d4 100644 --- a/mfkdf2/src/definitions/mfkdf_derived_key/strengthening.rs +++ b/mfkdf2/src/definitions/mfkdf_derived_key/strengthening.rs @@ -93,6 +93,10 @@ impl MFKDF2DerivedKey { /// [`crate::error::MFKDF2Error::PolicyIntegrityCheckFailed`] if an attacker attempts to /// weaken or roll back the policy, as that invalidates the integrity MAC. pub fn strengthen(&mut self, time: u32, memory: u32) -> MFKDF2Result<()> { + // derive internal key + let internal_key = self.derive_internal_key()?; + + // update policy time and memory self.policy.time = time; self.policy.memory = memory; @@ -112,7 +116,7 @@ impl MFKDF2DerivedKey { ) .hash_password_into(&self.secret, &salt, &mut kek)?; - let policy_key = encrypt(self.key.as_ref(), &kek); + let policy_key = encrypt(&internal_key, &kek); self.policy.key = general_purpose::STANDARD.encode(policy_key); Ok(()) } diff --git a/mfkdf2/src/derive/factors/ooba.rs b/mfkdf2/src/derive/factors/ooba.rs index 285b6337..69865ea7 100644 --- a/mfkdf2/src/derive/factors/ooba.rs +++ b/mfkdf2/src/derive/factors/ooba.rs @@ -4,16 +4,20 @@ //! RSA key, and embeds an initial code and metadata in the policy. //! - During derive, this module consumes a user‑entered OOBA code Wᵢⱼ, decrypts the target using //! the stored pad, and prepares the next encrypted payload and code for the following login +use aes::Aes256; use base64::{Engine, engine::general_purpose}; +use cbc::Encryptor; +use cipher::KeyIvInit; use rsa::Oaep; use serde_json::{Value, json}; use sha2::Sha256; use crate::{ - crypto::{decrypt, encrypt, hkdf_sha256_with_info}, + crypto::{decrypt_cbc, encrypt_cbc, hkdf_sha256_with_info}, definitions::{FactorType, Key, MFKDF2Factor}, derive::FactorDerive, error::{MFKDF2Error, MFKDF2Result}, + rng::GlobalRng, setup::factors::ooba::{Ooba, OobaPublicKey, generate_alphanumeric_characters}, }; @@ -36,7 +40,10 @@ impl FactorDerive for Ooba { } let key = hkdf_sha256_with_info(self.code.as_bytes(), &[], &[]); - self.target = decrypt(pad, &key); + let iv = general_purpose::STANDARD.decode(config["iv"].as_str().unwrap()).unwrap(); + let key = cipher::Key::>::from(key); + let iv = cipher::Iv::>::from_iter(iv); + self.target = decrypt_cbc::(&pad, &key, &iv)?; self.length = params["length"] .as_u64() @@ -55,10 +62,17 @@ impl FactorDerive for Ooba { let code = generate_alphanumeric_characters(self.length.into()).to_uppercase(); let next_key = hkdf_sha256_with_info(code.as_bytes(), &[], &[]); - let pad = encrypt(&self.target, &next_key); + let key = cipher::Key::>::from(next_key); + let iv = Encryptor::::generate_iv(&mut GlobalRng); + let pad = encrypt_cbc::(&self.target, &key, &iv)?; let mut params = self.params.clone(); params["code"] = Value::String(code); + params["iv"] = json!(general_purpose::STANDARD.encode(iv)); + + // store the iv in public params for the next derivation + let mut pub_params = self.params.clone(); + pub_params["iv"] = json!(general_purpose::STANDARD.encode(iv)); let plaintext = serde_json::to_vec(¶ms)?; let public_key = @@ -69,7 +83,7 @@ impl FactorDerive for Ooba { Ok(json!({ "length": self.length, "key": self.jwk, - "params": self.params, + "params": pub_params, "next": hex::encode(ciphertext), "pad": general_purpose::STANDARD.encode(pad), })) @@ -248,10 +262,13 @@ mod tests { ) .unwrap(); let code = decrypted["code"].as_str().unwrap(); + let iv = general_purpose::STANDARD.decode(decrypted["iv"].as_str().unwrap()).unwrap(); let prev_key = hkdf_sha256_with_info(code.as_bytes(), &[], &[]); let pad = general_purpose::STANDARD.decode(derive_params["pad"].as_str().unwrap()).unwrap(); - let target = decrypt(pad, &prev_key); + let key = cipher::Key::>::from(prev_key); + let iv = cipher::Iv::>::from_iter(iv); + let target = crate::crypto::decrypt_cbc::(&pad, &key, &iv).unwrap(); assert_eq!(ooba.target, target); } @@ -343,7 +360,7 @@ mod tests { // 7. Get `next` and `params` let next_hex = derive_params["next"].as_str().unwrap(); - let params_from_derive = derive_params["params"].clone(); + // let params_from_derive = derive_params["params"].clone(); let ciphertext = hex::decode(next_hex).unwrap(); // 8. Decrypt @@ -354,8 +371,9 @@ mod tests { let mut decrypted_params = serde_json::from_slice::(&decrypted_bytes).unwrap(); decrypted_params.as_object_mut().unwrap().remove("code"); + // TODO (@lonerapier): this won't be same after CBC encryption since IV is different // 10. Assert - assert_eq!(decrypted_params, params_from_derive); + // assert_eq!(decrypted_params, params_from_derive); } fn get_ooba_for_test() -> Ooba { diff --git a/mfkdf2/src/derive/key.rs b/mfkdf2/src/derive/key.rs index b97c8778..9f32d85f 100644 --- a/mfkdf2/src/derive/key.rs +++ b/mfkdf2/src/derive/key.rs @@ -304,33 +304,6 @@ pub fn key( let pad = general_purpose::STANDARD.decode(&factor.pad)?; let plaintext = decrypt(pad, &stretched); - if let Some(ref factor_hint) = factor.hint { - let buffer = hkdf_sha256_with_info( - &stretched, - &salt_bytes, - format!("mfkdf2:factor:hint:{}", factor.id).as_bytes(), - ); - - let binary_string = buffer.iter().fold(String::new(), |mut acc, byte| { - write!(&mut acc, "{byte:08b}").unwrap(); - acc - }); - - // Take the last `hint_len` characters - let hint = binary_string - .chars() - .rev() - .take(factor_hint.len()) - .collect::>() - .into_iter() - .rev() - .collect::(); - - if hint != *factor_hint { - return Err(MFKDF2Error::HintMismatch(factor.id.clone())); - } - } - // TODO (autoparallel): It would be preferred to know the size of this array at compile // time. shares_bytes.push(Some(plaintext)); @@ -372,15 +345,64 @@ pub fn key( } let policy_key_bytes = general_purpose::STANDARD.decode(policy.key.as_bytes())?; - let key = decrypt(policy_key_bytes, &kek); + + // Create an internal key for deriving separate keys for parameters, secret, and integrity + let internal_key = decrypt(policy_key_bytes.clone(), &kek); + + // Perform integrity check + let integrity_key = + hkdf_sha256_with_info(&internal_key, &salt_bytes, "mfkdf2:integrity".as_bytes()); + if verify { + let integrity_data = policy.extract(); + let digest = hmacsha256(&integrity_key, &integrity_data); + let hmac = general_purpose::STANDARD.encode(digest); + if policy.hmac != hmac { + return Err(MFKDF2Error::PolicyIntegrityCheckFailed); + } + } for factor in &mut new_policy.factors { let Some(material) = factors.get(factor.id.as_str()) else { continue; }; + // Perform hint verification after policy integrity check + if let Some(ref factor_hint) = factor.hint { + let salt = general_purpose::STANDARD.decode(&factor.salt)?; + let stretched = hkdf_sha256_with_info( + &material.data(), + &salt, + format!("mfkdf2:factor:pad:{}", factor.id).as_bytes(), + ); + + let buffer = hkdf_sha256_with_info( + &stretched, + &salt, + format!("mfkdf2:factor:hint:{}", factor.id).as_bytes(), + ); + + let binary_string = buffer.iter().fold(String::new(), |mut acc, byte| { + write!(&mut acc, "{byte:08b}").unwrap(); + acc + }); + + // Take the last `hint_len` characters + let hint = binary_string + .chars() + .rev() + .take(factor_hint.len()) + .collect::>() + .into_iter() + .rev() + .collect::(); + + if hint != *factor_hint { + return Err(MFKDF2Error::HintMismatch(factor.id.clone())); + } + } + let params_key = hkdf_sha256_with_info( - &key, + &internal_key, &general_purpose::STANDARD.decode(&factor.salt)?, format!("mfkdf2:factor:params:{}", factor.id).as_bytes(), ); @@ -388,15 +410,7 @@ pub fn key( factor.params = params; } - let integrity_key = hkdf_sha256_with_info(&key, &salt_bytes, "mfkdf2:integrity".as_bytes()); - if verify { - let integrity_data = policy.extract(); - let digest = hmacsha256(&integrity_key, &integrity_data); - let hmac = general_purpose::STANDARD.encode(digest); - if policy.hmac != hmac { - return Err(MFKDF2Error::PolicyIntegrityCheckFailed); - } - } + // Update the policy HMAC if !policy.hmac.is_empty() { let integrity_data = new_policy.extract(); let digest = hmacsha256(&integrity_key, &integrity_data); @@ -411,9 +425,16 @@ pub fn key( ) .map_err(|_| MFKDF2Error::ShareRecovery)?; + // derive a dedicated final key to ensure domain separation between internal and external keys + let final_key = if !stack { + hkdf_sha256_with_info(&internal_key, &salt_bytes, "mfkdf2:key:final".as_bytes()) + } else { + internal_key.try_into().map_err(|_| MFKDF2Error::TryFromVec)? + }; + Ok(MFKDF2DerivedKey { policy: new_policy, - key: key.try_into()?, + key: final_key.into(), secret: secret_arr.to_vec(), shares: original_shares.into_iter().map(|s| Vec::from(&s)).collect(), outputs, diff --git a/mfkdf2/src/error.rs b/mfkdf2/src/error.rs index dd954c9d..98b3adf9 100644 --- a/mfkdf2/src/error.rs +++ b/mfkdf2/src/error.rs @@ -113,4 +113,10 @@ pub enum MFKDF2Error { #[error(transparent)] Regex(#[from] rand_regex::Error), + + #[error(transparent)] + Encrypt(#[from] cipher::inout::PadError), + + #[error(transparent)] + Decrypt(#[from] cipher::inout::block_padding::UnpadError), } diff --git a/mfkdf2/src/integrity.rs b/mfkdf2/src/integrity.rs index 8b8d314c..fdff2bbd 100644 --- a/mfkdf2/src/integrity.rs +++ b/mfkdf2/src/integrity.rs @@ -62,6 +62,7 @@ pub fn extract_factor_core(factor: &PolicyFactor) -> [u8; 32] { hasher.update(factor.pad.as_bytes()); hasher.update(factor.salt.as_bytes()); hasher.update(factor.secret.as_bytes()); + hasher.update(factor.hint.as_ref().unwrap_or(&String::new()).as_bytes()); hasher.finalize().into() } diff --git a/mfkdf2/src/setup/factors/ooba.rs b/mfkdf2/src/setup/factors/ooba.rs index 7c64c96f..7280b751 100644 --- a/mfkdf2/src/setup/factors/ooba.rs +++ b/mfkdf2/src/setup/factors/ooba.rs @@ -24,7 +24,10 @@ //! login. Even if the OOBA channel is only partially trusted, the resulting factor //! material remains uniformly distributed and provides the same information‑theoretic //! guarantees as the HOTP and TOTP constructions. +use aes::Aes256; use base64::{Engine, engine::general_purpose}; +use cbc::Encryptor; +use cipher::KeyIvInit; use jsonwebtoken::jwk::Jwk; use rand::rngs::OsRng; use rsa::{Oaep, RsaPublicKey}; @@ -33,9 +36,10 @@ use serde_json::{Value, json}; use sha2::Sha256; use crate::{ - crypto::{encrypt, hkdf_sha256_with_info}, + crypto::{encrypt_cbc, hkdf_sha256_with_info}, definitions::{FactorType, Key, MFKDF2Factor, factor::FactorMetadata}, error::{MFKDF2Error, MFKDF2Result}, + rng::GlobalRng, setup::FactorSetup, }; @@ -136,11 +140,21 @@ impl FactorSetup for Ooba { let code = generate_alphanumeric_characters(self.length.into()).to_uppercase(); let prev_key = hkdf_sha256_with_info(code.as_bytes(), &[], &[]); - let pad = encrypt(&self.target, &prev_key); + let key = cipher::Key::>::from(prev_key); + let iv = Encryptor::::generate_iv(&mut GlobalRng); + let pad = encrypt_cbc::(&self.target, &key, &iv)?; + + // store the secret code for the next derivation let mut params = self.params.clone(); params["code"] = json!(code); + params["iv"] = json!(general_purpose::STANDARD.encode(iv)); + + // store the iv in public params for the next derivation + let mut pub_params = self.params.clone(); + pub_params["iv"] = json!(general_purpose::STANDARD.encode(iv)); + // encrypt the params and store the ciphertext for the next derivation let plaintext = serde_json::to_vec(¶ms)?; let key = OobaPublicKey::try_from(self.jwk.as_ref().ok_or(MFKDF2Error::MissingOobaKey)?)?; let ciphertext = key.0.encrypt(&mut OsRng, Oaep::new::(), &plaintext)?; @@ -148,7 +162,7 @@ impl FactorSetup for Ooba { Ok(json!({ "length": self.length, "key": self.jwk, - "params": self.params, + "params": pub_params, "next": hex::encode(ciphertext), "pad": general_purpose::STANDARD.encode(pad), })) @@ -249,7 +263,6 @@ mod tests { use rsa::{RsaPrivateKey, traits::PublicKeyParts}; use super::*; - use crate::crypto::decrypt; fn keypair() -> (RsaPrivateKey, RsaPublicKey) { let bits = 2048; @@ -392,10 +405,13 @@ mod tests { ) .unwrap(); let code = decrypted["code"].as_str().unwrap(); + let iv = general_purpose::STANDARD.decode(decrypted["iv"].as_str().unwrap()).unwrap(); let prev_key = hkdf_sha256_with_info(code.as_bytes(), &[], &[]); let pad = general_purpose::STANDARD.decode(params["pad"].as_str().unwrap()).unwrap(); - let target = decrypt(pad, &prev_key); + let key = cipher::Key::>::from(prev_key); + let iv = cipher::Iv::>::from_iter(iv); + let target = crate::crypto::decrypt_cbc::(&pad, &key, &iv).unwrap(); assert_eq!(ooba.target, target); } diff --git a/mfkdf2/src/setup/key.rs b/mfkdf2/src/setup/key.rs index aee9e617..81ccc1d2 100644 --- a/mfkdf2/src/setup/key.rs +++ b/mfkdf2/src/setup/key.rs @@ -217,8 +217,9 @@ pub fn key(factors: &[MFKDF2Factor], options: MFKDF2Options) -> MFKDF2Result MFKDF2Result MFKDF2Result MFKDF2Result MFKDF2Result> = derived_key.shares.iter().take(threshold as usize).cloned().collect();