Repository navigation
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation safely aligns API behavior, callback handling, tests, and documentation without unresolved issues.
Review effort: Balanced
Findings: None
What changed in this PR
Enables SLH-DSA randomized and deterministic signing APIs to accept empty messages as (NULL, 0), aligning them with existing sign/verify behavior.
Changes:
- Permits and safely canonicalizes
(NULL, 0)messages before callback dispatch. - Adds round-trip and argument-validation coverage.
- Updates public and source documentation.
| File | Description |
|---|---|
wolfcrypt/src/wc_slhdsa.c |
Updates validation and empty-message handling. |
wolfcrypt/test/test.c |
Adds deterministic empty-message round-trip coverage. |
tests/api/test_slhdsa.c |
Tests argument-check behavior for (NULL, 0). |
doc/dox_comments/header_files/wc_slhdsa.h |
Documents accepted empty-message semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi @Arpan0995, thanks for the contribution, looks good to me. I see you have a contributor agreement pending. Once that goes through we can proceed with this PR. |
|
retest this please |
|
Hi @padelsbach My contributor agreement has been approved, could you please review this PR now? |
wc_SlhDsaKey_SignWithRandom() rejected msg == NULL for any length, so it and wc_SlhDsaKey_SignDeterministic() returned BAD_FUNC_ARG for an empty message passed as (NULL, 0), unlike wc_SlhDsaKey_Sign() and Verify(). Use the same check as Sign() and, like Sign(), replace a NULL message with a one-byte static stand-in before the crypto callback dispatch. Update the doxygen and source comments, and add tests for the (NULL, 0) case.
ff96497 to
8de5fcf
Compare
|
Thanks @Arpan0995 for the update. The approval process is still propagating through our org, so I cannot merge it quite yet, but I'll review it this week. |
| * On out, length of signature data. | ||
| * @return 0 on success. | ||
| * @return BAD_FUNC_ARG when key, key's parameters, msg or sig is NULL. | ||
| * @return BAD_FUNC_ARG when key, key's parameters, sig or sigSz is NULL. |
There was a problem hiding this comment.
Please update this comment, also wc_SlhDsaKey_Sign and wc_SlhDsaKey_Verify regarding msg being NULL
| \return BAD_FUNC_ARG if key, msg, sig, or sigSz is NULL. | ||
| \return BAD_FUNC_ARG if key, sig, or sigSz is NULL, or msg is NULL but | ||
| msgSz is greater than 0. | ||
| \return BUFFER_E if the output buffer is too small. |
There was a problem hiding this comment.
I think it should be BAD_LENGTH_E if the buffer is too small
There was a problem hiding this comment.
Agreed, changed to BAD_LENGTH_E, which is what SignWithRandom() returns when sigSz is too small. I also added the same "may be NULL if msgSz is 0" note to the msg parameter in the four doxygen blocks.
| * wc_SlhDsaKey_SignWithRandom(). */ | ||
| sigLen = WC_SLHDSA_MAX_SIG_LEN; | ||
| PRIVATE_KEY_UNLOCK(); | ||
| ret = wc_SlhDsaKey_SignDeterministic(key, NULL, 0, NULL, 0, sig, |
There was a problem hiding this comment.
Can you add code to compare SignDeterministic(NULL, 0) with SignDeterministic(msg, 0)?
There was a problem hiding this comment.
Added. In the SHAKE128F block the test now signs again with SignDeterministic() using a non-NULL message of length 0 and checks that the length and bytes match the (NULL, 0) signature. The second buffer is only declared when 128F is enabled.
| WC_NO_ERR_TRACE(BAD_FUNC_ARG)); /* msg==NULL */ | ||
| tinySigSz = 1; | ||
| ExpectIntEQ(wc_SlhDsaKey_SignWithRandom(&key, dummyMsg, 0, NULL, | ||
| 0, dummySig, &tinySigSz, dummyAddRnd), |
There was a problem hiding this comment.
For robustness, can you use a valid sig size instead of tinySigSz?
There was a problem hiding this comment.
Changed to a full-size signature buffer, so the (NULL, 0) call now signs and the test checks that it returns 0 with the expected signature length.
- Source comments: msg may be NULL if msgSz is 0 for the sign and verify functions, and Sign(), Verify() and slhdsakey_sign_external() reject a NULL msg only when msgSz is greater than 0. - Doxygen: note on the msg parameter, and SignDeterministic() returns BAD_LENGTH_E for a too-small signature buffer. - test.c: SignDeterministic() with (NULL, 0) must match the signature for a non-NULL message of length 0. - test_slhdsa.c: SignWithRandom() with (NULL, 0) and a full-size signature buffer signs successfully.
Description
Commit 916de9c (#11381) made the sign and verify entry points accept an empty message passed as (NULL, 0). #11055, merged after it, added crypto callback support to wc_SlhDsaKey_SignWithRandom() together with an argument check of its own, and that check rejects msg == NULL for any length. As a result, wc_SlhDsaKey_SignWithRandom() and wc_SlhDsaKey_SignDeterministic(), which calls it, return BAD_FUNC_ARG for (NULL, 0), while wc_SlhDsaKey_Sign() and wc_SlhDsaKey_Verify() accept it. Both commits are in v5.9.4-stable.
This uses the same check as wc_SlhDsaKey_Sign(), so a NULL msg is rejected only when msgSz is not 0. As in Sign() and Verify(), a NULL message is then replaced with a one-byte static stand-in before the crypto callback dispatch, so devices and the hash code never see a NULL pointer. The stand-in is a one-element array to match the Coverity ARRAY_VS_SINGLETON change in #11457. The doxygen for SignDeterministic, SignWithRandom, Sign and Verify, and the source comments for the first two, are updated to match.
Testing
and the same with:
Without the wc_slhdsa.c change, both new checks fail with BAD_FUNC_ARG. With it, testwolfcrypt (including the crypto callback test) and the slhdsa API group pass in both builds. Tested on macOS (arm64) with Apple clang.
Checklist