Fix ML-DSA key cache size check for verify-only builds#452
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an ML-DSA server key-cache sizing check to accommodate WOLFSSL_DILITHIUM_VERIFY_ONLY builds where only the public key DER needs to be cached, avoiding unnecessary compile-time failures in verify-only configurations.
Changes:
- Adds a configuration-dependent ML-DSA maximum cache DER size selection in
wh_server_crypto.c. - Updates the key-cache static assert logic for verify-only vs full-keypair builds.
Comments suppressed due to low confidence (1)
src/wh_server_crypto.c:787
- In
WOLFSSL_DILITHIUM_VERIFY_ONLYbuilds the static assert now allowsWOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZEto be sized for only the public-key DER, butwh_Server_MlDsaKeyCacheImport()still hard-codesMLDSA_MAX_BOTH_KEY_DER_SIZEwhen acquiring a cache slot and serializing. That can make verify-only builds pass the compile-time check but fail key cache import at runtime due to the larger requested size. UseWH_SERVER_MLDSA_MAX_CACHE_DER_SIZEfor the cache-slot check and serialization sizing.
#endif
int wh_Server_MlDsaKeyCacheImport(whServerContext* ctx, wc_MlDsaKey* key,
whKeyId keyId, whNvmFlags flags,
uint16_t label_len, uint8_t* label)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
padelsbach
left a comment
There was a problem hiding this comment.
Looks good, but can you please add simple test cases to build-and-test.yml and build-and-test-refactor.yml
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #452
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
Medium (1)
wh_Server_MlDsaKeyCacheImport hardcodes MLDSA_MAX_BOTH_KEY_DER_SIZE after static assert is relaxed
File: src/wh_server_crypto.c:800
Function: wh_Server_MlDsaKeyCacheImport
Category: Logic errors
wh_Server_MlDsaKeyCacheImport() still requests MLDSA_MAX_BOTH_KEY_DER_SIZE from wh_Server_KeystoreGetCacheSlotChecked(), but the PR now allows WOLFHSM_CFG_SERVER_KEYCACHE_BIG_BUFSIZE >= MLDSA_MAX_PUB_KEY_DER_SIZE in verify-only mode, so key import fails at runtime in any minimally-configured verify-only build.
Recommendation: Replace both MLDSA_MAX_BOTH_KEY_DER_SIZE occurrences in wh_Server_MlDsaKeyCacheImport() with WH_SERVER_MLDSA_MAX_CACHE_DER_SIZE.
Referenced code: src/wh_server_crypto.c:800-804 (5 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
Agree with the Fenrir finding. Please correct uses of |
Fix fenrir comments
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #452
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
|
@miyazakh there was a github outage earlier which caused test failures. Can you push again to re-trigger CI? |
|
@padelsbach |
|
LGTM, will give @bigbrett a chance for a final review. |
bigbrett
left a comment
There was a problem hiding this comment.
overall looks good, I think one of the test cases should be removed as it is overkill.
| # Build and test with ML-DSA verify-only and a reduced keycache buffer. | ||
| # 4096 < MLDSA_MAX_BOTH_KEY_DER_SIZE(7520) so this would fail the static | ||
| # assert without MLDSA_VERIFY_ONLY; it passes because verify-only only | ||
| # needs MLDSA_MAX_PUB_KEY_DER_SIZE(2614). LMS/XMSS full keys fit in 4096. | ||
| - name: Build and test DMA ASAN MLDSA verify-only reduced keycache | ||
| run: cd test && make clean && make -j DMA=1 ASAN=1 MLDSA_VERIFY_ONLY=1 KEYCACHE_BIG_BUFSIZE=4096 WOLFSSL_DIR=../wolfssl && make run |
There was a problem hiding this comment.
I appreciate the spirit of adding test cases for both of these but IMO its pretty overkill to add two separate builds and expose two internal parameters just to essentially verify at compile time.I'd prefer to skip this second one that verifies the static assert hits just for the sake of conserving CI time.IMO something like this is better suited for a nightly (which we don't have yet.
Summary
When
WOLFSSL_DILITHIUM_VERIFY_ONLYis defined, the server only needs tocache the public key DER, not the full keypair. The previous static assert
unconditionally required the cache buffer to fit a full keypair DER, causing
unnecessary failures in verify-only configurations.
This change introduces
WH_SERVER_MLDSA_MAX_CACHE_DER_SIZEto select thecorrect maximum size at compile time:
WOLFSSL_DILITHIUM_VERIFY_ONLYdefined:MLDSA_MAX_PUB_KEY_DER_SIZEMLDSA_MAX_BOTH_KEY_DER_SIZEThe static assert is updated to use this macro so it reflects the actual
requirement for each build configuration.
Testing
WOLFSSL_DILITHIUM_VERIFY_ONLYand confirm thestatic assert passes in both cases.