Constant-time behaviour test using valgrind memtest.

[gmaxwell]

Valgrind does bit-level tracking of the "uninitialized" status of memory,
property tracks memory which is tainted by any uninitialized memory, and
warns if any branch or array access depends on an uninitialized bit.

That is exactly the verification we need on secret data to test for
constant-time behaviour. All we need to do is tell valgrind our
secret key is actually uninitialized memory.

This adds a valgrind_ctime_test which is compiled if valgrind is installed:

Run it with libtool --mode=execute:
$ libtool --mode=execute valgrind ./valgrind_ctime_test

[gmaxwell]

Not completely yet mostly because it finds real but irrelevant non-constant time behaviour!

I used this approach a couple years ago to originally test the constant time keygen/signing code-- I'm not sure if back then it didn't have checks on the keys being in-range or if I just ignored these because they're irrelevant.

Manually ignoring them isn't an option for a CI runnable shipped test, however.

It could be suppressed via valgrind suppressions, but I'd actually prefer to fix it if it can be done without making too much of a mess. E.g. if the key is invalid, cmov in a dummy key to sign with then at the end cmov in an empty signature.

Valgrind suppressions would have a disadvantage of needing to be kept in sync with the code. I also believe the presence of them weakens the valgrind tracing somewhat (the result of the suppressed branch isn't taint tracked, I think) and could hide issues. (plus, I personally feel like suppressions send a signal of low quality work)

[gmaxwell]

Here is the current output:

==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485B060: secp256k1_ec_pubkey_create (secp256k1.c:521)
==84274==    by 0x401214: main (valgrind_ctime_test.c:39)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485B08C: secp256k1_ec_pubkey_create (secp256k1.c:521)
==84274==    by 0x401214: main (valgrind_ctime_test.c:39)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485A98D: secp256k1_ecdsa_sign (secp256k1.c:465)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485A9AF: secp256k1_ecdsa_sign (secp256k1.c:465)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485AA66: secp256k1_ecdsa_sign (secp256k1.c:475)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485AA88: secp256k1_ecdsa_sign (secp256k1.c:475)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485AD9D: secp256k1_ecdsa_sig_sign (ecdsa_impl.h:307)
==84274==    by 0x485AD9D: secp256k1_ecdsa_sign (secp256k1.c:476)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 
==84274== Conditional jump or move depends on uninitialised value(s)
==84274==    at 0x485AE20: secp256k1_ecdsa_sig_sign (ecdsa_impl.h:310)
==84274==    by 0x485AE20: secp256k1_ecdsa_sign (secp256k1.c:476)
==84274==    by 0x40134B: main (valgrind_ctime_test.c:45)
==84274== 

[gmaxwell]

Here is a demonstration that it can find real bugs-- e.g.

--- a/src/ecmult_gen_impl.h
+++ b/src/ecmult_gen_impl.h
@@ -152 +152 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25
-        secp256k1_gej_add_ge(r, r, &add);
+        secp256k1_gej_add_ge_var(r, r, &add, NULL);

==84996== Conditional jump or move depends on uninitialised value(s)
==84996==    at 0x484AAB9: secp256k1_fe_normalizes_to_zero_var (field_5x52_impl.h:206)
==84996==    by 0x484FDDC: secp256k1_gej_add_ge_var.constprop.0 (group_impl.h:443)
==84996==    by 0x48503F3: secp256k1_ecmult_gen (ecmult_gen_impl.h:152)
==84996==    by 0x4859152: secp256k1_ecdsa_sig_sign (ecdsa_impl.h:284)
==84996==    by 0x4859152: secp256k1_ecdsa_sign (secp256k1.c:476)
==84996==    by 0x40134B: main (valgrind_ctime_test.c:45)

[gmaxwell]

And a demonstration that it catches secrets in array indexes:

--- a/src/ecmult_gen_impl.h
+++ b/src/ecmult_gen_impl.h
@@ -137,0 +138 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25
+        adds = (*ctx->prec)[j][bits];
@@ -149 +150 @@ static void secp256k1_ecmult_gen(const secp256k1_ecmult_gen_context *ctx, secp25
-            secp256k1_ge_storage_cmov(&adds, &(*ctx->prec)[j][i], i == bits);
+            /*secp256k1_ge_storage_cmov(&adds, &(*ctx->prec)[j][i], i == bits);*/

==86172== Use of uninitialised value of size 8
==86172==    at 0x4850C3E: secp256k1_ecmult_gen (ecmult_gen_impl.h:138)
==86172==    by 0x485AD12: secp256k1_ecdsa_sig_sign (ecdsa_impl.h:284)
==86172==    by 0x485AD12: secp256k1_ecdsa_sign (secp256k1.c:476)
==86172==    by 0x40134B: main (valgrind_ctime_test.c:45)

[gmaxwell]

So one possibility for the s==0 loop in signing would be to add a VALGRIND_MAKE_MEM_DEFINED to libsecp256k1.c on the result of the is_zero test. I think that would be cleanest.

E.g. if the library is compile with valgrind headers available, it could make_defined the result of the zero comparison. This behaviour could even be controlled by a flag stored in the context, so as to not diminish test sensitivity outside of CT tests. The valgrind macros add ~6 instructions to the code... and this would not be inside a performance critical loop.

The overflow case I think should just be handled by running on dummy data.

[gmaxwell]

See #709.

[gmaxwell]

todo: freeking valgrind m4 script doesn't actually check if the valgrind headers are available.

[gmaxwell]

The integer divide instruction is variable time essentially everywhere. Normally division by a constant gets converted into something else by the compiler, but not every compiler or every optimization does in every case.

In ECDH there was an unsigned /2 where GCC emits an idiv when compiled with -Os. I modified valgrind memcheck to throw an error on undefined input into divisions and this successfully detects the case in ECDH and none elsewhere in the code.

Unfortunately depending on a modified valgrind starts creating the same problem that other constant time analysis tools have-- they're hard to keep working. This PR is still highly useful without this functionality, I just wanted to make a note of this limitation and the fact that it's not particularly hard to work around.

[gmaxwell]

I updated with the ECDH and randomization tests.

[gmaxwell]

GCC 9.2.1 on POWER9 emits a lot of branches for carries in the 32-bit scalar code. :( The issue seems to be similar to the one in the ECDH code. Those comparisons aren't reliably turned into constant time assembly.

[practicalswift]

Concept ACK

Very nice and thanks for implementing!

DJB demoed this clever technique in the "High-assurance crypto software" talk at 36C3:

[sipa]

@practicalswift Yes, that's where we learned about it.

[gmaxwell]

@sipa no it isn't. I first used it on libsecp256k1 years ago (long before the ECDH module was written), you just don't remember.

https://moderncrypto.org/mail-archive/curves/2015/000641.html

I was unaware of the video/talk. I'll check it out.

This PR was created because I was surprised that no one had tested the ECDH code but were talking about making it non-experimental and thought that packaging up the test would be the only way to make it likely that anyone would bother testing in the future.

[sipa]

@gmaxwell Ah, interesting coincidence in that case.

[sipa]

Concept and code review ACK. What is still WIP about this?

[gmaxwell]

I added tests for the remaining seckey operations. I also changed it to just check for the memcheck header directly instead of including the big valgrind m4 macro that didn't actually check for the header. I think this is done now, but it probably doesn't make sense to merge it until it tests clean (so as to not confuse someone doing a bisection). See #710.

[gmaxwell]

@practicalswift Looks like from the video that DJB may be unaware of the memcheck macros. The issue with the approach he's using is that (1) valgrind can incorrectly think the memory is initialized, particularly its a stack variable, and (2) the compiler may be smart enough to realize the memory is uninitialized and optimize in ways that undermine the tests. Use of the memcheck macros addresses both of those issues.

[real-or-random]

Do you think it makes sense to add tests for the tweak functions?
The secret key is clearly a secret in those function but I supposed they should be constant-time in also the tweak?

edit: This could also be done in a second PR of course.

[gmaxwell]

@real-or-random I did three days ago.

(also fixes to them in the other PR)

[real-or-random]

@real-or-random I did three days ago.

Nevermind, I missed it.

[gmaxwell]

I rebased this on #710 because I intend for it to be merged after.

[jonasnick]

obvious concept ACK is obious

How about we add this to make check? That way travis would also run it.

valgrind_ctime_test should be in .gitignore

[gmaxwell]

It's not clear to me how to override the TESTS_ENVIROMENT for only a single test w/ make check, and I doubt we want to make all the tests run in valgrind by default (a make check-valgrind however would be fine).

[gmaxwell]

FWIW, the tests passes on 32-bit arm with and without asm for me. Though I note that when openssl tests are enable all the test libraries are getting linked to libcrypto and that makes the old version of valgrind I have on my 32bit arm die with an unknown instruction error.

[jonasnick]

It's not clear to me how to override the TESTS_ENVIROMENT for only a single test w/ make check

In that case I'm fine with leaving as is for now. If make check-valgrind runs everything make check under valgrind it's too inflexible because the valgrind_ctime_test finishes much faster than valground tests. I created a commit that runs valgrind_ctime_test in all travis test configurations (excluding BUILD=distcheck) jonasnick#12. Feel free to cherry-pick.

Would be helpful to mention valgrind_ctime_test in the README.

[jonasnick]

ACK 2da822d

[real-or-random]

@gmaxwell We forgot secp256k1_ecdsa_sign_recoverable here. Do you want to open a second PR for this?
https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/main_impl.h#L123

I think the code is (or now: was) very similar to the ordinary signing. I wonder if we should move more of the common code to ecdsa_impl.h and make the public API functions just very thin wrappers. This avoids code duplication, and without code duplication we probably wouldn't have missed secp256k1_ecdsa_sign_recoverable here.

[jonasnick]

@real-or-random abstracting away the common code of sign and sign_recoverable is also useful for sign-to-contract. The PR even contains what you're suggesting ab4fd52.

[gmaxwell]

@real-or-random Seems I missed it, sorry. :( I don't think I have the time or energy to see another PR through to completion here right now. Exploiting a refactor that increases code sharing sounds like a good idea.

[prusnak]

Just a side note to give a broader picture to the topic: these tests are great, but they don't guarantee the execution is really constant time. This depends on the processor implementation.

For example, ARM Cortex-M3 does not have a constant-time multiply instruction: it takes 1-2 cycles less if both multiplicands fit into 16 bites, or either multiplicand is 0, or either multiplicand is 2^n. ARM Cortex-M4 is not affected.

More info about different architectures: https://www.bearssl.org/ctmul.html

[elichai]

Just a side note to give a broader picture to the topic: these tests are great, but they don't guarantee the execution is really constant time. This depends on the processor implementation.

Yep. same in MSVC: #711 altough that's at the OS level, but yeah there's also the CPU level, but that's quite harder to check and fix

[gmaxwell]

prusnak: indeed, we were aware. It's not terribly difficult to modify valgrind to consider whatever conditions you want (I used a modified version that also catches div/mod non-constant timeness, for example). Getting constant time behaviour on that arm m3 stuff though seems like a performance killing nightmare...

[real-or-random]

@prusnak Indeed we're aware but it seems very hard to protect against this. I'm curious if you had implemented any countermeasures against this on Cortex-M3 (before the switch to our library here)?

[prusnak]

For us, this is not a big problem because you can't do signing in a non-interactive way and you need to enter the PIN on boot anyway.