Fenrir fixes - #521
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current changes include a security-sensitive scrub length truncation risk in the POSIX DMA callback and multiple test error paths that can leak NVM test artifacts, which can undermine both security intent and test reliability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens wolfHSM’s server-side verification and cryptographic handling by snapshotting verification keys before image verification callbacks, adding explicit memory scrubbing for sensitive materials, tightening edge-case error handling, and expanding regression coverage to prevent false successes/stale responses.
Changes:
- Add bounded key snapshot buffering for image verification, plus explicit zeroization after verification.
- Scrub CMAC key/context state and DMA-transport temporary buffers after use.
- Improve counter-server invalid-action behavior and add regression tests for malformed/invalid requests and key-usage policy enforcement.
File summaries
| File | Description |
|---|---|
wolfhsm/wh_settings.h |
Adds WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE to bound image-manager key snapshots. |
src/wh_server_img_mgr.c |
Copies verify keys into a private buffer under lock and force-zeroes after verification. |
src/wh_server_crypto.c |
Force-zeroes CMAC temporary key material and CMAC context state after operations. |
src/wh_server_counter.c |
Ensures invalid counter actions produce a zero-length response size. |
port/posix/posix_transport_shm.c |
Scrubs temporary DMA buffers before freeing them. |
docs/src/5-Features.md |
Documents key snapshot behavior and new max-key-size configuration. |
docs/src/9-Configuration.md |
Documents WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE. |
test/wh_test_she.c |
Adds a regression test ensuring corrupted M3 is rejected and restored M3 succeeds. |
test/wh_test_server_img_mgr.c |
Adds a key-snapshot regression test to ensure verify callbacks see stable key material. |
test/wh_test_crypto.c |
Adds usage-policy regression tests (ECC verify without VERIFY, X25519 derive denial, RSA fallback behavior). |
test/wh_test_clientserver.c |
Adds regression coverage for invalid counter actions returning zero-length responses. |
test/wh_test_cert.c |
Adds regression coverage for malformed undersized ACERT_DMA requests not reporting false success. |
Review details
Suppressed comments (5)
port/posix/posix_transport_shm.c:678
- Same truncation issue as above: casting
lentouint32_tcan result in only partially scrubbing a large temp buffer prior to freeing it.
uint8_t* ptr = (uint8_t*)dmaPtr + (uintptr_t)*xformedCliAddr;
memcpy((void*)clientAddr, ptr,
len); /* copy results of what server wrote */
/* Scrub key material before freeing. len is bounded by the temp
* buffer's XMALLOC, well within uint32_t for this transport. */
wh_Utils_ForceZero(ptr, (uint32_t)len);
XFREE(ptr, heap, DYNAMIC_TYPE_TMP_BUFFER);
test/wh_test_server_img_mgr.c:1683
- If
wh_Server_ImgMgrInit()fails, the snapshot signature object remains in NVM because the error path cleans up the server but never deletes the test object, which can affect later tests.
ret = wh_Server_ImgMgrInit(&imgMgr, &imgMgrConfig);
if (ret != WH_ERROR_OK) {
WH_ERROR_PRINT("Failed to initialize image manager: %d\n", ret);
wh_Server_Cleanup(server);
return ret;
}
test/wh_test_server_img_mgr.c:1697
- If caching the snapshot key fails, the test returns after cleaning up the server but never deletes the snapshot signature object it created in NVM, leaving stray state behind.
ret =
wh_Server_KeystoreCacheKey(server, &keyMeta, (uint8_t*)testSnapshotKey);
if (ret != WH_ERROR_OK) {
WH_ERROR_PRINT("Failed to cache snapshot key: %d\n", ret);
wh_Server_Cleanup(server);
return ret;
}
test/wh_test_server_img_mgr.c:1704
- If image verification fails, this error path cleans up the server but does not delete the snapshot signature object that was added to NVM at the start of the test, which can leak test artifacts into later runs.
ret = wh_Server_ImgMgrVerifyImg(&imgMgr, &testImage, &result);
if (ret != WH_ERROR_OK) {
WH_ERROR_PRINT("Snapshot image verification failed: %d\n", ret);
wh_Server_Cleanup(server);
return ret;
}
test/wh_test_server_img_mgr.c:1711
- If the verify method itself fails, the server is cleaned up but the snapshot signature object is not deleted from NVM, leaving behind test data and potentially interfering with subsequent tests.
if (result.verifyMethodResult != WH_ERROR_OK) {
WH_ERROR_PRINT("Snapshot verify method failed: %d\n",
result.verifyMethodResult);
wh_Server_Cleanup(server);
return result.verifyMethodResult;
}
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| uint8_t* ptr = (uint8_t*)dmaPtr + (uintptr_t)*xformedCliAddr; | ||
| /* Scrub key material before freeing. len is bounded by the temp | ||
| * buffer's XMALLOC, well within uint32_t for this transport. */ | ||
| wh_Utils_ForceZero(ptr, (uint32_t)len); | ||
| XFREE(ptr, heap, DYNAMIC_TYPE_TMP_BUFFER); |
| ret = wh_Server_Init(server, serverCfg); | ||
| if (ret != WH_ERROR_OK) { | ||
| WH_ERROR_PRINT("Failed to initialize server: %d\n", ret); | ||
| return ret; | ||
| } |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #521
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| /* Image manager maximum verification key size. Sized to hold an ASN.1 RSA4096 | ||
| * public key. Raise it for larger keys such as ML-DSA. */ | ||
| #ifndef WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE | ||
| #define WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE 1200 |
There was a problem hiding this comment.
Image-manager key snapshot bound is smaller than the keystore's big-cache buffer in PQC builds · Cryptographic operation flaws
WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE is a fixed 1200 while WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE is 8192 when MLDSA/MLKEM/LMS/XMSS are enabled. _ImgMgrCopyKeyFromKeystore then gets WH_ERROR_NOSPACE from wh_Server_KeystoreReadKey for keys the keystore accepts, and wh_Server_ImgMgrVerifyImg returns before invoking either verifyMethod or verifyAction. Previously wh_Server_KeystoreFreshenKey handled any cached key size.
Related known finding #6200 (similar but distinct): Both concern an inconsistent wh_settings.h size default, but #6200 is a documentation-versus-code mismatch for keywrap buffers; this is an operational capacity mismatch between image-manager snapshot storage and the keystore cache. The root cause, affected path, and required patch differ.
Fix: Default WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE to WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE so it tracks the keystore's actual key capacity.
This pull request introduces several important security and robustness improvements to the image manager, cryptography, and transport layers, as well as updates to documentation and regression tests. The changes focus on better key material handling, explicit memory zeroization, and improved error handling in edge cases.
Security and Key Handling Improvements:
The image manager now copies verification keys from the keystore into a private buffer before verification, ensuring that verification operates on a stable snapshot and not a live cache slot. The buffer size is bounded by the new
WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZEconfiguration, and the buffer is explicitly zeroed after use to prevent key leakage. (src/wh_server_img_mgr.c[1] [2] [3] [4] [5];docs/src/5-Features.md[6];docs/src/9-Configuration.md[7]Added explicit zeroization of sensitive key material and cryptographic contexts after use in both CMAC operations and DMA transport callbacks, reducing the risk of sensitive data lingering in memory. (
src/wh_server_crypto.c[1] [2];port/posix/posix_transport_shm.c[3] [4]Robustness and Error Handling:
src/wh_server_counter.csrc/wh_server_counter.cR228)Testing and Regression Coverage:
Added regression tests to ensure malformed ACERT_DMA requests and invalid counter actions are correctly handled and do not result in false successes or stale data. (
test/wh_test_cert.c[1];test/wh_test_clientserver.c[2]Added a test to verify that ECDSA verification is properly denied when the key does not have the VERIFY usage flag, ensuring correct enforcement of key usage policies. (
test/wh_test_crypto.ctest/wh_test_crypto.cR17148-R17201)Documentation Updates:
docs/src/5-Features.md[1];docs/src/9-Configuration.md[2]Key Handling and Security:
WOLFHSM_CFG_SERVER_IMG_MGR_MAX_KEY_SIZE, and zeroes the buffer after use. This prevents race conditions and key leakage. [1] [2] [3] [4] [5] [6] [7]Robustness and Error Handling:
Testing and Regression:
Documentation: