fix: verify ECDSA signatures from a local buffer before publishing to caller output

[SashaMIT]

Summary

In plain terms: the batch ECDSA signing function handed signatures to the caller before checking them itself. If the check then failed, the caller's buffer was left holding a mix of good signatures and one unchecked one — even though the function reported an error.

Concretely, ecdsa_2p::sign_batch_impl did:

sigs[i] = sig.to_der();          // write into the caller's output vector
// verify
if (rv = ecc_verification_key.verify(msgs[i], sigs[i])) return ...;

A malicious counterparty (P2) that crafts a bad ciphertext mid-batch triggers the verification failure at index i — at which point the caller's sigs vector holds valid signatures at 0..i-1 plus one unverified signature at i. The API returns an error, but any caller that (incorrectly but plausibly) inspects partial results on failure consumes an unverified signature. ecdsa_mp::sign had the same write-then-verify ordering into its output parameter.

The library already does this correctly elsewhere: schnorr_2p builds candidate_sigs, verifies each one, and commits to the caller's vector with a single std::move after all checks pass; the single-sign ecdsa_2p::sign() wrapper is safe for the same reason (it batches into a local and copies out only on success).

Fix

Both ECDSA paths now mirror the candidate-then-commit pattern: serialize DER to a local buffer, verify, then move into the caller's output. No behavior change on the success path.

Test plan

• Full unit suite: 1426/1426 pass (custom OpenSSL 3.6.1 build per README)
• Success-path behavior unchanged (signatures are bit-identical; only the publish point moved)

Made with Cursor

Made with Cursor

[cb-heimdall]

🟡 Heimdall Review Status

Requirement	Status	More Info
Reviews	🟡 0/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

[Rob1Ham]

Thanks for this fix — verifying from a local buffer before publishing into
caller state is the right direction, and the success path is indeed unchanged.
Two line comments inline on the commit mechanics; three general notes below.

1. Contract documentation. #126 added a @notes line to schnorr_2p.h /
schnorr_mp.h ("sigs is cleared before execution and populated only if
every signature verifies successfully."). Suggest the same note on the batch
entry points in include-internal/cbmpc/internal/protocol/ecdsa_2p.h so
callers know what state to expect on error.

2. Regression test. A test that fills sigs with a successful batch
call, then triggers a failure (e.g. a tampered round-2 or round-4 message)
and asserts sigs is empty would pin the contract. With the current patch
that test still fails (see the inline note on clear-on-entry); with the full
#126 shape it passes.

3. Minor narrative nit (reachability framing). The PR description says a
bad ciphertext mid-batch triggers the verification failure at index i. In
default mode the round-4 zk_ecdsa proof is verified at
src/cbmpc/protocol/ecdsa_2p.cpp:380 (inside the !global_abort_mode guard
opened at :372), before signature assembly, so a tampered ciphertext fails
there first; the signature-verify failure at the patched line is
realistically reachable via the internal global-abort mode
(sign_with_global_abort{,_batch}). This doesn't affect the validity of the
fix — it just calibrates the reachability story for the changelog.

None of this blocks the direction — happy to re-review an updated diff, or
these could be tracked as a follow-up if you'd rather land the
unverified-write fix on its own.

---

This feedback was drafted with AI assistance (Kimi k3); all claims verified
against the public repository.

[Rob1Ham]

Two related observations on the publish-into-sigs mechanics:

sigs is still never cleared on entry, so the stale-buffer path remains.
sign_batch_impl still does sigs.resize(n_sigs) at entry (:249).
resize is a no-op on a recycled vector of the same size, so any failure
before or during this loop — e.g. the round-2/round-3 proof checks at
:301/:321, or the round-4 zk_ecdsa verify at :380 — returns with
whatever the caller's buffer already contained. If a caller recycles a buffer
across batches, those stale entries can be valid signatures from a previous
call (over different messages), which would pass a downstream signature
check — arguably a sneakier failure mode than the unverified-bytes case this
PR fixes. After this patch the failure index itself also moves into the
stale/empty category: on a verify failure at index i, sigs[i] no longer
holds the unverified DER (good), but it holds whatever the recycled buffer
had there, and sigs[i+1..] likewise. #126 handled this with sigs.clear()
at entry.

Incremental commit vs. atomic commit. Moving each verified DER into
sigs[i] inside the loop means a mid-batch failure still leaves verified
partial output (sigs[0..i-1]) in caller-visible state on error return.
#126's stated contract was stronger: "On any failure sigs is left cleared
instead of holding partial or unverified signatures" — it builds a local
candidate_sigs and commits with a single sigs = std::move(candidate_sigs)
after all checks pass. Mirroring that here is only a few lines:

sigs.clear();  // at entry, instead of sigs.resize(n_sigs)
...
std::vector<buf_t> candidate_sigs(n_sigs);
// inside the P1 output loop:
candidate_sigs[i] = sig.to_der();
crypto::ecc_pub_key_t ecc_verification_key(key.Q);
if (rv = ecc_verification_key.verify(msgs[i], candidate_sigs[i])) {
  // existing global_abort_mode / default error returns
}
...
sigs = std::move(candidate_sigs);  // once, after the loop

[Rob1Ham]

The local-buffer fix is correct for the unverified write. The same
clear-on-entry consideration applies to the single sig output on failure —
a recycled caller buffer keeps its stale bytes — and a matching header note
on sign would document the contract.

[SashaMIT]

Thanks @Rob1Ham. Applied the clear-on-entry + candidate-then-commit shape for ecdsa_2p::sign_batch_impl (and clear-on-entry for the single-sig wrappers / ecdsa_mp::sign), plus the @notes contract on the headers. Agree the mid-batch verify failure is mainly the global-abort path in default mode; I can tighten the PR body reachability wording if useful.

I have not added the recycled-buffer regression test yet (needs the existing harness wiring). Happy to add that next if you want it in this PR rather than a follow-up.

[hsiuhsiu]

Thank you for bringing this issue to our attention and taking the time to submit this PR. We agree with the issue is worth hardening!

To address the underlying issue more thoroughly across the project, we've put together a broader fix in PR #131. In line with our contributing guidelines, we wanted to keep you in the loop regarding how we're handling this fix.

Thanks again for your contribution and for helping improve the project!

[SashaMIT]

Thanks @hsiuhsiu. Glad this is worth hardening project-wide. #131 looks like the right landing path (clear-on-entry + candidate-then-commit across the sign surfaces, plus the regression coverage). Happy for that to supersede this PR.

If you close #129 in favor of #131, a short thanks note or credit line pointing at this report would be appreciated. Either way, thanks for keeping me in the loop.

[hsiuhsiu]

I'm closing this PR as we have a generalization in #131. Let's continue the discussion there if anything pops up. Thanks!