Rust wrapper: fix unsound HMAC::clone() - #11527
Open
holtrop-wolfssl wants to merge 1 commit into
Open
holtrop-wolfssl wants to merge 1 commit into
holtrop-wolfssl wants to merge 1 commit into
Conversation
wc_HmacCopy() takes src by non-const pointer and may write through it (wc_MAXQ10XX_Sha256Copy(), WOLF_CRYPTO_CB_COPY device callbacks), but HMAC::clone() cast that pointer from a shared reference. Hold the wolfSSL context in an UnsafeCell so the write is sound, and add the fallible HMAC::copy() alongside it, mirroring SHA256::copy()/SHA384::copy(). Fixes F-8285.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (3)
!Sync, so no other thread can be touching the context at the same time” is a bit… · Newcopy()vs… · New
What changed in this PR
Fixes unsoundness in the Rust HMAC wrapper’s Clone implementation by making wc_HmacCopy()’s potentially-mutating source pointer sound, and adds a fallible copy API plus tests.
Changes:
- Store the underlying
sys::HmacinsideUnsafeCellto makewc_HmacCopy()from&selfsound. - Add
HMAC::copy()(fallible) and refactorCloneto use a shared internalcopy_from. - Add tests covering both
copy()andclone(), and update the changelog.
| File | Description |
|---|---|
| wrapper/rust/wolfssl-wolfcrypt/src/hmac.rs | Wraps HMAC context in UnsafeCell, adds fallible copy(), and makes Clone sound |
| wrapper/rust/wolfssl-wolfcrypt/tests/test_hmac.rs | Adds tests validating copy()/clone() behavior and independence of state |
| wrapper/rust/wolfssl-wolfcrypt/CHANGELOG.md | Documents the new API and the UB fix |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// `src` to the device callback as a writable pointer. `Clone` therefore | ||
| /// needs a pointer that may be written to while only holding a `&self`, | ||
| /// which is sound only through `UnsafeCell`. `UnsafeCell` is also `!Sync`, | ||
| /// so no other thread can be touching the context at the same time. |
Comment on lines
+58
to
+61
| #[test] | ||
| fn test_hmac_sha256_copy() { | ||
| let key = b"\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"; | ||
| let expected = b"\xb0\x34\x4c\x61\xd8\xdb\x38\x53\x5c\xa8\xaf\xce\xaf\x0b\xf1\x2b\x88\x1d\xc2\x00\xc9\x83\x3d\xa7\x26\xe9\x37\x6c\x2e\x32\xcf\xf7"; |
Comment on lines
+59
to
+84
| fn test_hmac_sha256_copy() { | ||
| let key = b"\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"; | ||
| let expected = b"\xb0\x34\x4c\x61\xd8\xdb\x38\x53\x5c\xa8\xaf\xce\xaf\x0b\xf1\x2b\x88\x1d\xc2\x00\xc9\x83\x3d\xa7\x26\xe9\x37\x6c\x2e\x32\xcf\xf7"; | ||
|
|
||
| let mut hmac = HMAC::new(HMAC::TYPE_SHA256, key).expect("Error with new()"); | ||
| hmac.update(b"Hi ").expect("Error with update()"); | ||
|
|
||
| let mut forked = hmac.copy().expect("Error with copy()"); | ||
|
|
||
| // The copy continues independently from the same point. | ||
| forked.update(b"There").expect("Error with update()"); | ||
| let mut hash_forked = [0u8; SHA256::DIGEST_SIZE]; | ||
| forked.finalize(&mut hash_forked).expect("Error with finalize()"); | ||
| assert_eq!(hash_forked, *expected); | ||
|
|
||
| // The original is unaffected by the copy. | ||
| hmac.update(b"There").expect("Error with update()"); | ||
| let mut hash_orig = [0u8; SHA256::DIGEST_SIZE]; | ||
| hmac.finalize(&mut hash_orig).expect("Error with finalize()"); | ||
| assert_eq!(hash_orig, *expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_hmac_sha256_clone() { | ||
| let key = b"\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"; | ||
| let expected = b"\xb0\x34\x4c\x61\xd8\xdb\x38\x53\x5c\xa8\xaf\xce\xaf\x0b\xf1\x2b\x88\x1d\xc2\x00\xc9\x83\x3d\xa7\x26\xe9\x37\x6c\x2e\x32\xcf\xf7"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Rust wrapper: fix unsound HMAC::clone()
wc_HmacCopy() takes src by non-const pointer and may write through it (wc_MAXQ10XX_Sha256Copy(), WOLF_CRYPTO_CB_COPY device callbacks), but HMAC::clone() cast that pointer from a shared reference. Hold the wolfSSL context in an UnsafeCell so the write is sound, and add the fallible HMAC::copy() alongside it, mirroring SHA256::copy()/SHA384::copy().
Fixes F-8285.
Testing
How did you test?
Checklist