Skip to content

Clear scalar variables computed from scalar in secp256k1_ecmult_const #1885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
niooss-ledger wants to merge 2 commits into
base: master
Choose a base branch
from
+9 −0
Open

Clear scalar variables computed from scalar in secp256k1_ecmult_const #1885

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41784db
Clear scalar variables computed from scalar in secp256k1_ecmult_const
niooss-ledger Jun 29, 2026
3523d43
Clear more temporary variables in secp256k1_ecmult_const
niooss-ledger Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/ecmult_const_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ static void secp256k1_ecmult_const_odd_multiples_table_globalz(secp256k1_ge *pre
} \
secp256k1_fe_negate(&neg_y, &(r)->y, 1); \
secp256k1_fe_cmov(&(r)->y, &neg_y, negative); \
secp256k1_fe_clear(&neg_y); \
} while(0)

/* For K as defined in the comment of secp256k1_ecmult_const, we have several precomputed
Expand Down Expand Up @@ -247,6 +248,7 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
int j;

ECMULT_CONST_TABLE_GET_GE(&t, pre_a, bits1);
secp256k1_memclear_explicit(&bits1, sizeof(bits1));
if (group == ECMULT_CONST_GROUPS - 1) {
/* Directly set r in the first iteration. */
secp256k1_gej_set_ge(r, &t);
Expand All @@ -258,11 +260,18 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
secp256k1_gej_add_ge(r, r, &t);
}
ECMULT_CONST_TABLE_GET_GE(&t, pre_a_lam, bits2);
secp256k1_memclear_explicit(&bits2, sizeof(bits2));
secp256k1_gej_add_ge(r, r, &t);
secp256k1_ge_clear(&t);

@theStack theStack Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in theory it should be slightly faster to do the stack clearing only once at the last loop iteration (i.e if group == 0) rather than repeatedly, but practically even with the current code there seems to be no measurable slow-down compared to master (bench_ecmult shows 19.2us for ecmult_const for both branches on my machine), so seems fine as-is

@niooss-ledger niooss-ledger Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, as compilers are free to unroll the loop for (group = ECMULT_CONST_GROUPS - 1; group >= 0; --group) and allocate local variables in different places for each iteration, it is a bit unsound to do if (group == 0) { /* clear variables */ }.

If calling clear functions several times is an issue, it will make more sense to move the relevant variables (t, neg_y...) outside of the loop and to clear them after the loop. I did not do this, as I wanted to keep the changes small.

@theStack theStack Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point, the if (group == 0) { ... } proposal was a bit half-baked. I agree that moving variables outside of the loop would be preferred (in the hypothetical case that the multiple clears were indeed an issue).

}

/* Map the result back to the secp256k1 curve from the isomorphic curve. */
secp256k1_fe_mul(&r->z, &r->z, &global_z);

/* Clear local variables computed from scalar q */
secp256k1_scalar_clear(&s);
secp256k1_scalar_clear(&v1);
secp256k1_scalar_clear(&v2);
}

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) {
Expand Down
Loading