Clear scalar variables computed from scalar in `secp256k1_ecmult

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?

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 outdated

 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).

real-or-random commented at 8:35 AM on July 28, 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 */ }.

Hm, that's pretty subtle.

When the variable is declared inside the loop, then the C standard is really not clear (to me) about whether the variable will have the same address in every loop iteration. So yes, I see the concern. But do you have evidence that compilers in the real world do such a thing, i.e., allocate variables in different places for each iteration?

Keep in mind that, in general, nothing is "sound" here and all of this stack cleaning is a best-effort thing. The compiler may spill copies of the variable all over the stack if this helps optimizing even with our memclear (as long as it clears the "official" one where the address points to). I don't think that's a big concern in C but I've seen people discuss this for example in Rust with its copy semantics.

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.

My gut feeling is that, at least in case of neg_y and index this is in the inner loop, I have a slight preference of move these variables outside the loop body. Unfortunately that's a bit ugly in case of ECMULT_CONST_TABLE_GET_GE, which is macro. I believe the nicest way is to convert it into a function. There should be no performace difference between an inlined function and a macro... In the worst case, we need to force the compiler to inline. (If you prefer keeping it as a macro, it will need another do {} while(1) just to make sure the macro is a single compound statement.) @niooss-ledger I don't want to pull you into more work. If you want to make these changes, please go ahead. If not, just leave a comment here and I think we can take of it.

[1] Not even C23 is clear here, I think: "its lifetime extends from entry into the block with which it is associated until execution of that block ends in any way." The most conservative interpretation is that reaching the end of the loop body means that the block ends (even if another iteration follows).

niooss-ledger commented at 2:08 PM on July 28, 2026:

When the variable is declared inside the loop, then the C standard is really not clear (to me) about whether the variable will have the same address in every loop iteration. So yes, I see the concern. But do you have evidence that compilers in the real world do such a thing, i.e., allocate variables in different places for each iteration?

It appears I have spent a bit too much time analyzing assembly code generated by the Rust compiler (which is in general more complex than code generated for C programs, partly due to the way variables are moved/copied on several stack locations in the same function).

I have been trying to see how GCC and Clang behave when unrolling loops. It seem they both re-use the same location for local variables, as long as there is no variable-length array involved. Here is some C program I used to try to coerce this corner-case behavior.

<details> <summary>Example of C program I wrote to test this</summary>

#include <stdio.h>

void * __attribute__((noinline, weak)) do_something_and_get_instruction_pointer(void *p) {
    (void)p;
    return __builtin_return_address(0);
}

#ifndef __clang__
    // Enable unrolling loops even when compiling with "gcc -O0"
    #pragma GCC optimize ("unroll-loops")
#endif

int main(void) {
    unsigned int i;

#ifdef __clang__
    #pragma clang loop unroll_count(4)
#else
    #pragma GCC unroll 4
#endif
    for (i = 0; i < 16; i++) {
        unsigned int j[2] = {0};
        if (i % 2) {
            j[0] = 1;
        }
        void *ip = do_something_and_get_instruction_pointer(&j);
        printf("[main+%#zx] i = %d, j at %p is %u\n", (size_t)ip - (size_t)main, i, &j, j[0]);
    }

    return 0;
}

</details>

Keep in mind that, in general, nothing is "sound" here and all of this stack cleaning is a best-effort thing. The compiler may spill copies of the variable all over the stack if this helps optimizing even with our memclear (as long as it clears the "official" one where the address points to).

I agree, and even though we could check from time to time how the generated assembly code uses the stack, I understand this approach of trying to clear sensitive values is fragile and has to remain best-effort.

@niooss-ledger I don't want to pull you into more work. If you want to make these changes, please go ahead. If not, just leave a comment here and I think we can take of it.

Thanks :) As I will soon be on holidays and am a bit uncomfortable to "push & leave", feel free to take care of the changes.

theStack commented at 5:26 PM on June 29, 2026: contributor

Concept ACK

real-or-random added this to the milestone 0.7.2 on Jul 28, 2026

in src/ecmult_const_impl.h:101 in 3523d434fe outdated

  97 | @@ -98,6 +98,7 @@ static void secp256k1_ecmult_const_odd_multiples_table_globalz(secp256k1_ge *pre
  98 |      } \
  99 |      secp256k1_fe_negate(&neg_y, &(r)->y, 1); \
 100 |      secp256k1_fe_cmov(&(r)->y, &neg_y, negative); \
 101 | +    secp256k1_fe_clear(&neg_y); \

real-or-random commented at 8:14 AM on July 28, 2026:

Shouldn't we clear index here, too?

niooss-ledger commented at 3:22 PM on July 28, 2026:

Indeed. Looking at the generated assembly code (from ./autogen.sh && ./configure && make on Ubuntu x86_64), and analyzing function secp256k1_ecmult_const from src/libsecp256k1_la-secp256k1.o), index is actually pushed to a stack location. In Ghidra, it looks like this (when rsp + 8 is used):

But adding secp256k1_memclear_explicit(&index, sizeof(index)); does not seem to actually clear this copy. Generating the assembly code with gcc -DPACKAGE_NAME=\"libsecp256k1\" -DPACKAGE_TARNAME=\"libsecp256k1\" -DPACKAGE_VERSION=\"0.7.2-dev\" "-DPACKAGE_STRING=\"libsecp256k1 0.7.2-dev\"" -DPACKAGE_BUGREPORT=\"https://github.com/bitcoin-core/secp256k1/issues\" -DPACKAGE_URL=\"https://github.com/bitcoin-core/secp256k1\" -DPACKAGE=\"libsecp256k1\" -DVERSION=\"0.7.2-dev\" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DSTDC_HEADERS=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_WAIT_H=1 -DHAVE_UNISTD_H=1 -I. -DUSE_ASM_X86_64=1 -DECMULT_WINDOW_SIZE=15 -DCOMB_BLOCKS=43 -DCOMB_TEETH=6 -DENABLE_MODULE_ELLSWIFT=1 -DENABLE_MODULE_MUSIG=1 -DENABLE_MODULE_SCHNORRSIG=1 -DENABLE_MODULE_EXTRAKEYS=1 -DENABLE_MODULE_ECDH=1 -O2 -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -g -O2 -MT src/libsecp256k1_la-secp256k1.lo -MD -MP -MF src/.deps/libsecp256k1_la-secp256k1.Tpo -c src/secp256k1.c -S -o src/libsecp256k1_la-secp256k1.s (GCC version gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0 on Ubuntu 22.04) shows:

.LBE54234:
        .loc 26 251 9 view .LVU48389
        movl    %eax, 32(%rsp)                      ; store index to rsp+32

...

.LBE54228:
.LBE54236:
        .loc 26 251 9 is_stmt 1 discriminator 3 view .LVU48407
        xorl    %r8d, %r8d
        cmpl    %r12d, 32(%rsp)                     ; compare the index with m
.LBB54238:
        .loc 3 356 44 is_stmt 0 discriminator 3 view .LVU48408
        movq    (%rdx), %r13
.LBE54238:
.LBE54237:
        .loc 26 251 9 discriminator 3 view .LVU48409
        sete    %r8b

...

.LBE54245:
.LBE54248:
        .loc 26 251 9 is_stmt 1 discriminator 3 view .LVU48487
        .loc 26 251 9 discriminator 3 view .LVU48488
        cmpl    $16, %r12d
        jne     .L697                                ; end of for (m = 1 ; ...) loop
        movq    %rax, 440(%rsp)
        movq    8(%rsp), %rsi
        movq    (%rsp), %rax
        movq    %r10, 408(%rsp)
        movq    %rbp, 416(%rsp)
        movq    %rax, 368(%rsp)
        .loc 26 251 9 view .LVU48489
.LVL6784:
.LBB54249:
.LBI54249:
        .loc 8 269 30 view .LVU48490
        .loc 8 271 5 view .LVU48491
.LBB54250:
.LBI54250:
        .loc 8 237 30 view .LVU48492
.LBB54251:
        .loc 8 253 5 view .LVU48493
        .loc 8 254 5 is_stmt 0 view .LVU48494
        movq    64(%rsp), %rax
        movq    %r9, 424(%rsp)
        movq    %rbx, 432(%rsp)
        movq    %rsi, 400(%rsp)
        movq    %r11, 392(%rsp)
        movq    %r14, 384(%rsp)
        movq    %r15, 376(%rsp)
        movl    $0, 148(%rsp)                       ; rsp+148 gets cleared, not rsp+32

It feels a bit weird that GCC is not clearing the right 32-bit stack location, and this might be a bug in an old version of GCC, or an error in my analysis of the assembly code.

Anyway, I don't have much more time to spend on debugging this and if you want to clear index, please go ahead :)

real-or-random commented at 6:44 AM on July 29, 2026:

Wow, thanks for digging that deep. So we should check that this does what it's supposed to do when compiled with GCC,.

But that's an orthogonal concern and probably should not hold up adding secp256k1_memclear_explicit(&index, sizeof(index));. In the worst case, it doesn't do anything, but it doesn't make the code worse.

niooss-ledger commented at 1:20 PM on July 29, 2026:

I pushed a new commit, adding secp256k1_memclear_explicit(&index, sizeof(index));. Please let me know if you want me to rebase this Pull Request :)

real-or-random commented at 8:36 AM on July 28, 2026: contributor

Concept ACK

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.

f272556953

niooss-ledger force-pushed on Jul 29, 2026

theStack approved

theStack commented at 5:26 PM on July 29, 2026: contributor

ACK f2725569533cad027c78bd3435933007cbec898d

theStack removed this from the milestone 0.8.0 on Aug 3, 2026

theStack added this to the milestone 0.8.1 on Aug 3, 2026

IsaqueFranklin commented at 12:20 AM on August 20, 2026: none

I went through the entire ecmult_const code in detail, including the GLV/signed-digit stuff and the handling of those temporary variables we are dealing with, and the proposed clearings. It seems very reasonable to me that clearing everything that could hold "sensitive data" here is a valid approach and makes sense as a "best-effort thing". I also ran bench_ecmult on macOS arm64 and did not notice a substantial performance regression.

ecmult_const PR benchmark: 11.8 us ecmult_const master benchmark: 11.7 us

IsaqueFranklin approved

IsaqueFranklin commented at 12:27 AM on August 20, 2026: none

ACK f2725569533cad027c78bd3435933007cbec898d

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