Clear scalar variables computed from scalar in `secp256k1_ecmult_const` #1885

pull niooss-ledger wants to merge 2 commits into bitcoin-core:master from niooss-ledger:clear-scalar-variables-secp256k1_ecmult_const changing 1 files +9 −0

niooss-ledger commented at 8:22 AM on June 29, 2026: contributor

Hello, Function secp256k1_ecdh performs an ECDH computation and clears some intermediate values from the stack before returning: https://github.com/bitcoin-core/secp256k1/blob/68b45fd4e266c5a6b80097319155114475de697e/src/modules/ecdh/main_impl.h#L70-L74

This function calls secp256k1_ecmult_const, which does not clear anything.

Clear the scalar variables computed from the scalar operand in secp256k1_ecmult_const.

N.B. After this PR, some stack variables still hold values related to scalar q. For example: macro ECMULT_CONST_TABLE_GET_GE uses a temporary variable secp256k1_fe neg_y which value comes from some bits of the scalar ; a for loop uses a temporary point secp256k1_ge t which is never cleared ; variables unsigned int bits1 and unsigned int bits2 are not cleared, even though the compiler may choose to put them in registers instead of the stack. Nonetheless these remaining variables do not seem to enable reconstructing the value of input scalar q (contrary to s, v1, v2, whose values could be used to reconstruct q). I therefore believe it is all right to limit the clearing to what this Pull Request adds.

Clear scalar variables computed from scalar in secp256k1_ecmult_const

Function secp256k1_ecdh performs an ECDH computation and clears some
intermediate values from the stack before returning. This function calls
secp256k1_ecmult_const, which does not clear anything.

Clear the scalar variables computed from the scalar operand in
secp256k1_ecmult_const.

41784dbe55

real-or-random added the label side-channel on Jun 29, 2026
real-or-random added the label tweak/refactor on Jun 29, 2026
real-or-random commented at 12:34 PM on June 29, 2026: contributor

Thanks for a PR!

N.B. After this PR, some stack variables still hold values related to scalar q

I think clearing is cheap enough so that we'd prefer to add on the side of caution here and clear all variables that depend on q, even if they allow reconstructing q only partially. Do you want to make that change?

Clear more temporary variables in secp256k1_ecmult_const

Some temporary variables in secp256k1_ecmult_const depends on few bits
of the scalar. Clear them as well.

3523d434fe

niooss-ledger commented at 3:49 PM on June 29, 2026: contributor
Thanks for your quick review! I added a commit which clears the variables I mentioned. I ran the test suite (make check) as well as the benchmark (./bench_ecmult) and the timings of ecmult_const did not change much (I did not spend time to go through a thorough performance impact analysis).

I also took a look at the generated assembly code on x86_64, and it seemed reasonable:
```
In C : secp256k1_fe_clear(&neg_y);
   1187a:       48 89 ef                mov    rdi,rbp
   1187d:       89 d8                   mov    eax,ebx
   1187f:       b9 0c 00 00 00          mov    ecx,0xc
   11884:       f3 ab                   rep stos DWORD PTR es:[rdi],eax
In C : secp256k1_memclear_explicit(&bits2, sizeof(bits2));
   11886:       c7 44 24 54 00 00 00    mov    DWORD PTR [rsp+0x54],0x0
   1188d:       00 
```
By the way, to reduce the use of stack variables, unsigned int bits2 = secp256k1_scalar_get_bits_var(&v2, group * ECMULT_CONST_GROUP_SIZE, ECMULT_CONST_GROUP_SIZE); could be moved right before ECMULT_CONST_TABLE_GET_GE(&t, pre_a_lam, bits2);, but I feel like this kind of change would be too intrusive for this Pull Request.
in src/ecmult_const_impl.h:265 in 3523d434fe
```
 259 | @@ -258,7 +260,9 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
 260 |              secp256k1_gej_add_ge(r, r, &t);
 261 |          }
 262 |          ECMULT_CONST_TABLE_GET_GE(&t, pre_a_lam, bits2);
 263 | +        secp256k1_memclear_explicit(&bits2, sizeof(bits2));
 264 |          secp256k1_gej_add_ge(r, r, &t);
 265 | +        secp256k1_ge_clear(&t);
```
theStack commented at 5:26 PM on June 29, 2026:

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 commented at 6:55 PM on June 29, 2026:

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 commented at 8:53 PM on June 29, 2026:

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).
theStack commented at 5:26 PM on June 29, 2026: contributor

Concept ACK

Contributors

niooss-ledger

real-or-random

theStack

Labels

side-channel tweak/refactor