fix: clear signature outputs before signing and populate only after verification

[hsiuhsiu]

This PR is a generalization of #129. Thanks @SashaMIT for raising this!

Ensure ECDSA and Schnorr (2P and MP) signing paths never leave stale or unverified signature material in the caller-provided output buffers.

• Clear sig/sigs outputs at the start of each sign entry point.
• Compute signatures into local candidate buffers and only move them into the caller's output after the final self-verification against the public key succeeds, so failure paths return no signature bytes.
• Document the guarantee in the internal protocol headers.
• Add unit tests covering output clearing on failure and reuse across calls.

[cb-heimdall]

✅ Heimdall Review Status

Requirement	Status	More Info
Reviews	✅ 2/2	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 2 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 1 Global minimum 0 Max 2 2 1 if commit is unverified 0 Sum 2
CODEOWNERS	✅ None for this change

[SashaMIT]

Thanks @hsiuhsiu for the generalization in this PR, and for the credit note pointing at #129. Happy to help review or answer questions on the ECDSA path if useful.

[Rob1Ham]

Review: PR #131 — clear signature outputs before signing and populate only after verification

Context: Generalization of PR #129 to all ECDSA and Schnorr signing paths (2P and MP).

Overall assessment: This is a clean, correct, and well-tested generalization. The pattern — clear output at entry, compute into local buffer, verify before publishing — is the right approach and eliminates the stale-output class of issues across the entire signing surface.

What looks good

• Uniform coverage: All 6 signing entry points (ECDSA 2P/MP, Schnorr 2P/MP, EdDSA 2P/MP) get the same treatment. No gaps.
• Defense in depth: sig.free() moved to the top of each function, before validation. Previously it was after validation — meaning invalid inputs now clear the output even before the validation check fires. This is strictly safer.
• Correct idiom in ecdsa_mp.cpp:482: sig = std::move(candidate_sig) after pub.verify(msg, candidate_sig) — compute into local, verify, then move. This is the right pattern.
• Test coverage: test_sign_output.cpp covers failure-path clearing for all 6 APIs. The stale_signature() sentinel ({0x53, 0x49, 0x47} — "SIG") is a nice touch.
• Protocol-level test: VerificationFailureClearsSignatureOutputs in test_ecdsa_2p.cpp correctly verifies that both parties' outputs are empty when the final signature verification fails in the global-abort path.

Observations (non-blocking)

1. API contract change worth documenting

Moving sig.free() to the top means: previously, an invalid input (wrong role, wrong blob) would leave the caller's sig buffer untouched; now it's cleared first. Callers who relied on reading sig even on early validation failure would see empty output. This is the right behavior for a security library, but it's a subtle API contract change worth noting in the changelog or docs.

2. buf_t::free() doesn't zero memory

buf_t::free() releases memory back to the allocator but doesn't appear to zero it first. If a caller previously had a signature in sig and calls sign() again (e.g., in a retry loop), the old signature bytes remain in freed memory. For a security library handling key material, consider whether buf_t should have a secure zeroing destructor or whether callers should use a secure_buf_t type. Pre-existing concern, not introduced by this PR.

3. Batch sign edge case

In sign_batch_impl, if n_sigs == 0, the function returns E_BADARG after sigs.clear() — so the output is correctly cleared. But note that sigs.resize(n_sigs) was removed, so if the caller passed a non-empty vector, it stays non-empty until the end. If any signature fails verification, sigs = std::move(candidate_sigs) never runs and sigs stays empty. This is correct but worth a comment in the header documenting that a failed batch leaves sigs empty (not partially populated).

4. Documentation completeness

The header comment for ecdsa_2p.h documents "On any error, sig_der is cleared." The same guarantee should be documented on all 6 API headers (ecdsa_mp.h, eddsa_2p.h, eddsa_mp.h, schnorr_2p.h, schnorr_mp.h) for consistency. Currently only ecdsa_2p.h has the note.

5. Missing test: batch path

test_sign_output.cpp covers the single-sign paths (all 6 APIs) but not sign_batch/sign_batch_impl. A test verifying that a failed batch (e.g., one signature fails verification) clears the entire sigs vector would complete the coverage.

6. Missing test: reuse across calls

The PR description mentions "reuse across calls" but I don't see a test that calls sign twice on the same buffer — first success, then failure — to verify the second call clears the first's output. The current tests only test the failure path on a fresh buffer.

Summary

The PR correctly generalizes the fix from #129 across all signing paths. The pattern is right, the tests cover the failure paths, and the change is safe. The main things to consider are the API contract change (output cleared on early validation failure), the buf_t::free() zeroing question, and completing the test coverage for batch paths and reuse-across-calls.

Happy to help with the EdDSA path or answer questions on the 2P/MP signing flows.

[valery-osheter-cb]

Approved

[SashaMIT]

Thanks for landing this, and for the credit note pointing at #129. Appreciate the generalization.