Clear scalar variables computed from scalar in secp256k1_ecmult_const

[niooss-ledger]

Hello,
Function secp256k1_ecdh performs an ECDH computation and clears some intermediate values from the stack before returning:

secp256k1/src/modules/ecdh/main_impl.h

Lines 70 to 74 in 68b45fd

	secp256k1_memclear_explicit(x, sizeof(x));
	secp256k1_memclear_explicit(y, sizeof(y));
	secp256k1_scalar_clear(&s);
	secp256k1_ge_clear(&pt);
	secp256k1_gej_clear(&res);

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.

[real-or-random]

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]

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.

[theStack]

Concept ACK

[theStack]

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]

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]

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