This has been not been caught by the new constant-time tests because valgrind currently gives us a zero exit code even if finds errors, see #723 (review) .
Note that the timing leak here was the bit whether a secret key was out of range. This leak is harmless and not exploitable. It is just our overcautious practice to prefer constant-time code even here.
Here’s the “failure” on the current master: https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/666399947#L462
170+ volatile int *flagp = &flag;
171+ unsigned char mask = -(unsigned char) *flagp;
172 p = (unsigned char *)s;
173 while (len) {
174- *p ^= *p & mask;
175+ *p &= ~mask;
For completeness, this was the code generated by clang:
0memczero():
1src/util.h:169
2static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
3 unsigned char *p;
4 unsigned char mask = -(unsigned char)flag;
5 p = (unsigned char *)s;
6 while (len) {
7 *p ^= *p & mask;
8 20405: 0f 10 5b 10 movups 0x10(%rbx),%xmm3
9 20409: 0f 10 53 20 movups 0x20(%rbx),%xmm2
10 2040d: 0f 10 4b 30 movups 0x30(%rbx),%xmm1
11 20411: 0f 57 c0 xorps %xmm0,%xmm0
12 20414: 74 05 je 2041b <secp256k1_ec_pubkey_create+0x15b>
13 20416: 0f 57 e4 xorps %xmm4,%xmm4
14 20419: eb 03 jmp 2041e <secp256k1_ec_pubkey_create+0x15e>
15 2041b: 0f 10 23 movups (%rbx),%xmm4
16 2041e: 0f 11 23 movups %xmm4,(%rbx)
17 20421: 48 8b 04 24 mov (%rsp),%rax
18 20425: 75 42 jne 20469 <secp256k1_ec_pubkey_create+0x1a9>
19 20427: 0f 11 5b 10 movups %xmm3,0x10(%rbx)
20 2042b: 75 45 jne 20472 <secp256k1_ec_pubkey_create+0x1b2>
21 2042d: 0f 11 53 20 movups %xmm2,0x20(%rbx)
22 20431: 74 03 je 20436 <secp256k1_ec_pubkey_create+0x176>
23 20433: 0f 57 c9 xorps %xmm1,%xmm1
24 20436: 0f 11 4b 30 movups %xmm1,0x30(%rbx)
This shows that our tests are essentially working. :+1: Except the exit value which will be fixed in #723.
162@@ -163,10 +163,14 @@ SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;
163 /* Zero memory if flag == 1. Constant time. */
164 static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
165 unsigned char *p;
166- unsigned char mask = -(unsigned char)flag;
167+ /* Access flag with a volatile-qualified lvalue.
168+ This prevents clang from figuring out (after inlining) that flag can
169+ take only be 0 or 1, which leads to variable time code. */
170+ volatile int *flagp = &flag;
flag argument volatile int?
This works too but I find volatile function parameters very obscure. Some C++ people agree with me: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html#parret
Also the placement of the comment is more natural with this version.
volatile int instead of pointer, and copy flag into it? Otherwise flagp is declared as a pointer to volatile int, but does not in fact point to a volatile int.
I changed it to your suggestion. (I didn’t do this previously because I wrongly assumed it needs one more store).
Otherwise flagp is declared as a pointer to
volatile int, but does not in fact point to avolatile int.
When working on this PR I learned the truth about volatile. While the C standards speaks about “volatile objects”, what the authors meant and luckily what every compiler implemented are “volatile lvalues” [1]. In other words, being volatile is a property of an access to an object, not a property of the object itself. So both variants with and without pointer are equally good in practice but the one without pointer is simpler and indeed closer the letter of the standard.
[1] Defect report 476. http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_476
This has been not been caught by the new constant-time tests because
valgrind currently gives us a zero exit code even if finds errors, see
https://github.com/bitcoin-core/secp256k1/pull/723#discussion_r388246806 .
This commit also simplifies the arithmetic in memczero.
Note that the timing leak here was the bit whether a secret key was
out of range. This leak is harmless and not exploitable. It is just
our overcautious practice to prefer constant-time code even here.