Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Fenrir findings in the wolfssl-wolfcrypt Rust wrapper by hardening FFI boundary handling (size conversions), introducing zeroization behavior for sensitive contexts on drop, and adjusting some APIs/tests accordingly.
Changes:
- Replaced many
len() as u32/i32casts with checked conversions (buffer_len_to_u32/buffer_len_to_i32) before calling into the C API. - Added
zeroizesupport and explicit zeroization inDropimplementations across multiple crypto/RNG/key wrapper types. - Updated LMS Key ID retrieval API (
get_kid) and adapted the LMS unit test.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| wrapper/rust/wolfssl-wolfcrypt/tests/test_lms.rs | Updates LMS test to use the new get_kid(&mut [u8]) API. |
| wrapper/rust/wolfssl-wolfcrypt/src/sha.rs | Adds checked length conversions and zeroization-on-drop for SHA/SHA3/SHAKE contexts. |
| wrapper/rust/wolfssl-wolfcrypt/src/rsa.rs | Adds checked length conversions and zeroization behavior for RSA wrapper. |
| wrapper/rust/wolfssl-wolfcrypt/src/random.rs | Fixes RNG initialization pattern and adds checked size conversions + zeroization-on-drop. |
| wrapper/rust/wolfssl-wolfcrypt/src/prf.rs | Uses checked size conversions for PRF sizes passed to C. |
| wrapper/rust/wolfssl-wolfcrypt/src/mlkem.rs | Uses checked size conversions, documents thread-safety, and adds zeroization/drop behavior for ML-KEM wrapper. |
| wrapper/rust/wolfssl-wolfcrypt/src/lms.rs | Uses checked size conversions; changes get_kid API; adds zeroization-on-drop for LMS key. |
| wrapper/rust/wolfssl-wolfcrypt/src/lib.rs | Introduces zeroize_raw helper and size conversion helpers (buffer_len_to_u32/i32). |
| wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs | Uses checked size conversions; adds SRTP KDF argument validation. |
| wrapper/rust/wolfssl-wolfcrypt/src/hmac.rs | Uses checked size conversions and adds zeroization-on-drop for HMAC context. |
| wrapper/rust/wolfssl-wolfcrypt/src/hkdf.rs | Uses checked size conversions for HKDF input/output sizes. |
| wrapper/rust/wolfssl-wolfcrypt/src/ed448.rs | Uses checked size conversions; validates Ed448 context length; adds zeroization-on-drop. |
| wrapper/rust/wolfssl-wolfcrypt/src/ed25519.rs | Uses checked size conversions; validates Ed25519 context length; adds zeroization-on-drop. |
| wrapper/rust/wolfssl-wolfcrypt/src/ecc.rs | Uses checked size conversions; refactors init/error paths; adds zeroization-on-drop for ECC/ECCPoint. |
| wrapper/rust/wolfssl-wolfcrypt/src/dilithium.rs | Uses checked size conversions; simplifies cfg usage; adds zeroization-on-drop. |
| wrapper/rust/wolfssl-wolfcrypt/src/dh.rs | Uses checked size conversions and adds zeroization-on-drop for DH context; adds overflow guards in compare helper. |
| wrapper/rust/wolfssl-wolfcrypt/src/curve25519.rs | Uses checked size conversions and adds zeroization-on-drop for Curve25519 key wrapper. |
| wrapper/rust/wolfssl-wolfcrypt/src/cmac.rs | Uses checked size conversions and adds zeroization-on-drop for CMAC context. |
| wrapper/rust/wolfssl-wolfcrypt/src/chacha20_poly1305.rs | Uses checked size conversions; adds key zeroization via derive for AEAD key-holder structs. |
| wrapper/rust/wolfssl-wolfcrypt/src/blake2.rs | Uses checked size conversions; adds zeroization-on-drop; prevents empty finalize buffer edge-case. |
| wrapper/rust/wolfssl-wolfcrypt/src/aes.rs | Uses checked size conversions; tightens APIs to &[u8]; adds zeroization-on-drop; adds bounds checks in AEAD helpers. |
| wrapper/rust/wolfssl-wolfcrypt/build.rs | Removes cfg scanning for Dilithium seed constants. |
| wrapper/rust/wolfssl-wolfcrypt/Cargo.toml | Adds zeroize dependency. |
| wrapper/rust/wolfssl-wolfcrypt/Cargo.lock | Locks zeroize (and derive) dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs:600
wc_SRTP_KDFalways treats the SRTP index asWC_SRTP_INDEX_LENbytes (6) on the C side, regardless ofkdr_index(see wolfcrypt/src/kdf.c whereWC_SRTP_INDEX_LENis passed). The current validation based onidx.len() * 8doesn't prevent passing a shorteridxslice, which can lead to out-of-bounds reads in the C code. Please validateidx.len() == WC_SRTP_INDEX_LEN(or at least>=) before calling into C (and adjust/remove the bit-based check accordingly).
if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) {
// The kdr_index value must be either -1 or the number of bits that
// will be read from the idx slice.
return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG);
}
let key_size = crate::buffer_len_to_u32(key.len())?;
let salt_size = crate::buffer_len_to_u32(salt.len())?;
let key1_size = crate::buffer_len_to_u32(key1.len())?;
let key2_size = crate::buffer_len_to_u32(key2.len())?;
let key3_size = crate::buffer_len_to_u32(key3.len())?;
let rc = unsafe {
sys::wc_SRTP_KDF(key.as_ptr(), key_size, salt.as_ptr(), salt_size,
kdr_index, idx.as_ptr(), key1.as_mut_ptr(), key1_size,
key2.as_mut_ptr(), key2_size, key3.as_mut_ptr(), key3_size)
wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs:706
- Same issue as
srtp_kdf:wc_SRTCP_KDFuses a fixed index length (WC_SRTCP_INDEX_LEN, 4 bytes) internally in C, so the currentidx.len() * 8validation doesn't ensure the slice is long enough. Passing a shortidxcan cause out-of-bounds reads in the C implementation. Validateidx.len() == WC_SRTCP_INDEX_LEN(or at least>=) before calling into C.
if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) {
// The kdr_index value must be either -1 or the number of bits that
// will be read from the idx slice.
return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG);
}
let key_size = crate::buffer_len_to_u32(key.len())?;
let salt_size = crate::buffer_len_to_u32(salt.len())?;
let key1_size = crate::buffer_len_to_u32(key1.len())?;
let key2_size = crate::buffer_len_to_u32(key2.len())?;
let key3_size = crate::buffer_len_to_u32(key3.len())?;
let rc = unsafe {
sys::wc_SRTCP_KDF(key.as_ptr(), key_size, salt.as_ptr(), salt_size,
kdr_index, idx.as_ptr(), key1.as_mut_ptr(), key1_size,
key2.as_mut_ptr(), key2_size, key3.as_mut_ptr(), key3_size)
};
wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs:648
wc_SRTP_KDF_labelassumes an SRTP index ofWC_SRTP_INDEX_LENbytes (6) on the C side. This wrapper doesn't validateidxlength before passingidx.as_ptr(), so a too-short slice can trigger out-of-bounds reads in C. Please enforce the required index length (e.g.,idx.len() == WC_SRTP_INDEX_LEN).
pub fn srtp_kdf_label(key: &[u8], salt: &[u8], kdr_index: i32, idx: &[u8],
label: u8, keyout: &mut [u8]) -> Result<(), i32> {
let key_size = crate::buffer_len_to_u32(key.len())?;
let salt_size = crate::buffer_len_to_u32(salt.len())?;
let keyout_size = crate::buffer_len_to_u32(keyout.len())?;
let rc = unsafe {
sys::wc_SRTP_KDF_label(key.as_ptr(), key_size, salt.as_ptr(), salt_size,
kdr_index, idx.as_ptr(), label, keyout.as_mut_ptr(), keyout_size)
};
wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs:753
wc_SRTCP_KDF_labelassumes an SRTCP index ofWC_SRTCP_INDEX_LENbytes (4) on the C side. This wrapper doesn't validateidxlength before passingidx.as_ptr(), so a too-short slice can trigger out-of-bounds reads in C. Please enforce the required index length (e.g.,idx.len() == WC_SRTCP_INDEX_LEN).
pub fn srtcp_kdf_label(key: &[u8], salt: &[u8], kdr_index: i32, idx: &[u8],
label: u8, keyout: &mut [u8]) -> Result<(), i32> {
let key_size = crate::buffer_len_to_u32(key.len())?;
let salt_size = crate::buffer_len_to_u32(salt.len())?;
let keyout_size = crate::buffer_len_to_u32(keyout.len())?;
let rc = unsafe {
sys::wc_SRTCP_KDF_label(key.as_ptr(), key_size, salt.as_ptr(), salt_size,
kdr_index, idx.as_ptr(), label, keyout.as_mut_ptr(), keyout_size)
};
wrapper/rust/wolfssl-wolfcrypt/src/blake2.rs:96
digest_sizeis still cast withas u32here. If a caller passes adigest_size>u32::MAX, this will truncate and pass an unexpected value into the C API. Consider converting withbuffer_len_to_u32(digest_size)(and ideally also validating it against the algorithm's allowed digest sizes) to avoid silent truncation.
pub fn new_with_key(digest_size: usize, key: &[u8]) -> Result<Self, i32> {
let key_size = crate::buffer_len_to_u32(key.len())?;
let digest_size = digest_size as u32;
let mut wc_blake2b: MaybeUninit<sys::Blake2b> = MaybeUninit::uninit();
let rc = unsafe {
sys::wc_InitBlake2b_WithKey(wc_blake2b.as_mut_ptr(), digest_size,
key.as_ptr(), key_size)
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retest this please (job removed) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix F-1062. If wolfSSL returns an error after initializing ECC struct with wc_ecc_init_ex(), wc_ecc_free() might not have been called in all cases. Move construction of the ECC struct earlier ahead of further wolfSSL calls after wc_ecc_init_ex() so if those subsequent wolfSSL calls return an error the Drop impl for ECC will be called to deinitialize.
This is related to F-1070 but not the same. We do not need to check that hash_size being passed in matches the initialized digest size because the C function will use the passed-in size as long as it is non-zero.
Fixes F-1071. This is an API-breaking change, so will lead to a new crate major version.
Fixes F-1065.
3bc4154 to
d957afb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
wrapper/rust/wolfssl-wolfcrypt/src/kdf.rs:648
srtp_kdf_label()forwardskdr_index/idxdirectly intowc_SRTP_KDF_label(), which in wolfCrypt expects an SRTP index of fixed length (WC_SRTP_INDEX_LEN = 6) and akdrIdxin range -1..=24. Unlikesrtp_kdf(), this wrapper currently does no pre-validation ofkdr_indexoridxlength, so a shortidxslice can cause the C code to read out of bounds. Add the samekdr_indexrange andidxlength validation here as insrtp_kdf()(but aligned to wolfCrypt’s documented constraints).
pub fn srtp_kdf_label(key: &[u8], salt: &[u8], kdr_index: i32, idx: &[u8],
label: u8, keyout: &mut [u8]) -> Result<(), i32> {
let key_size = crate::buffer_len_to_u32(key.len())?;
let salt_size = crate::buffer_len_to_u32(salt.len())?;
let keyout_size = crate::buffer_len_to_u32(keyout.len())?;
let rc = unsafe {
sys::wc_SRTP_KDF_label(key.as_ptr(), key_size, salt.as_ptr(), salt_size,
kdr_index, idx.as_ptr(), label, keyout.as_mut_ptr(), keyout_size)
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) { | ||
| // The kdr_index value must be either -1 or the number of bits that | ||
| // will be read from the idx slice. | ||
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | ||
| } |
There was a problem hiding this comment.
srtcp_kdf() uses the same kdr_index <= idx.len() * 8 check as SRTP, but the C API (wc_SRTCP_KDF) treats idx as a fixed-length SRTCP index (WC_SRTCP_INDEX_LEN = 4) and reads from it based on kdrIdx. With a short idx slice, this wrapper can still pass validation and trigger an out-of-bounds read in C. Update validation to match wolfCrypt: enforce kdr_index in -1..=24 and require idx.len() to be sufficient for what wolfCrypt reads (e.g., at least 4 bytes when kdr_index >= 0, or 4 - (kdr_index >> 3)), returning BAD_FUNC_ARG when invalid.
| if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) { | |
| // The kdr_index value must be either -1 or the number of bits that | |
| // will be read from the idx slice. | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| if !(-1..=24).contains(&kdr_index) { | |
| // wolfCrypt accepts only -1 or a key-derivation rate bit index in the | |
| // fixed-size 32-bit SRTCP index. | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| if kdr_index >= 0 { | |
| // wc_SRTCP_KDF treats idx as a 4-byte SRTCP index and reads from it | |
| // based on kdr_index. Ensure the slice is large enough for the bytes | |
| // wolfCrypt may access before passing it across the FFI boundary. | |
| let required_idx_len = 4usize - ((kdr_index as usize) >> 3); | |
| if idx.len() < required_idx_len { | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| } |
| pub fn srtcp_kdf_label(key: &[u8], salt: &[u8], kdr_index: i32, idx: &[u8], | ||
| label: u8, keyout: &mut [u8]) -> Result<(), i32> { | ||
| let key_size = key.len() as u32; | ||
| let salt_size = salt.len() as u32; | ||
| let keyout_size = keyout.len() as u32; | ||
| let key_size = crate::buffer_len_to_u32(key.len())?; | ||
| let salt_size = crate::buffer_len_to_u32(salt.len())?; | ||
| let keyout_size = crate::buffer_len_to_u32(keyout.len())?; | ||
| let rc = unsafe { | ||
| sys::wc_SRTCP_KDF_label(key.as_ptr(), key_size, salt.as_ptr(), salt_size, | ||
| kdr_index, idx.as_ptr(), label, keyout.as_mut_ptr(), keyout_size) |
There was a problem hiding this comment.
srtcp_kdf_label() currently performs no validation of kdr_index or idx length before calling wc_SRTCP_KDF_label(). wolfCrypt expects a fixed-length SRTCP index (WC_SRTCP_INDEX_LEN = 4) and kdrIdx in -1..=24, and will read from idx accordingly; a short Rust slice can therefore lead to an OOB read in the C code. Add the same kdr_index range and idx length validation as srtcp_kdf() (but aligned to wolfCrypt’s documented constraints).
| let max_valid = (index.len() * 8) as i32; | ||
| let mut key_e = [0u8; 16]; | ||
| let mut key_a = [0u8; 20]; | ||
| let mut key_s = [0u8; 14]; | ||
|
|
||
| let rc = srtp_kdf(&key, &salt, -2, &index, &mut key_e, &mut key_a, &mut key_s) | ||
| .expect_err("srtp_kdf() should fail for kdr_index = -2"); | ||
| assert_eq!(rc, sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | ||
|
|
||
| let rc = srtp_kdf(&key, &salt, max_valid + 1, &index, &mut key_e, &mut key_a, &mut key_s) | ||
| .expect_err("srtp_kdf() should fail for too-large kdr_index"); | ||
| assert_eq!(rc, sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); |
There was a problem hiding this comment.
These tests derive max_valid from index.len() * 8 and use that to define the upper bound for invalid kdr_index. In wolfCrypt, kdrIdx is constrained to -1..=24 regardless of index length (see docs / wc_SRTP_KDF_label validation in wolfcrypt/src/kdf.c). If the wrapper validation is updated to match wolfCrypt, these tests should be updated to use a value like 25 (and also consider adding coverage for too-short idx slices, which can otherwise lead to out-of-bounds reads in the C code).
| let max_valid = (index.len() * 8) as i32; | ||
| let mut key_e = [0u8; 16]; | ||
| let mut key_a = [0u8; 20]; | ||
| let mut key_s = [0u8; 14]; | ||
|
|
||
| let rc = srtcp_kdf(&key, &salt, -2, &index, &mut key_e, &mut key_a, &mut key_s) | ||
| .expect_err("srtcp_kdf() should fail for kdr_index = -2"); | ||
| assert_eq!(rc, sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | ||
|
|
||
| let rc = srtcp_kdf(&key, &salt, max_valid + 1, &index, &mut key_e, &mut key_a, &mut key_s) | ||
| .expect_err("srtcp_kdf() should fail for too-large kdr_index"); | ||
| assert_eq!(rc, sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); |
There was a problem hiding this comment.
Same issue as the SRTP test above: max_valid is computed from index.len() * 8, but wolfCrypt constrains kdrIdx to -1..=24. If the wrapper is aligned to wolfCrypt semantics, update this test to use 25 as the invalid upper bound (and consider adding a regression test for too-short idx slices).
| if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) { | ||
| // The kdr_index value must be either -1 or the number of bits that | ||
| // will be read from the idx slice. | ||
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | ||
| } |
There was a problem hiding this comment.
srtp_kdf() validates kdr_index against idx.len() * 8, but the underlying C API (wc_SRTP_KDF) always treats idx as a fixed-length SRTP index (WC_SRTP_INDEX_LEN = 6) and will read from idx based on kdrIdx (see wc_srtp_kdf_first_block in wolfcrypt/src/kdf.c). As a result, callers can pass a short idx slice (e.g., 1 byte) with kdr_index = 0, this check passes, and the C code will read past the end of the slice (OOB read / memory safety issue). Update the wrapper to (a) enforce the wolfCrypt range for kdr_index (-1..=24), and (b) require idx.len() to be large enough for the bytes the C code may read (e.g., at least 6 bytes when kdr_index >= 0, or more precisely 6 - (kdr_index >> 3)), returning BAD_FUNC_ARG on violation. Also update the comment here—the current wording about “number of bits read from idx” doesn’t match wolfCrypt’s semantics.
| if !(kdr_index == -1 || (0 <= kdr_index && (kdr_index as usize) <= idx.len() * 8)) { | |
| // The kdr_index value must be either -1 or the number of bits that | |
| // will be read from the idx slice. | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| const SRTP_INDEX_LEN: usize = 6; | |
| const MAX_KDR_INDEX: i32 = 24; | |
| if !(-1..=MAX_KDR_INDEX).contains(&kdr_index) { | |
| // wolfCrypt accepts kdr_index values in the range -1..=24. | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| if kdr_index >= 0 { | |
| let required_idx_len = SRTP_INDEX_LEN - ((kdr_index as usize) >> 3); | |
| // wolfCrypt treats idx as a fixed-width SRTP index and may read | |
| // SRTP_INDEX_LEN - (kdr_index >> 3) bytes from it. | |
| if idx.len() < required_idx_len { | |
| return Err(sys::wolfCrypt_ErrorCodes_BAD_FUNC_ARG); | |
| } | |
| } |
Description
This branch fixes all Fenrir findings against the Rust crate.
It also adds zeroize functionality to zeroize all structs on drop.
Testing
Unit/CI tests.
Checklist