Merge bitcoin-core/secp256k1#1948: tests: add ECDSA verify case where r + n overflows p
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1948: tests: add ECDSA verify case where r + n overflows p
fc88acc3e2e2cbba7ee202634eb73d5d21694126 tests: add ECDSA verify case where r + n overflows p (ViniciusCestarii)
Pull request description:
Currently no test checks that ECDSA verify rejects a signature where r + n ≥ p and (r + n) mod p equals x(R), so the following mutant lives:
```diff
diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h
index 5963877..5301eaa 100644
--- a/src/ecdsa_impl.h
+++ b/src/ecdsa_impl.h
@@ -258,10 +258,6 @@ static int secp256k1_ecdsa_sig_verify(const secp256k1_scalar *sigr, const secp25
/* xr * pr.z^2 mod p == pr.x, so the signature is valid. */
return 1;
}
- if (secp256k1_fe_cmp_var(&xr, &secp256k1_ecdsa_const_p_minus_order) >= 0) {
- /* xr + n >= p, so we can skip testing the second case. */
- return 0;
- }
secp256k1_fe_add(&xr, &secp256k1_ecdsa_const_order_as_fe);
if (secp256k1_gej_eq_x_var(&xr, &pr)) {
/* (xr + n) * pr.z^2 mod p == pr.x, so the signature is valid. */
```
This adds a case with r = p - n + 1, so that (r + n) mod p = 1, and a pubkey chosen so that x(R) = 1, covering this gap.
ACKs for top commit:
real-or-random:
utACK fc88acc3e2e2cbba7ee202634eb73d5d21694126
theStack:
re-ACK fc88acc3e2e2cbba7ee202634eb73d5d21694126
Tree-SHA512: b9ab5ff0e7312e3ff8b5667eed56f616f7fc59ad8d1777fd54d70fcb3342b5084dc8915c0638a6486fdfe4522da5905521dd32622a64d5a2b5d0a3385e381098
What changed, and why it matters
This commit only adds a new test case to the project's test suite. It does not change any production cryptographic code. The new test verifies that the ECDSA signature verification function correctly rejects a crafted invalid signature in a rare edge case where a mathematical overflow condition occurs. It closes a testing gap that could otherwise allow a subtle code mutation to go undetected, but it is not a security fix by itself.
No immediate action required. Treat as a normal test-coverage improvement. Reviewers may optionally verify that the new test vector is correct and that existing CI runs pass with the added case.
Security signals we found
Adds test coverage for ECDSA verification edge case involving r + n overflow against field order p
References a surviving mutant where the early-rejection check in secp256k1_ecdsa_sig_verify is removed
Does not modify production code; purely a test-suite enhancement
Evidence from the diff
The commit adds one test block in src/tests.c under run_ecdsa_edge_cases(). It constructs an ECDSA signature with r = p - n + 1 (where p is the field order and n is the curve order), s = 1, message = 0, and a public key chosen so that x(R) = 1. In this case (r + n) mod p equals x(R), which could theoretically match the second verification branch if the implementation’s early-rejection check for r + n ≥ p were removed. The test asserts that secp256k1_ecdsa_sig_verify returns 0, confirming the existing guard rejects it. No implementation code in src/ecdsa_impl.h or elsewhere is modified.
Changed components
src/tests.cECDSA verification test suiteInspect captured patch +28 / −0
### src/tests.c
@@ -7729,6 +7729,34 @@ static void run_ecdsa_edge_cases(void) {
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 0);
}
+ /* Verify signature where r + n overflows p fails. */
+ {
+ /* Scalar r as chars: r = p - n + 1, so that (r + n) mod p = 1 */
+ const unsigned char csr[32] = {
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+ 0x45, 0x51, 0x23, 0x19, 0x50, 0xb7, 0x5f, 0xc4,
+ 0x40, 0x2d, 0xa1, 0x72, 0x2f, 0xc9, 0xba, 0xef
+ };
+ /* With s = 1 and msg = 0, verification computes R = r * pubkey.
+ * pubkey = r^-1 * (1, y), so x(R) = 1. */
+ const unsigned char pubkey[33] = {
+ 0x02, 0x57, 0xad, 0x61, 0xc8, 0x68, 0x3f, 0xcf,
+ 0x06, 0x99, 0x19, 0x11, 0x8c, 0x0f, 0x99, 0xb9,
+ 0x38, 0x9f, 0x65, 0x05, 0x9b, 0xa0, 0x71, 0xba,
+ 0xbe, 0xa6, 0x32, 0x05, 0x34, 0x14, 0x45, 0xda,
+ 0xe8
+ };
+ secp256k1_ge key;
+ secp256k1_scalar msg;
+ secp256k1_scalar sr, ss;
+ secp256k1_scalar_set_int(&ss, 1);
+ secp256k1_scalar_set_int(&msg, 0);
+ secp256k1_scalar_set_b32(&sr, csr, NULL);
+ CHECK(secp256k1_ge_parse33(&key, pubkey));
+ CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 0);
+ }
+
/* Signature where s would be zero. */
{
secp256k1_pubkey pubkey;Why this scored 12/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.