Rust wrapper: add password-hash, kem, mac traits; fix a few Fenrir findings#10305
Rust wrapper: add password-hash, kem, mac traits; fix a few Fenrir findings#10305holtrop-wolfssl wants to merge 8 commits intowolfSSL:masterfrom
Conversation
…_key() Fixes F-3093
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds RustCrypto trait support (password-hash, KEM, MAC) to the wolfCrypt Rust wrapper while addressing several static-analysis (Fenrir) findings and aligning tests/docs with API changes.
Changes:
- Add PBKDF2
password-hash(PHC) hasher/verifier support and corresponding tests. - Add ML-KEM RustCrypto
kemtrait wrappers and tests. - Add HMAC/CMAC RustCrypto
digest::Macwrappers and tests; include ECC/LMS/ML-KEM fixes and doc/test updates.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| wrapper/rust/wolfssl-wolfcrypt/src/pbkdf2_password_hash.rs | Implements password-hash traits for PBKDF2 and PHC formatting/parsing. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_pbkdf2_password_hash.rs | Adds PBKDF2 password-hash tests (roundtrip, params, algorithm variants). |
| wrapper/rust/wolfssl-wolfcrypt/src/mlkem_kem.rs | Adds RustCrypto kem trait wrappers for ML-KEM. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_mlkem_kem.rs | Adds ML-KEM KEM trait tests (roundtrip, export/import, rejection). |
| wrapper/rust/wolfssl-wolfcrypt/src/hmac_mac.rs | Adds RustCrypto digest::Mac trait wrappers for HMAC variants. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_hmac_mac.rs | Adds HMAC MAC-trait vector and behavior tests. |
| wrapper/rust/wolfssl-wolfcrypt/src/cmac_mac.rs | Adds RustCrypto digest::Mac trait wrappers for AES-CMAC. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_cmac_mac.rs | Adds CMAC MAC-trait tests including wrong key size. |
| wrapper/rust/wolfssl-wolfcrypt/src/mlkem.rs | Changes ML-KEM encode APIs to return Result<(), i32> and updates docs. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_mlkem.rs | Updates ML-KEM tests for new encode API return type. |
| wrapper/rust/wolfssl-wolfcrypt/src/lms.rs | Renames signature-availability API to has_sigs_left and updates docs. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_lms.rs | Updates tests for has_sigs_left rename. |
| wrapper/rust/wolfssl-wolfcrypt/src/ecc.rs | Adds NUL-termination validation and adjusts return-value handling. |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_ecc.rs | Adds test coverage for rs_hex_to_sig NUL-termination behavior. |
| wrapper/rust/wolfssl-wolfcrypt/src/lib.rs | Exposes new modules behind feature/cfg gates. |
| wrapper/rust/wolfssl-wolfcrypt/Cargo.toml | Adds new features/deps for mac, password-hash, kem, hybrid-array. |
| wrapper/rust/wolfssl-wolfcrypt/Makefile | Enables new features in the default feature list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut out_buf = [0u8; Output::MAX_LENGTH]; | ||
| let out_slice = &mut out_buf[..params.output_len]; | ||
| kdf::pbkdf2(password, salt.as_ref(), iterations, algorithm.hmac_type(), out_slice) | ||
| .map_err(|_| Error::Crypto)?; | ||
| let output = Output::new(out_slice)?; |
There was a problem hiding this comment.
params.output_len is used to slice out_buf without validating it is within 1..=Output::MAX_LENGTH. If a caller supplies output_len > Output::MAX_LENGTH, this will panic (DoS). Add explicit bounds validation for params.output_len and return Error::ParamInvalid { name: "l" } (or similar) instead of panicking.
| let output_len = if let Some(ref h) = hash.hash { | ||
| h.len() | ||
| } else if let Some(l) = hash.params.get_decimal("l") { | ||
| l as usize |
There was a problem hiding this comment.
l as usize is accepted without validating the size is sane (e.g., non-zero and <= Output::MAX_LENGTH). This can later lead to panics when allocating/slicing output buffers based on parsed PHC parameters. Validate l (range + overflow-safe conversion) and reject invalid values with Error::ParamInvalid { name: "l" }.
| l as usize | |
| let output_len = | |
| usize::try_from(l).map_err(|_| Error::ParamInvalid { name: "l" })?; | |
| if output_len == 0 || output_len > Output::MAX_LENGTH { | |
| return Err(Error::ParamInvalid { name: "l" }); | |
| } | |
| output_len |
| /// } | ||
| /// ``` | ||
| pub fn rs_hex_to_sig(r: &[u8], s: &[u8], dout: &mut [u8]) -> Result<usize, i32> { | ||
| if r[r.len() - 1] != 0 || s[s.len() - 1] != 0 { |
There was a problem hiding this comment.
This will panic if r or s is empty due to r[r.len() - 1]. Add an explicit empty-slice guard (treat empty as BAD_FUNC_ARG) before checking the trailing NUL byte.
| if r[r.len() - 1] != 0 || s[s.len() - 1] != 0 { | |
| if r.is_empty() || s.is_empty() || r[r.len() - 1] != 0 || s[s.len() - 1] != 0 { |
| #[test] | ||
| fn test_ecc_rs_hex_to_sig_not_null_terminated() { | ||
| let r_hex = b"AABB\0"; | ||
| let s_hex = b"CCDD\0"; | ||
| let r_hex_no_nul = b"AABB"; | ||
| let s_hex_no_nul = b"CCDD"; | ||
| let mut sig_out = [0u8; 128]; | ||
|
|
||
| // Both null-terminated should succeed | ||
| assert!(ECC::rs_hex_to_sig(r_hex, s_hex, &mut sig_out).is_ok()); |
There was a problem hiding this comment.
Most tests in this crate call common::setup() to ensure wolfCrypt is initialized. This new test does not, which can make it order-dependent if initialization is required. Add common::setup(); at the start of this test for consistency and to avoid potential flakes.
| #[cfg(feature = "kem")] | ||
| pub mod mlkem_kem; |
There was a problem hiding this comment.
The module declarations are gated only on the Cargo features, but the module files themselves have stricter cfgs (mlkem_kem requires mlkem, pbkdf2_password_hash requires hmac + kdf_pbkdf2). This can expose “present but empty” modules (or missing items) when the feature is enabled without the underlying algorithm cfgs. Align the lib.rs #[cfg(...)] conditions with the module-level cfg requirements to make the public API predictable.
| #[cfg(feature = "password-hash")] | ||
| pub mod pbkdf2_password_hash; |
There was a problem hiding this comment.
The module declarations are gated only on the Cargo features, but the module files themselves have stricter cfgs (mlkem_kem requires mlkem, pbkdf2_password_hash requires hmac + kdf_pbkdf2). This can expose “present but empty” modules (or missing items) when the feature is enabled without the underlying algorithm cfgs. Align the lib.rs #[cfg(...)] conditions with the module-level cfg requirements to make the public API predictable.
| } | ||
|
|
||
| /// Return the number of signatures remaining for this key. | ||
| /// Return whether there are more signatures remaining for this key. |
There was a problem hiding this comment.
Renaming sigs_left to has_sigs_left is a breaking change for any downstream users. Consider keeping sigs_left as a deprecated wrapper around has_sigs_left (at least for one release), so consumers get a graceful transition path.
| pub fn has_sigs_left(&mut self) -> Result<bool, i32> { | ||
| let rc = unsafe { sys::wc_LmsKey_SigsLeft(&mut self.ws_key) }; |
There was a problem hiding this comment.
Renaming sigs_left to has_sigs_left is a breaking change for any downstream users. Consider keeping sigs_left as a deprecated wrapper around has_sigs_left (at least for one release), so consumers get a graceful transition path.
Description
Rust wrapper: add password-hash, kem, mac traits; fix a few Fenrir findings
Testing
Unit/CI tests
Checklist