Preserve in-place message on signing failure#1230
Conversation
Signed-off-by: Fredrik Dahlgren <fredrik.dahlgren@trailofbits.com>
| * MLD_CONFIG_CONTEXT_PARAMETER is defined; type set by | ||
| * MLD_CONFIG_CONTEXT_PARAMETER_TYPE. | ||
| * | ||
| * On failure, *smlen is set to 0. No partial signed-message output is |
There was a problem hiding this comment.
The parameter is called sig here. Can we align this across sign.h, api.h, and sign.c, while at it?
I think sig and siglen are more intuitive than sm/smlen. @mkannwischer wdyt
| uint8_t sig[MLDSA_CRYPTO_BYTES]; | ||
| size_t siglen; | ||
| size_t i; | ||
|
|
||
| ret = mld_sign_signature(sig, &siglen, m, mlen, ctx, ctxlen, sk, context); | ||
| if (ret != 0) | ||
| { | ||
| *smlen = 0; | ||
| goto cleanup; | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm, I'm not too happy with this additional stack allocation (NB: large allocations should come from MLD_ALLOC/FREE, but that is a simple change).
I agree, though, that's unintuitive that the message is corrupted upon error.
Alternative: If signing fails and sm == m (the only interesting case), copy the message from the tail of sm to m. This isn't pretty, but it does the job, because that tail of sm is guaranteed not to be touched.
Leveling up, though: Why do we need this API in the first place? @mkannwischer can you share some perspectives?
hanno-becker
left a comment
hanno-becker
left a comment
There was a problem hiding this comment.
I agree that the function's behavior is not ideal and should, at the very least, be documented if not changed.
However, I'm hesitant about the additional allocation of MLDSA_CRYPTO_BYTES bytes here.
I see three alternative options; @mkannwischer please chime in:
- Just document and warn about the behavior.
- For
sm == m, restore the message upon failure. - Remove the API altogether.
2 participants
Summary
crypto_signfails before producing a signature.Validation
clang-format -i mldsa/src/sign.c mldsa/src/sign.h mldsa/mldsa_native.h test/src/test_mldsa.c test/src/test_rng_fail.c: passedgit diff --check -- mldsa/src/sign.c mldsa/src/sign.h mldsa/mldsa_native.h test/src/test_mldsa.c test/src/test_rng_fail.c: passedmake run_func run_rng_fail -j4: passed./scripts/formatbecausenixpkgs-fmtis not installed in this local environment./scripts/lintbecauseshfmtis not installed in this local environmentFixes #1217
This work was completed by Trail of Bits as part of the Patch The Planet project in collaboration with OpenAI. The issue was identified primarily by the Codex coding agent, and manually reviewed before submission.