← Watch feed
Merge bitcoin-core/secp256k1#1933: refactor: introduce `ecmult_const_ge` helper (preventing accidential gej leaks)
Public commit record
100/100 · Strong
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1933: refactor: introduce `ecmult_const_ge` helper (preventing accidential gej leaks)
02e00f54a0e0ac72c74eb4f5b84855b290bba124 test: refactor: simplify tests by using `_ecmult_const_ge` helper (Sebastian Falbesoner)
b4fbee61e91d30aedbe0e84f407393e0c27c8c55 refactor: rename `_ecmult_const` -> `_ecmult_const_gej` for consistency (Sebastian Falbesoner)
ace566f66c1b75d929c367612abf22944d81fb8e refactor: introduce `_ecmult_const_ge` helper (preventing accidental gej leaks) (Sebastian Falbesoner)
Pull request description:
This PR is the counterpart of #1861 for `_ecmult_const`: constant-time scalar multiplication with arbitrary points frequently involves a conversion to affine coordinates and clearing out the temporary Jacobian group element object after to avoid leaking secret key material, i.e. executing the following three functions:
* `secp256k1_ecmult_const(&rj, ...)`
* `secp256k1_ge_set_gej(&r, &rj)`
* `secp256k1_gej_clear(&rj)`
A helper `ecmult_const_ge` is introduced to deduplicate code and mitigate the risk that the last step is forgotten (which can easily happen, as it would not be detected by tests). It is applied in the ECDH and silentpayments modules.
The idea came up in the course of reviewing the DLEQ module, where a gej clearing was missing, see https://github.com/bitcoin-core/secp256k1/pull/1802#discussion_r3960829657.
ACKs for top commit:
real-or-random:
utACK 02e00f54a0e0ac72c74eb4f5b84855b290bba124
Tree-SHA512: dadfbace89875d50c331170738ebc413516a01d8b05a1e75a3d9c10ade194772633920a3ea0592c4b709cfb78b86a3bc72806e3f06f3ba5567a42e17ae1e2309
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit is a defensive code cleanup in Bitcoin Core's secp256k1 cryptography library. It introduces a helper function that automatically clears temporary secret data after a constant-time elliptic curve multiplication, and switches existing code to use it. The change prevents future mistakes where sensitive intermediate values could be left in memory, but the commit itself does not claim to fix an active vulnerability in the current code.
Recommended action
Treat as a hardening improvement rather than an urgent security patch. Review whether the DLEQ module and any other modules using `secp256k1_ecmult_const_gej` directly still perform proper `gej_clear` cleanup, since the commit explicitly notes that risk and references a prior missing-clear issue. Consider whether the new helper should be used in additional call sites to reduce future accidental leaks.
Security signals we found
01
Defensive refactoring to prevent accidental leakage of secret scalar information from Jacobian coordinates
02
New helper enforces memory clearing of intermediate group element after constant-time scalar multiplication
03
Adoption in ECDH and silentpayments modules, which process secret keys
04
Reference to a prior review finding of a missing gej_clear in a related DLEQ module
05
No direct bug fix or CVE claim in commit message or diff
Evidence from the diff
The commit refactors secp256k1_ecmult_const into secp256k1_ecmult_const_gej and adds a new secp256k1_ecmult_const_ge helper. The helper performs constant-time scalar multiplication, converts the result to affine coordinates, and explicitly clears the temporary Jacobian group element (secp256k1_gej_clear). It is adopted in the ECDH and silentpayments modules, replacing manual sequences of multiply/convert/clear. The stated goal is to reduce the risk of forgetting the gej_clear step, which could leak information about secret scalars through Jacobian coordinate residues. The commit references a prior review comment in PR #1802 where a gej_clear was noted as missing in the DLEQ module, but that module is not modified here.
Changed components
src/ecmult_const.hsrc/ecmult_const_impl.hsrc/modules/ecdh/main_impl.hsrc/modules/silentpayments/main_impl.hsrc/bench_ecmult.csrc/tests.csrc/tests_exhaustive.cInspect captured patch +35 / −42
### src/bench_ecmult.c
@@ -118,7 +118,7 @@ static void bench_ecmult_const(void* arg, int iters) {
int i;
for (i = 0; i < iters; ++i) {
- secp256k1_ecmult_const(&data->output[i], &data->pubkeys[(data->offset1+i) % POINTS], &data->scalars[(data->offset2+i) % POINTS]);
+ secp256k1_ecmult_const_gej(&data->output[i], &data->pubkeys[(data->offset1+i) % POINTS], &data->scalars[(data->offset2+i) % POINTS]);
}
}
### src/ecmult_const.h
@@ -13,10 +13,11 @@
/**
* Multiply: R = q*A (in constant-time for q)
*/
-static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
+static void secp256k1_ecmult_const_gej(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
+static void secp256k1_ecmult_const_ge(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_scalar *q);
/**
- * Same as secp256k1_ecmult_const, but takes in an x coordinate of the base point
+ * Same as secp256k1_ecmult_const_gej, but takes in an x coordinate of the base point
* only, specified as fraction n/d (numerator/denominator). Only the x coordinate of the result is
* returned.
*
### src/ecmult_const_impl.h
@@ -119,7 +119,7 @@ static const secp256k1_scalar secp256k1_ecmult_const_K = SECP256K1_SCALAR_CONST(
# error "Unknown ECMULT_CONST_BITS"
#endif
-static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q) {
+static void secp256k1_ecmult_const_gej(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q) {
/* The approach below combines the signed-digit logic from Mike Hamburg's
* "Fast and compact elliptic-curve cryptography" (https://eprint.iacr.org/2012/309)
* Section 3.3, with the GLV endomorphism.
@@ -265,6 +265,15 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
secp256k1_fe_mul(&r->z, &r->z, &global_z);
}
+SECP256K1_INLINE static void secp256k1_ecmult_const_ge(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_scalar *q) {
+ secp256k1_gej rj;
+ secp256k1_ecmult_const_gej(&rj, a, q);
+ secp256k1_ge_set_gej(r, &rj);
+ /* Jacobian coordinates resulting from our multiplication algorithm could potentially leak
+ * information about the secret input scalar, so clear the memory out to be on the safe side. */
+ secp256k1_gej_clear(&rj);
+}
+
static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n, const secp256k1_fe *d, const secp256k1_scalar *q, int known_on_curve) {
/* This algorithm is a generalization of Peter Dettman's technique for
@@ -384,7 +393,7 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
/* Perform x-only EC multiplication of P with q. */
VERIFY_CHECK(!secp256k1_scalar_is_zero(q));
- secp256k1_ecmult_const(&rj, &p, q);
+ secp256k1_ecmult_const_gej(&rj, &p, q);
VERIFY_CHECK(!secp256k1_gej_is_infinity(&rj));
/* The resulting (X, Y, Z) point on the effective-affine isomorphic curve corresponds to
### src/modules/ecdh/main_impl.h
@@ -34,7 +34,6 @@ const secp256k1_ecdh_hash_function secp256k1_ecdh_hash_function_default = ecdh_h
int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const secp256k1_pubkey *point, const unsigned char *scalar, secp256k1_ecdh_hash_function hashfp, void *data) {
int ret = 0;
int is_sec_valid;
- secp256k1_gej res;
secp256k1_ge pt;
secp256k1_scalar s;
unsigned char x[32];
@@ -49,8 +48,7 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
is_sec_valid = secp256k1_scalar_set_b32_seckey(&s, scalar);
secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, !is_sec_valid);
- secp256k1_ecmult_const(&res, &pt, &s);
- secp256k1_ge_set_gej(&pt, &res);
+ secp256k1_ecmult_const_ge(&pt, &pt, &s);
/* Compute a hash of the point */
secp256k1_fe_normalize(&pt.x);
@@ -69,7 +67,6 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
secp256k1_memclear_explicit(y, sizeof(y));
secp256k1_scalar_clear(&s);
secp256k1_ge_clear(&pt);
- secp256k1_gej_clear(&res);
return (!!ret) & is_sec_valid;
}
### src/modules/silentpayments/main_impl.h
@@ -86,14 +86,12 @@ static int secp256k1_silentpayments_calculate_input_hash_scalar(const secp256k1_
}
static void secp256k1_silentpayments_create_shared_secret(unsigned char *shared_secret33, const secp256k1_ge *public_component, const secp256k1_scalar *secret_component) {
- secp256k1_gej ss_j;
secp256k1_ge ss;
VERIFY_CHECK(!secp256k1_ge_is_infinity(public_component));
VERIFY_CHECK(!secp256k1_scalar_is_zero(secret_component));
- secp256k1_ecmult_const(&ss_j, public_component, secret_component);
- secp256k1_ge_set_gej(&ss, &ss_j);
+ secp256k1_ecmult_const_ge(&ss, public_component, secret_component);
/* serialize shared secret in constant-time */
secp256k1_fe_normalize(&ss.x);
@@ -103,7 +101,6 @@ static void secp256k1_silentpayments_create_shared_secret(unsigned char *shared_
/* Leaking these values would break indistinguishability of the transaction, so clear them. */
secp256k1_ge_clear(&ss);
- secp256k1_gej_clear(&ss_j);
}
/** Set hash state to the BIP340 tagged hash midstate for "BIP0352/SharedSecret". */
### src/tests.c
@@ -4694,9 +4694,9 @@ static void test_ecmult_target(const secp256k1_scalar* target, int mode) {
secp256k1_ecmult(&p2j, &pj, &n2, &secp256k1_scalar_zero);
secp256k1_ecmult(&ptj, &pj, target, &secp256k1_scalar_zero);
} else {
- secp256k1_ecmult_const(&p1j, &p, &n1);
- secp256k1_ecmult_const(&p2j, &p, &n2);
- secp256k1_ecmult_const(&ptj, &p, target);
+ secp256k1_ecmult_const_gej(&p1j, &p, &n1);
+ secp256k1_ecmult_const_gej(&p2j, &p, &n2);
+ secp256k1_ecmult_const_gej(&ptj, &p, target);
}
/* Add them all up: n1*P + n2*P + target*P = (n1+n2+target)*P = (n1+n1-n1-n2)*P = 0. */
@@ -4761,7 +4761,7 @@ static void ecmult_const_random_mult(void) {
0xb84e4e1b, 0xfb77e21f, 0x96baae2a, 0x63dec956
);
secp256k1_gej b;
- secp256k1_ecmult_const(&b, &a, &xn);
+ secp256k1_ecmult_const_gej(&b, &a, &xn);
CHECK(secp256k1_ge_is_valid_var(&a));
CHECK(secp256k1_gej_eq_ge_var(&b, &expected_b));
@@ -4770,21 +4770,15 @@ static void ecmult_const_random_mult(void) {
static void ecmult_const_commutativity(void) {
secp256k1_scalar a;
secp256k1_scalar b;
- secp256k1_gej res1;
- secp256k1_gej res2;
secp256k1_ge mid1;
secp256k1_ge mid2;
testutil_random_scalar_order_test(&a);
testutil_random_scalar_order_test(&b);
- secp256k1_ecmult_const(&res1, &secp256k1_ge_const_g, &a);
- secp256k1_ecmult_const(&res2, &secp256k1_ge_const_g, &b);
- secp256k1_ge_set_gej(&mid1, &res1);
- secp256k1_ge_set_gej(&mid2, &res2);
- secp256k1_ecmult_const(&res1, &mid1, &b);
- secp256k1_ecmult_const(&res2, &mid2, &a);
- secp256k1_ge_set_gej(&mid1, &res1);
- secp256k1_ge_set_gej(&mid2, &res2);
+ secp256k1_ecmult_const_ge(&mid1, &secp256k1_ge_const_g, &a);
+ secp256k1_ecmult_const_ge(&mid2, &secp256k1_ge_const_g, &b);
+ secp256k1_ecmult_const_ge(&mid1, &mid1, &b);
+ secp256k1_ecmult_const_ge(&mid2, &mid2, &a);
CHECK(secp256k1_ge_eq_var(&mid1, &mid2));
}
@@ -4802,20 +4796,20 @@ static void ecmult_const_mult_zero_one(void) {
secp256k1_ge_set_infinity(&inf);
/* 0*point */
- secp256k1_ecmult_const(&res1, &point, &secp256k1_scalar_zero);
+ secp256k1_ecmult_const_gej(&res1, &point, &secp256k1_scalar_zero);
CHECK(secp256k1_gej_is_infinity(&res1));
/* s*inf */
- secp256k1_ecmult_const(&res1, &inf, &s);
+ secp256k1_ecmult_const_gej(&res1, &inf, &s);
CHECK(secp256k1_gej_is_infinity(&res1));
/* 1*point */
- secp256k1_ecmult_const(&res1, &point, &secp256k1_scalar_one);
+ secp256k1_ecmult_const_gej(&res1, &point, &secp256k1_scalar_one);
secp256k1_ge_set_gej(&res2, &res1);
CHECK(secp256k1_ge_eq_var(&res2, &point));
/* -1*point */
- secp256k1_ecmult_const(&res1, &point, &negone);
+ secp256k1_ecmult_const_gej(&res1, &point, &negone);
secp256k1_gej_neg(&res1, &res1);
secp256k1_ge_set_gej(&res2, &res1);
CHECK(secp256k1_ge_eq_var(&res2, &point));
@@ -4855,7 +4849,7 @@ static void ecmult_const_edges(void) {
secp256k1_scalar_add(&q, &q, &scalars_near_split_bounds[i - 1]);
}
testutil_random_ge_test(&point);
- secp256k1_ecmult_const(&res, &point, &q);
+ secp256k1_ecmult_const_gej(&res, &point, &q);
ecmult_const_check_result(&point, &q, &res);
}
}
@@ -4927,17 +4921,12 @@ static void ecmult_const_chain_multiply(void) {
0x5d195d20, 0xe191bf7f, 0x1be3e55f, 0x56a80196,
0x6071ad01, 0xf1462f66, 0xc997fa94, 0xdb858435
);
- secp256k1_gej point;
- secp256k1_ge res;
+ secp256k1_ge res = secp256k1_ge_const_g;
int i;
- secp256k1_gej_set_ge(&point, &secp256k1_ge_const_g);
for (i = 0; i < 100; ++i) {
- secp256k1_ge tmp;
- secp256k1_ge_set_gej(&tmp, &point);
- secp256k1_ecmult_const(&point, &tmp, &scalar);
+ secp256k1_ecmult_const_ge(&res, &res, &scalar);
}
- secp256k1_ge_set_gej(&res, &point);
CHECK(secp256k1_gej_eq_ge_var(&expected_point, &res));
}
@@ -5746,7 +5735,7 @@ static void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar
secp256k1_ecmult(&rj[3], &infj, &secp256k1_scalar_zero, x);
CHECK(secp256k1_ecmult_multi_var(&CTX->error_callback, scratch, &rj[4], x, NULL, NULL, 0));
CHECK(secp256k1_ecmult_multi_var(&CTX->error_callback, scratch, &rj[5], &secp256k1_scalar_zero, test_ecmult_accumulate_cb, (void*)x, 1));
- secp256k1_ecmult_const(&rj[6], &secp256k1_ge_const_g, x);
+ secp256k1_ecmult_const_gej(&rj[6], &secp256k1_ge_const_g, x);
secp256k1_ge_set_gej_var(&r, &rj[0]);
for (i = 0; i < ARRAY_SIZE(rj); i++) {
CHECK(secp256k1_gej_eq_ge_var(&rj[i], &r));
@@ -5951,7 +5940,7 @@ static void test_ecmult_gen_edge_cases(void) {
/* Run test with gn = i - scalar_offset (so that the ecmult_gen recoded value represents i). */
secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &res1, &gn);
secp256k1_ecmult(&res2, NULL, &secp256k1_scalar_zero, &gn);
- secp256k1_ecmult_const(&res3, &secp256k1_ge_const_g, &gn);
+ secp256k1_ecmult_const_gej(&res3, &secp256k1_ge_const_g, &gn);
CHECK(secp256k1_gej_eq_var(&res1, &res2));
CHECK(secp256k1_gej_eq_var(&res1, &res3));
secp256k1_scalar_add(&gn, &gn, &secp256k1_scalar_one);
### src/tests_exhaustive.c
@@ -168,7 +168,7 @@ static void test_exhaustive_ecmult(const secp256k1_ge *group, const secp256k1_ge
secp256k1_scalar_set_int(&ng, j);
/* Test secp256k1_ecmult_const. */
- secp256k1_ecmult_const(&tmp, &group[i], &ng);
+ secp256k1_ecmult_const_gej(&tmp, &group[i], &ng);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i * j) % EXHAUSTIVE_TEST_ORDER]));
if (i != 0 && j != 0) {Risk score
Our methodology →Why this scored 37/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.