ellswift: fix overflow flag handling in secp256k1_ellswift_xdh
What the developer wrote
Authored by gzJx0DuTRHytnHe7P5RmMbPf3wKy2BztweVGXTf
ellswift: fix overflow flag handling in secp256k1_ellswift_xdh
The secp256k1_ellswift_xdh function uses overflow = secp256k1_scalar_is_zero(&s) which overwrites the overflow flag from the preceding secp256k1_scalar_set_b32 call. This means secret keys >= the curve order are silently accepted (reduced mod n) instead of being rejected.
The fix changes = to |=, matching the correct pattern already used in secp256k1_ecdh (main_impl.h, line 51).
The ECDH module's test suite explicitly tests overflow rejection (passes secp256k1_group_order_bytes as a key and checks the function returns 0). The ellswift test suite has no corresponding test, which is why this went undetected.
What changed, and why it matters
This commit fixes a bug in the ElligatorSwift-based ECDH key exchange function. The function was supposed to reject secret keys that are invalid (zero or greater than or equal to the curve order), but due to a one-character bug it only checked for zero keys and silently accepted too-large keys by reducing them modulo the curve order. The fix makes it reject both invalid cases, matching the behavior of the regular ECDH function. There is no evidence this was exploited or that it caused real-world harm, but it is a genuine cryptographic correctness issue.
Apply the patch and add a test case to the ellswift test suite that verifies rejection of secp256k1_group_order_bytes as a secret key, mirroring the ECDH module's overflow-rejection test. Review any downstream protocols that may have relied on strict rejection behavior.
Security signals we found
Cryptographic secret-key validation bypass
Silent modular reduction instead of rejection for out-of-range secrets
Inconsistency with documented/referenced ECDH overflow handling
Missing test coverage for overflow rejection in ellswift module
Evidence from the diff
In secp256k1_ellswift_xdh, the overflow flag from secp256k1_scalar_set_b32 (which is set when seckey32 >= n) was being overwritten by the result of secp256k1_scalar_is_zero (which only detects all-zero keys). Changing ‘overflow = …’ to ‘overflow |= …’ preserves the high-order-bit reduction condition and causes the function to return 0 for out-of-range secrets, consistent with secp256k1_ecdh. The fallback path replaces the secret with scalar_one when overflow is true, so the practical effect of the bug was that keys >= n were reduced mod n and used rather than rejected.
Changed components
src/modules/ellswift/main_impl.hsecp256k1_ellswift_xdhInspect captured patch +1 / −1
diff --git a/src/modules/ellswift/main_impl.h b/src/modules/ellswift/main_impl.h
index 096f4a3..4a42566 100644
--- a/src/modules/ellswift/main_impl.h
+++ b/src/modules/ellswift/main_impl.h
@@ -564,7 +564,7 @@ int secp256k1_ellswift_xdh(const secp256k1_context *ctx, unsigned char *output,
/* Load private key (using one if invalid). */
secp256k1_scalar_set_b32(&s, seckey32, &overflow);
- overflow = secp256k1_scalar_is_zero(&s);
+ overflow |= secp256k1_scalar_is_zero(&s);
secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, overflow);
/* Compute shared X coordinate. */
Why this scored 62/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.