group: Avoid using infinity field directly in other modules
What changed, and why it matters
This commit is a code-cleanup change in Bitcoin Core's libsecp256k1 cryptographic library. It replaces direct reads and writes of an internal 'infinity' flag on elliptic-curve points with calls to dedicated helper functions. The stated goal is to hide the internal field from other modules, making future mistakes less likely. There is no direct evidence in the commit that this fixes an exploitable vulnerability, but it is a defensive hardening change in security-critical code.
Treat as routine defensive hardening. Review that all external `.infinity` accesses are now covered and that the new helper functions preserve existing behavior, especially in constant-time paths. No urgent security response is indicated by the commit itself.
Security signals we found
Encapsulation of internal point representation to reduce risk of inconsistent state
Use of dedicated constructors (`secp256k1_ge_set_xy`) ensures `infinity` flag is set consistently
Changes in constant-time multiplication code (`ecmult_const_impl.h`) where state consistency matters
No direct bug or vulnerability described in commit message or diff
Evidence from the diff
The patch removes direct access to the .infinity member of secp256k1_ge and secp256k1_gej structures outside of the group module, replacing it with accessor/constructor functions such as secp256k1_ge_set_xy, secp256k1_ge_set_infinity, secp256k1_ge_is_infinity, secp256k1_gej_is_infinity, secp256k1_gej_set_ge, and secp256k1_gej_rescale. It also replaces manual Jacobian coordinate scaling in test utilities with secp256k1_gej_rescale. The changes are spread across constant-time and non-constant-time scalar multiplication implementations and test code. This is an encapsulation/hardening refactor rather than a patch for a known bug.
Changed components
src/ecmult_const_impl.hsrc/ecmult_impl.hsrc/tests.csrc/tests_exhaustive.csrc/testutil.hInspect captured patch +25 / −28
diff --git a/src/ecmult_const_impl.h b/src/ecmult_const_impl.h
index 4f66209..1d24aea 100644
--- a/src/ecmult_const_impl.h
+++ b/src/ecmult_const_impl.h
@@ -87,17 +87,15 @@ static void secp256k1_ecmult_const_odd_multiples_table_globalz(secp256k1_ge *pre
secp256k1_fe neg_y; \
VERIFY_CHECK((n) < (1U << ECMULT_CONST_GROUP_SIZE)); \
VERIFY_CHECK(index < (1U << (ECMULT_CONST_GROUP_SIZE - 1))); \
- /* Unconditionally set r->x = (pre)[m].x. r->y = (pre)[m].y. because it's either the correct one
+ /* Unconditionally set r->x = (pre)[m].x and r->y = (pre)[m].y because it's either the correct one
* or will get replaced in the later iterations, this is needed to make sure `r` is initialized. */ \
- (r)->x = (pre)[m].x; \
- (r)->y = (pre)[m].y; \
+ secp256k1_ge_set_xy((r), &(pre)[m].x, &(pre)[m].y); \
for (m = 1; m < ECMULT_CONST_TABLE_SIZE; m++) { \
/* This loop is used to avoid secret data in array indices. See
* the comment in ecmult_gen_impl.h for rationale. */ \
secp256k1_fe_cmov(&(r)->x, &(pre)[m].x, m == index); \
secp256k1_fe_cmov(&(r)->y, &(pre)[m].y, m == index); \
} \
- (r)->infinity = 0; \
secp256k1_fe_negate(&neg_y, &(r)->y, 1); \
secp256k1_fe_cmov(&(r)->y, &neg_y, negative); \
} while(0)
@@ -375,11 +373,14 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
SECP256K1_FE_VERIFY_MAGNITUDE(&g, 2);
- /* Compute base point P = (n*g, g^2), the effective affine version of (n*g, g^2, v), which has
- * corresponding affine X coordinate n/d. */
- secp256k1_fe_mul(&p.x, &g, n);
- secp256k1_fe_sqr(&p.y, &g);
- p.infinity = 0;
+ /* Compute base point P = (n*g, g^2), the effective affine version of
+ * (n*g, g^2, v), which has corresponding affine X coordinate n/d. */
+ {
+ secp256k1_fe x, y;
+ secp256k1_fe_mul(&x, &g, n);
+ secp256k1_fe_sqr(&y, &g);
+ secp256k1_ge_set_xy(&p, &x, &y);
+ }
/* Perform x-only EC multiplication of P with q. */
VERIFY_CHECK(!secp256k1_scalar_is_zero(q));
diff --git a/src/ecmult_impl.h b/src/ecmult_impl.h
index 0b53b3f..20cf750 100644
--- a/src/ecmult_impl.h
+++ b/src/ecmult_impl.h
@@ -75,7 +75,7 @@ static void secp256k1_ecmult_odd_multiples_table(int n, secp256k1_ge *pre_a, sec
secp256k1_ge d_ge;
int i;
- VERIFY_CHECK(!a->infinity);
+ VERIFY_CHECK(!secp256k1_gej_is_infinity(a));
secp256k1_gej_double_var(&d, a, NULL);
@@ -341,7 +341,7 @@ static void secp256k1_ecmult_strauss_wnaf(const struct secp256k1_strauss_state *
}
}
- if (!r->infinity) {
+ if (!secp256k1_gej_is_infinity(r)) {
secp256k1_fe_mul(&r->z, &r->z, &Z);
}
}
diff --git a/src/tests.c b/src/tests.c
index 0ccdbee..dc5f279 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -4152,8 +4152,8 @@ static void test_group_decompress(const secp256k1_fe* x) {
secp256k1_fe_normalize_var(&ge_even.y);
/* No infinity allowed. */
- CHECK(!ge_even.infinity);
- CHECK(!ge_odd.infinity);
+ CHECK(!secp256k1_ge_is_infinity(&ge_even));
+ CHECK(!secp256k1_ge_is_infinity(&ge_odd));
/* Check that the x coordinates check out. */
CHECK(secp256k1_fe_equal(&ge_even.x, x));
diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c
index f8bbcaa..13bda61 100644
--- a/src/tests_exhaustive.c
+++ b/src/tests_exhaustive.c
@@ -103,9 +103,11 @@ static void test_exhaustive_addition(const secp256k1_ge *group, const secp256k1_
secp256k1_gej_add_ge_var(&tmp, &groupj[i], &group[j], NULL);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i + j) % EXHAUSTIVE_TEST_ORDER]));
/* add_zinv_var */
- zless_gej.infinity = groupj[j].infinity;
- zless_gej.x = groupj[j].x;
- zless_gej.y = groupj[j].y;
+ if (secp256k1_gej_is_infinity(&groupj[j])) {
+ secp256k1_ge_set_infinity(&zless_gej);
+ } else {
+ secp256k1_ge_set_xy(&zless_gej, &groupj[j].x, &groupj[j].y);
+ }
secp256k1_gej_add_zinv_var(&tmp, &groupj[i], &zless_gej, &fe_inv);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i + j) % EXHAUSTIVE_TEST_ORDER]));
}
@@ -422,10 +424,8 @@ int main(int argc, char** argv) {
secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &generatedj, &scalar_i);
secp256k1_ge_set_gej(&generated, &generatedj);
- CHECK(group[i].infinity == 0);
- CHECK(generated.infinity == 0);
- CHECK(secp256k1_fe_equal(&generated.x, &group[i].x));
- CHECK(secp256k1_fe_equal(&generated.y, &group[i].y));
+ CHECK(!secp256k1_ge_is_infinity(&group[i]));
+ CHECK(secp256k1_ge_eq_var(&group[i], &generated));
}
}
diff --git a/src/testutil.h b/src/testutil.h
index 480f6a1..93ee3d5 100644
--- a/src/testutil.h
+++ b/src/testutil.h
@@ -96,17 +96,13 @@ static void testutil_random_ge_test(secp256k1_ge *ge) {
break;
}
} while(1);
- ge->infinity = 0;
}
static void testutil_random_ge_jacobian_test(secp256k1_gej *gej, const secp256k1_ge *ge) {
- secp256k1_fe z2, z3;
- testutil_random_fe_non_zero_test(&gej->z);
- secp256k1_fe_sqr(&z2, &gej->z);
- secp256k1_fe_mul(&z3, &z2, &gej->z);
- secp256k1_fe_mul(&gej->x, &ge->x, &z2);
- secp256k1_fe_mul(&gej->y, &ge->y, &z3);
- gej->infinity = ge->infinity;
+ secp256k1_fe z;
+ testutil_random_fe_non_zero_test(&z);
+ secp256k1_gej_set_ge(gej, ge);
+ secp256k1_gej_rescale(gej, &z);
}
static void testutil_random_gej_test(secp256k1_gej *gej) {
Why this scored 23/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.