fix: audit and common fixes by lonerapier · Pull Request #55 · multifactor/MFKDF2.rs

lonerapier · 2025-11-25T11:45:24Z

Audit fixes:

Not done:

F2: secret material data is not stored in policy anywhere, so during hint addition we decrypt stretched factor material to derive the hint key.
F4: Mentioned in the docs, but no specific changes made to library.
F5: Integrity protection is already true by default, but library delegate the decision to user at the end.

Autoparallel · 2025-12-01T19:06:50Z

Made this a draft for now.

* add cbc encryption for ooba target * use crate's inner trait method * add security warning

* add hint to integrity checks * add integrity check for add hint * verify policy integrity before hint checks

) clippy fixes

Copilot

Pull request overview

This PR implements several audit-driven security hardenings across the Rust core and web bindings, primarily focusing on OOBA pad encryption, policy integrity/hint handling, and key domain separation. It also updates CI/workflows and differential-testing pinning to improve reproducibility.

Changes:

Switch OOBA pad encryption from ECB to CBC and persist an IV in public params for decryption.
Introduce “internal key” vs “final key” domain separation and update setup/derive + derived-key utilities to use the internal key for policy operations.
Expand integrity coverage to include hints and reorder integrity verification ahead of hint checks to avoid entropy leakage.

Reviewed changes

Copilot reviewed 23 out of 25 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
`mfkdf2/src/setup/key.rs`	Generate/store an internal key in policy; derive a separate final key for output.
`mfkdf2/src/setup/factors/ooba.rs`	Use AES-CBC with random IV for OOBA pad; expose IV via public params.
`mfkdf2/src/integrity.rs`	Include factor hints in integrity extraction.
`mfkdf2/src/error.rs`	Add error variants to propagate CBC pad/unpad failures.
`mfkdf2/src/derive/key.rs`	Decrypt internal key, verify integrity earlier, and derive final key separately.
`mfkdf2/src/derive/factors/ooba.rs`	Decrypt/encrypt OOBA pads using CBC and IV stored in params; adjust tests.
`mfkdf2/src/definitions/mfkdf_derived_key/strengthening.rs`	Re-encrypt policy key during strengthening using derived internal key.
`mfkdf2/src/definitions/mfkdf_derived_key/reconstitution.rs`	Use internal key for factor secret/params derivations and integrity updates.
`mfkdf2/src/definitions/mfkdf_derived_key/mod.rs`	Add `derive_internal_key()` helper for internal key recovery from policy.
`mfkdf2/src/definitions/mfkdf_derived_key/mfdpg.rs`	Update MFDPG API to propagate new `get_subkey()` result type.
`mfkdf2/src/definitions/mfkdf_derived_key/hints.rs`	Add integrity verify/update around hint insertion; add leakage demonstration tests.
`mfkdf2/src/definitions/mfkdf_derived_key/crypto.rs`	Make `get_subkey()` fallible and derive from internal key path.
`mfkdf2/src/definitions/factor.rs`	Adjust docs example imports/usage.
`mfkdf2/src/crypto.rs`	Add CBC encrypt/decrypt helpers.
`mfkdf2/README.md`	Add notes on old-policy invalidation and integrity behavior.
`mfkdf2/Cargo.toml`	Add `cbc` dependency and enable `cipher` features needed for IV generation.
`mfkdf2-web/test/factors/hmacsha1.test.ts`	Update expected derived key output.
`mfkdf2-web/package.json`	Pin `mfkdf` reference dependency to a specific commit.
`mfkdf2-web/package-lock.json`	Lockfile update for the pinned `mfkdf` dependency.
`docs/src/differential-tests.md`	Document pinned commits and local repro steps for differential testing.
`SECURITY.md`	Record an additional known dependency concern.
`Cargo.lock`	Lock new Rust dependencies (notably `cbc` and `rand_core`).
`.github/workflows/rust.yaml`	Add doctest step; adjust coverage command flags.
`.github/workflows/differential.yaml`	Add dedicated workflow for pinned differential tests.
`.github/workflows/bindings.yaml`	Remove differential-test steps from the general bindings workflow.

Files not reviewed (1)

mfkdf2-web/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T21:37:56Z

    .hash_password_into(&self.secret, &salt, &mut kek)?;

-    let policy_key = encrypt(self.key.as_ref(), &kek);
+    let policy_key = encrypt(&internal_key, &kek);


internal_key here is a Key (ByteArray<32>), but crypto::encrypt expects a &[u8; 32]. Passing &internal_key won’t type-check (it derefs to &[u8]). Use &internal_key.0 (or change encrypt to accept AsRef<[u8]>) so this compiles and the intended bytes are encrypted into policy.key.

Suggested change

let policy_key = encrypt(&internal_key, &kek);

let policy_key = encrypt(&internal_key.0, &kek);

Copilot · 2026-04-20T21:37:56Z

+  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()))


get_subkey now calls derive_internal_key(), which runs Argon2 and decrypts the policy key each time a subkey is requested. This can make subkey derivation unexpectedly expensive (e.g., MFDPG password generation in tight loops). Consider caching the internal key once in MFKDF2DerivedKey (or caching the derived subkey) so repeated calls don’t repeatedly execute Argon2.

Copilot · 2026-04-20T21:37:57Z

 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


Typo in section header: "Integrity Protetion" should be "Integrity Protection".

Suggested change

# Integrity Protetion

# Integrity Protection

Copilot · 2026-04-20T21:37:57Z

  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());


Using factor.hint.as_ref().unwrap_or(&String::new()) creates a temporary String just to get an empty &str. Prefer factor.hint.as_deref().unwrap_or("") (or map_or(b"", |h| h.as_bytes())) to avoid the temporary and make the intent clearer.

Suggested change

hasher.update(factor.hint.as_ref().unwrap_or(&String::new()).as_bytes());

hasher.update(factor.hint.as_deref().unwrap_or("").as_bytes());

Copilot · 2026-04-20T21:37:57Z

    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?);


add_hint uses find(...).unwrap() when locating the factor to attach the hint. If factor_id is wrong/missing, this will panic instead of returning a structured error (like MFKDF2Error::MissingFactor, consistent with get_hint). Please handle the None case and return an error.

Suggested change

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?);

let hint = self.get_hint(factor_id, bits)?;

let factor_data = self

.policy

.factors

.iter_mut()

.find(|f| f.id == factor_id)

.ok_or_else(|| MFKDF2Error::MissingFactor(factor_id.to_string()))?;

factor_data.hint = Some(hint);

Copilot · 2026-04-20T21:37:58Z

+    // 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();


add_hint derives internal_key via Argon2, then calls get_hint, which derives the internal key again. Since Argon2 can be expensive (and may allocate large memory depending on policy), this doubles the cost of add_hint. Consider refactoring so get_hint can reuse the already-derived internal key (or cache the internal key in MFKDF2DerivedKey for the duration of the object).

lonerapier marked this pull request as ready for review November 25, 2025 11:45

Autoparallel added the type: security label Nov 25, 2025

Autoparallel marked this pull request as draft December 1, 2025 19:06

Autoparallel added lang: rust area: testing labels Dec 1, 2025

lonerapier force-pushed the fix/audit branch from a084085 to 73d02d9 Compare December 2, 2025 09:20

lonerapier marked this pull request as ready for review December 4, 2025 19:02

lonerapier added 4 commits December 5, 2025 00:41

fix doctests

705f2c6

task: pin differential tests to specific commit (#58)

40635f8

fix(F7): CBC encryption for ooba target (#56)

84407e6

* add cbc encryption for ooba target * use crate's inner trait method * add security warning

fix(F6): hint entropy leak (#57)

f07a884

* add hint to integrity checks * add integrity check for add hint * verify policy integrity before hint checks

lonerapier force-pushed the fix/audit branch from 73d02d9 to 9d3b905 Compare December 4, 2025 19:32

lonerapier requested a review from Autoparallel December 4, 2025 19:32

lonerapier added 2 commits December 5, 2025 01:04

fix(F3): derive internal key and external key for domain separation (#59

83c0ac5

) clippy fixes

fix(F4): add docs for integrity

7a62f03

lonerapier force-pushed the fix/audit branch from 9d3b905 to 7a62f03 Compare December 4, 2025 19:35

VCNinc requested review from Copilot and removed request for Autoparallel April 20, 2026 21:28

Copilot started reviewing on behalf of VCNinc April 20, 2026 21:29 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

VCNinc merged commit 3c81376 into main Apr 20, 2026
14 checks passed

VCNinc deleted the fix/audit branch April 20, 2026 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: audit and common fixes#55

fix: audit and common fixes#55
VCNinc merged 6 commits intomainfrom
fix/audit

lonerapier commented Nov 25, 2025 •

edited

Loading

Uh oh!

Autoparallel commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	let policy_key = encrypt(&internal_key, &kek);
	let policy_key = encrypt(&internal_key.0, &kek);

	hasher.update(factor.hint.as_ref().unwrap_or(&String::new()).as_bytes());
	hasher.update(factor.hint.as_deref().unwrap_or("").as_bytes());

Conversation

lonerapier commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Autoparallel commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lonerapier commented Nov 25, 2025 •

edited

Loading