From a3296d5e23f1475575cb8f0aa8209349cc9a6cfb Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 19 May 2026 16:29:05 +0200 Subject: [PATCH 1/2] refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) Scalar multiplication with the generator point 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 steps: - secp256k1_ecmult_gen(ctx, &rj, ...) - secp256k1_ge_set_gej(&r, &rj) - secp256k1_gej_clear(&rj) This commit introduces a corresponding helper to deduplicate code and mitigate the risk that last step is forgotten (which can easily happen and is not detected by tests). The idea came up during a conversation with furszy, see https://github.com/bitcoin-core/secp256k1/pull/1765#issuecomment-4482838033 --- src/ecdsa_impl.h | 5 +---- src/ecmult_gen.h | 1 + src/ecmult_gen_impl.h | 14 ++++++++++---- src/modules/schnorrsig/main_impl.h | 5 +---- src/secp256k1.c | 5 +---- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index 163539ebc1..32f1e58500 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -273,14 +273,12 @@ static int secp256k1_ecdsa_sig_verify(const secp256k1_scalar *sigr, const secp25 static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, secp256k1_scalar *sigr, secp256k1_scalar *sigs, const secp256k1_scalar *seckey, const secp256k1_scalar *message, const secp256k1_scalar *nonce, int *recid) { unsigned char b[32]; - secp256k1_gej rp; secp256k1_ge r; secp256k1_scalar n; int overflow = 0; int high; - secp256k1_ecmult_gen(ctx, &rp, nonce); - secp256k1_ge_set_gej(&r, &rp); + secp256k1_ecmult_gen_ge(ctx, &r, nonce); secp256k1_fe_normalize(&r.x); secp256k1_fe_normalize(&r.y); secp256k1_fe_get_b32(b, &r.x); @@ -296,7 +294,6 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec secp256k1_scalar_inverse(sigs, nonce); secp256k1_scalar_mul(sigs, sigs, &n); secp256k1_scalar_clear(&n); - secp256k1_gej_clear(&rp); secp256k1_ge_clear(&r); high = secp256k1_scalar_is_high(sigs); secp256k1_scalar_cond_negate(sigs, high); diff --git a/src/ecmult_gen.h b/src/ecmult_gen.h index 8bc4f14c31..b842e780fe 100644 --- a/src/ecmult_gen.h +++ b/src/ecmult_gen.h @@ -138,6 +138,7 @@ static void secp256k1_ecmult_gen_context_clear(secp256k1_ecmult_gen_context* ctx /** Multiply with the generator: R = a*G */ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context* ctx, secp256k1_gej *r, const secp256k1_scalar *a); +static void secp256k1_ecmult_gen_ge(const secp256k1_ecmult_gen_context* ctx, secp256k1_ge *r, const secp256k1_scalar *a); static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const secp256k1_hash_ctx *hash_ctx, const unsigned char *seed32); diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 5a954977eb..53dc5f3d90 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -281,11 +281,19 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25 secp256k1_memclear_explicit(&recoded, sizeof(recoded)); } +SECP256K1_INLINE static void secp256k1_ecmult_gen_ge(const secp256k1_ecmult_gen_context *ctx, secp256k1_ge *r, const secp256k1_scalar *a) { + secp256k1_gej rj; + secp256k1_ecmult_gen(ctx, &rj, a); + 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); +} + /* Setup blinding values for secp256k1_ecmult_gen. */ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const secp256k1_hash_ctx *hash_ctx, const unsigned char *seed32) { secp256k1_scalar b; secp256k1_scalar diff; - secp256k1_gej gb; secp256k1_fe f; unsigned char nonce32[32]; secp256k1_rfc6979_hmac_sha256 rng; @@ -325,15 +333,13 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const * which secp256k1_gej_add_ge cannot handle. */ secp256k1_scalar_cmov(&b, &secp256k1_scalar_one, secp256k1_scalar_is_zero(&b)); secp256k1_rfc6979_hmac_sha256_finalize(&rng); - secp256k1_ecmult_gen(ctx, &gb, &b); + secp256k1_ecmult_gen_ge(ctx, &ctx->ge_offset, &b); secp256k1_scalar_negate(&b, &b); secp256k1_scalar_add(&ctx->scalar_offset, &b, &diff); - secp256k1_ge_set_gej(&ctx->ge_offset, &gb); /* Clean up. */ secp256k1_memclear_explicit(nonce32, sizeof(nonce32)); secp256k1_scalar_clear(&b); - secp256k1_gej_clear(&gb); secp256k1_fe_clear(&f); secp256k1_rfc6979_hmac_sha256_clear(&rng); } diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h index 5100557f4d..efc7216546 100644 --- a/src/modules/schnorrsig/main_impl.h +++ b/src/modules/schnorrsig/main_impl.h @@ -123,7 +123,6 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi secp256k1_scalar sk; secp256k1_scalar e; secp256k1_scalar k; - secp256k1_gej rj; secp256k1_ge pk; secp256k1_ge r; unsigned char nonce32[32] = { 0 }; @@ -160,8 +159,7 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi ret &= !secp256k1_scalar_is_zero(&k); secp256k1_scalar_cmov(&k, &secp256k1_scalar_one, !ret); - secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &rj, &k); - secp256k1_ge_set_gej(&r, &rj); + secp256k1_ecmult_gen_ge(&ctx->ecmult_gen_ctx, &r, &k); /* We declassify r to allow using it as a branch point. This is fine * because r is not a secret. */ @@ -183,7 +181,6 @@ static int secp256k1_schnorrsig_sign_internal(const secp256k1_context* ctx, unsi secp256k1_scalar_clear(&sk); secp256k1_memclear_explicit(seckey, sizeof(seckey)); secp256k1_memclear_explicit(nonce32, sizeof(nonce32)); - secp256k1_gej_clear(&rj); return ret; } diff --git a/src/secp256k1.c b/src/secp256k1.c index e4b80fff24..b216872e12 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -624,15 +624,12 @@ int secp256k1_ec_seckey_verify(const secp256k1_context* ctx, const unsigned char } static int secp256k1_ec_pubkey_create_helper(const secp256k1_ecmult_gen_context *ecmult_gen_ctx, secp256k1_scalar *seckey_scalar, secp256k1_ge *p, const unsigned char *seckey) { - secp256k1_gej pj; int ret; ret = secp256k1_scalar_set_b32_seckey(seckey_scalar, seckey); secp256k1_scalar_cmov(seckey_scalar, &secp256k1_scalar_one, !ret); - secp256k1_ecmult_gen(ecmult_gen_ctx, &pj, seckey_scalar); - secp256k1_ge_set_gej(p, &pj); - secp256k1_gej_clear(&pj); + secp256k1_ecmult_gen_ge(ecmult_gen_ctx, p, seckey_scalar); return ret; } From 9e017e5062bfe306eb5724fe39debe94b6223592 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 4 Jun 2026 16:35:12 +0200 Subject: [PATCH 2/2] refactor: rename `_ecmult_gen` -> `_ecmult_gen_gej` for consistency Now that we have a function `_ecmult_gen_ge`, it makes sense to rename the existing function `_ecmult_gen` to `_ecmult_gen_gej` for consistency, to signal that the result is a Jacobian group element. This diff was created by applying ``` $ sed -i s/secp256k1_ecmult_gen\(/secp256k1_ecmult_gen_gej\(/g $(git ls-files) ``` --- src/bench_ecmult.c | 4 ++-- src/ecmult_gen.h | 2 +- src/ecmult_gen_impl.h | 4 ++-- src/modules/musig/session_impl.h | 2 +- src/tests.c | 26 +++++++++++++------------- src/tests_exhaustive.c | 2 +- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/bench_ecmult.c b/src/bench_ecmult.c index eb546db414..12d550a6ae 100644 --- a/src/bench_ecmult.c +++ b/src/bench_ecmult.c @@ -88,7 +88,7 @@ static void bench_ecmult_teardown_helper(bench_data* data, size_t* seckey_offset secp256k1_scalar_add(&sum_scalars, &sum_scalars, &s); } } - secp256k1_ecmult_gen(&data->ctx->ecmult_gen_ctx, &tmp, &sum_scalars); + secp256k1_ecmult_gen_gej(&data->ctx->ecmult_gen_ctx, &tmp, &sum_scalars); CHECK(secp256k1_gej_eq_var(&tmp, &sum_output)); } @@ -104,7 +104,7 @@ static void bench_ecmult_gen(void* arg, int iters) { int i; for (i = 0; i < iters; ++i) { - secp256k1_ecmult_gen(&data->ctx->ecmult_gen_ctx, &data->output[i], &data->scalars[(data->offset1+i) % POINTS]); + secp256k1_ecmult_gen_gej(&data->ctx->ecmult_gen_ctx, &data->output[i], &data->scalars[(data->offset1+i) % POINTS]); } } diff --git a/src/ecmult_gen.h b/src/ecmult_gen.h index b842e780fe..770b2cb21a 100644 --- a/src/ecmult_gen.h +++ b/src/ecmult_gen.h @@ -137,7 +137,7 @@ static void secp256k1_ecmult_gen_context_build(secp256k1_ecmult_gen_context* ctx static void secp256k1_ecmult_gen_context_clear(secp256k1_ecmult_gen_context* ctx); /** Multiply with the generator: R = a*G */ -static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context* ctx, secp256k1_gej *r, const secp256k1_scalar *a); +static void secp256k1_ecmult_gen_gej(const secp256k1_ecmult_gen_context* ctx, secp256k1_gej *r, const secp256k1_scalar *a); static void secp256k1_ecmult_gen_ge(const secp256k1_ecmult_gen_context* ctx, secp256k1_ge *r, const secp256k1_scalar *a); static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const secp256k1_hash_ctx *hash_ctx, const unsigned char *seed32); diff --git a/src/ecmult_gen_impl.h b/src/ecmult_gen_impl.h index 53dc5f3d90..a7a6d34d71 100644 --- a/src/ecmult_gen_impl.h +++ b/src/ecmult_gen_impl.h @@ -51,7 +51,7 @@ static void secp256k1_ecmult_gen_scalar_diff(secp256k1_scalar* diff) { secp256k1_scalar_add(diff, diff, &neghalf); } -static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp256k1_gej *r, const secp256k1_scalar *gn) { +static void secp256k1_ecmult_gen_gej(const secp256k1_ecmult_gen_context *ctx, secp256k1_gej *r, const secp256k1_scalar *gn) { uint32_t comb_off; secp256k1_ge add; secp256k1_fe neg; @@ -283,7 +283,7 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25 SECP256K1_INLINE static void secp256k1_ecmult_gen_ge(const secp256k1_ecmult_gen_context *ctx, secp256k1_ge *r, const secp256k1_scalar *a) { secp256k1_gej rj; - secp256k1_ecmult_gen(ctx, &rj, a); + secp256k1_ecmult_gen_gej(ctx, &rj, a); 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. */ diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h index 510ee89fc4..c05801eecf 100644 --- a/src/modules/musig/session_impl.h +++ b/src/modules/musig/session_impl.h @@ -415,7 +415,7 @@ static int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp /* Compute pubnonce as two gejs */ for (i = 0; i < 2; i++) { - secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &nonce_ptj[i], &k[i]); + secp256k1_ecmult_gen_gej(&ctx->ecmult_gen_ctx, &nonce_ptj[i], &k[i]); secp256k1_scalar_clear(&k[i]); } diff --git a/src/tests.c b/src/tests.c index 862bef61a7..9084c1d6d8 100644 --- a/src/tests.c +++ b/src/tests.c @@ -329,7 +329,7 @@ static void run_proper_context_tests(int use_prealloc) { /*** attempt to use them ***/ testutil_random_scalar_order_test(&msg); testutil_random_scalar_order_test(&key); - secp256k1_ecmult_gen(&my_ctx->ecmult_gen_ctx, &pubj, &key); + secp256k1_ecmult_gen_gej(&my_ctx->ecmult_gen_ctx, &pubj, &key); secp256k1_ge_set_gej(&pub, &pubj); /* obtain a working nonce */ @@ -4311,11 +4311,11 @@ static void test_ec_combine(void) { secp256k1_scalar s; testutil_random_scalar_order_test(&s); secp256k1_scalar_add(&sum, &sum, &s); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &Qj, &s); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &Qj, &s); secp256k1_ge_set_gej(&Q, &Qj); secp256k1_pubkey_save(&data[i - 1], &Q); d[i - 1] = &data[i - 1]; - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &Qj, &sum); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &Qj, &sum); secp256k1_ge_set_gej(&Q, &Qj); secp256k1_pubkey_save(&sd, &Q); CHECK(secp256k1_ec_pubkey_combine(CTX, &sd2, d, i) == 1); @@ -4593,9 +4593,9 @@ static void test_ecmult_target(const secp256k1_scalar* target, int mode) { /* EC multiplications */ if (mode == 0) { - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &p1j, &n1); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &p2j, &n2); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &ptj, target); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &p1j, &n1); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &p2j, &n2); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &ptj, target); } else if (mode == 1) { secp256k1_ecmult(&p1j, &pj, &n1, &secp256k1_scalar_zero); secp256k1_ecmult(&p2j, &pj, &n2, &secp256k1_scalar_zero); @@ -5162,7 +5162,7 @@ static int test_ecmult_multi_random(secp256k1_scratch *scratch) { secp256k1_scalar_mul(&scalars[filled], &sc_tmp, &g_scalar); secp256k1_scalar_inverse_var(&sc_tmp, &sc_tmp); secp256k1_scalar_negate(&sc_tmp, &sc_tmp); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &gejs[filled], &sc_tmp); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &gejs[filled], &sc_tmp); ++filled; ++mults; } @@ -5642,7 +5642,7 @@ static void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar size_t i; secp256k1_gej_set_ge(&gj, &secp256k1_ge_const_g); secp256k1_gej_set_infinity(&infj); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &rj[0], x); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &rj[0], x); secp256k1_ecmult(&rj[1], &gj, x, NULL); secp256k1_ecmult(&rj[2], &gj, x, &secp256k1_scalar_zero); secp256k1_ecmult(&rj[3], &infj, &secp256k1_scalar_zero, x); @@ -5796,13 +5796,13 @@ static void test_ecmult_gen_blind(void) { secp256k1_ge p; secp256k1_ge pge; testutil_random_scalar_order_test(&key); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &pgej, &key); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &pgej, &key); testrand256(seed32); b = CTX->ecmult_gen_ctx.scalar_offset; p = CTX->ecmult_gen_ctx.ge_offset; secp256k1_ecmult_gen_blind(&CTX->ecmult_gen_ctx, secp256k1_get_hash_context(CTX), seed32); CHECK(!secp256k1_scalar_eq(&b, &CTX->ecmult_gen_ctx.scalar_offset)); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &pgej2, &key); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &pgej2, &key); CHECK(!gej_xyz_equals_gej(&pgej, &pgej2)); CHECK(!secp256k1_ge_eq_var(&p, &CTX->ecmult_gen_ctx.ge_offset)); secp256k1_ge_set_gej(&pge, &pgej); @@ -5832,7 +5832,7 @@ static void test_ecmult_gen_edge_cases(void) { for (i = -1; i < 2; ++i) { /* Run test with gn = i - scalar_offset (so that the ecmult_gen recoded value represents i). */ - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &res1, &gn); + 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); CHECK(secp256k1_gej_eq_var(&res1, &res2)); @@ -6524,7 +6524,7 @@ static void test_ecdsa_sign_verify(void) { int recid; testutil_random_scalar_order_test(&msg); testutil_random_scalar_order_test(&key); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &pubj, &key); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &pubj, &key); secp256k1_ge_set_gej(&pub, &pubj); getrec = testrand_bits(1); /* The specific way in which this conditional is written sidesteps a potential bug in clang. @@ -7292,7 +7292,7 @@ static void run_ecdsa_edge_cases(void) { secp256k1_scalar_negate(&ss, &ss); secp256k1_scalar_inverse(&ss, &ss); secp256k1_scalar_set_int(&sr, 1); - secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &keyj, &sr); + secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &keyj, &sr); secp256k1_ge_set_gej(&key, &keyj); msg = ss; CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 0); diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c index 6d128bcca7..888b7ac927 100644 --- a/src/tests_exhaustive.c +++ b/src/tests_exhaustive.c @@ -425,7 +425,7 @@ int main(int argc, char** argv) { secp256k1_ge generated; secp256k1_scalar_set_int(&scalar_i, i); - secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &generatedj, &scalar_i); + secp256k1_ecmult_gen_gej(&ctx->ecmult_gen_ctx, &generatedj, &scalar_i); secp256k1_ge_set_gej(&generated, &generatedj); CHECK(!secp256k1_ge_is_infinity(&group[i]));