Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #425 +/- ##
========================================
Coverage 85.81% 85.81%
========================================
Files 18 18
Lines 2812 2812
Branches 426 427 +1
========================================
Hits 2413 2413
Misses 388 388
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1254712 to
1c3d4bd
Compare
1c3d4bd to
7a7557c
Compare
| explicit_bzero(secnonce->k_1, sizeof(secnonce->k_1)); | ||
| explicit_bzero(secnonce->k_2, sizeof(secnonce->k_2)); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
This isn't really a vulnerability because in case of musig2 signing failures, the signing is invalidated and it has to start over from Round 1. So we'd be leaving in memory a 'secret' that is just a randomly generated number that is no longer useful for anything - a new attempt at signing will generate a new random number.
However, I guess the hardening doesn't hurt.
In that case, however, we should therefore also do the same in the callers of musig_nonce_gen:
produce_and_yield_pubnoncedoesn't need the secnonce at all, so it can delete it straight awaysign_sighash_musig_and_yieldshould delete it once done (both on success and error cases).
There was a problem hiding this comment.
- Added immediate
secnoncezeroing out inproduce_and_yield_pubnonce() - In
sign_sighash_musig_and_yield()if I'm reading correctly that was already the case:secnonceiz zeroed out in case ofmusig_nonce_gen()error- it is zeroed out right after the latest use as well
9f0e1cb to
722aa6a
Compare
fce67e2 to
9204806
Compare
722aa6a to
5c782c1
Compare
5c782c1 to
8ff2b62
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens PSBT signing and MuSig2 flows by improving negative-return checks from merkleized map accessors, tightening preimage streaming validation, and ensuring nonce-related secrets are explicitly zeroed on failure/cleanup paths.
Changes:
- Replace
== -1checks with< 0for functions that can return multiple negative error codes. - Add explicit zeroization/cleanup paths for MuSig2 nonce generation/signing intermediates.
- Reject
partial_data_len == 0incall_stream_preimage()to avoid underflow/invalid buffer creation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/musig/musig.c |
Adds failure/cleanup labels to ensure nonce scalars and derived temporaries are explicitly wiped before returning. |
src/handler/sign_psbt/txhashes.c |
Uses < 0 when validating call_get_merkleized_map_value() result to properly catch all negative error returns. |
src/handler/sign_psbt/musig_signing.c |
Zeroes rand_i_j and secnonce buffers on all paths; refactors nonce-gen error handling to a cleanup pattern. |
src/handler/sign_psbt.c |
Updates merkleized map value length checks from == -1 to < 0 for correctness across error codes. |
src/handler/lib/stream_preimage.c |
Treats partial_data_len == 0 as invalid to prevent partial_data_len - 1 underflow when skipping the 0x00 prefix. |
src/handler/lib/policy.c |
Updates negative error handling for key info retrieval to use < 0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI-generated unit tests with a small added value, but with a lot of code lines have been removed finally.