From 7b50483ad789081ba158799e5b94330f62932607 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sat, 11 Jan 2020 13:31:50 +0000 Subject: [PATCH] Adds a declassify operation to aid constant-time analysis. ECDSA signing has a retry loop for the exceptionally unlikely case that S==0. S is not a secret at this point and this case is so rare that it will never be observed but branching on it will trip up tools analysing if the code is constant time with respect to secrets. Derandomized ECDSA can also loop on k being zero or overflowing, and while k is a secret these cases are too rare (1:2^255) to ever observe and are also of no concern. This adds a function for marking memory as no-longer-secret and sets it up for use with the valgrind memcheck constant-time test. --- .travis.yml | 12 +++--------- Makefile.am | 4 ++++ configure.ac | 3 +++ include/secp256k1.h | 2 ++ src/secp256k1.c | 33 ++++++++++++++++++++++++++++++--- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/.travis.yml b/.travis.yml index a1f247021a0ba..da4254cb9f8f2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,9 @@ language: c os: linux addons: apt: - packages: libgmp-dev + packages: + - libgmp-dev + - valgrind compiler: - clang - gcc @@ -60,18 +62,10 @@ matrix: env: - BIGNUM=no ENDOMORPHISM=yes ASM=x86_64 EXPERIMENTAL=yes ECDH=yes RECOVERY=yes - VALGRIND=yes EXTRAFLAGS="--disable-openssl-tests CPPFLAGS=-DVALGRIND" BUILD= - addons: - apt: - packages: - - valgrind - compiler: gcc env: # The same as above but without endomorphism. - BIGNUM=no ENDOMORPHISM=no ASM=x86_64 EXPERIMENTAL=yes ECDH=yes RECOVERY=yes - VALGRIND=yes EXTRAFLAGS="--disable-openssl-tests CPPFLAGS=-DVALGRIND" BUILD= - addons: - apt: - packages: - - valgrind before_script: ./autogen.sh diff --git a/Makefile.am b/Makefile.am index de56754bb2111..79dcf6e2f7c87 100644 --- a/Makefile.am +++ b/Makefile.am @@ -69,6 +69,10 @@ libsecp256k1_la_SOURCES = src/secp256k1.c libsecp256k1_la_CPPFLAGS = -DSECP256K1_BUILD -I$(top_srcdir)/include -I$(top_srcdir)/src $(SECP_INCLUDES) libsecp256k1_la_LIBADD = $(SECP_LIBS) $(COMMON_LIB) +if VALGRIND_ENABLED +libsecp256k1_la_CPPFLAGS += -DVALGRIND +endif + noinst_PROGRAMS = if USE_BENCHMARK noinst_PROGRAMS += bench_verify bench_sign bench_internal bench_ecmult diff --git a/configure.ac b/configure.ac index 50210405cd345..a2023aba43611 100644 --- a/configure.ac +++ b/configure.ac @@ -170,6 +170,9 @@ AC_ARG_WITH([ecmult-gen-precision], [AS_HELP_STRING([--with-ecmult-gen-precision AC_CHECK_TYPES([__int128]) +AC_CHECK_HEADER([valgrind/memcheck.h], [enable_valgrind=yes], [enable_valgrind=no], []) +AM_CONDITIONAL([VALGRIND_ENABLED],[test "$enable_valgrind" = "yes"]) + if test x"$enable_coverage" = x"yes"; then AC_DEFINE(COVERAGE, 1, [Define this symbol to compile out all VERIFY code]) CFLAGS="$CFLAGS -O0 --coverage" diff --git a/include/secp256k1.h b/include/secp256k1.h index 36020e5164677..1678406610e68 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -162,12 +162,14 @@ typedef int (*secp256k1_nonce_function)( /** The higher bits contain the actual data. Do not use directly. */ #define SECP256K1_FLAGS_BIT_CONTEXT_VERIFY (1 << 8) #define SECP256K1_FLAGS_BIT_CONTEXT_SIGN (1 << 9) +#define SECP256K1_FLAGS_BIT_CONTEXT_DECLASSIFY (1 << 10) #define SECP256K1_FLAGS_BIT_COMPRESSION (1 << 8) /** Flags to pass to secp256k1_context_create, secp256k1_context_preallocated_size, and * secp256k1_context_preallocated_create. */ #define SECP256K1_CONTEXT_VERIFY (SECP256K1_FLAGS_TYPE_CONTEXT | SECP256K1_FLAGS_BIT_CONTEXT_VERIFY) #define SECP256K1_CONTEXT_SIGN (SECP256K1_FLAGS_TYPE_CONTEXT | SECP256K1_FLAGS_BIT_CONTEXT_SIGN) +#define SECP256K1_CONTEXT_DECLASSIFY (SECP256K1_FLAGS_TYPE_CONTEXT | SECP256K1_FLAGS_BIT_CONTEXT_DECLASSIFY) #define SECP256K1_CONTEXT_NONE (SECP256K1_FLAGS_TYPE_CONTEXT) /** Flag to pass to secp256k1_ec_pubkey_serialize. */ diff --git a/src/secp256k1.c b/src/secp256k1.c index de4eea36816fa..c10ac4cda546a 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -20,6 +20,10 @@ #include "hash_impl.h" #include "scratch_impl.h" +#if defined(VALGRIND) +# include +#endif + #define ARG_CHECK(cond) do { \ if (EXPECT(!(cond), 0)) { \ secp256k1_callback_call(&ctx->illegal_callback, #cond); \ @@ -66,13 +70,15 @@ struct secp256k1_context_struct { secp256k1_ecmult_gen_context ecmult_gen_ctx; secp256k1_callback illegal_callback; secp256k1_callback error_callback; + int declassify; }; static const secp256k1_context secp256k1_context_no_precomp_ = { { 0 }, { 0 }, { secp256k1_default_illegal_callback_fn, 0 }, - { secp256k1_default_error_callback_fn, 0 } + { secp256k1_default_error_callback_fn, 0 }, + 0 }; const secp256k1_context *secp256k1_context_no_precomp = &secp256k1_context_no_precomp_; @@ -132,6 +138,7 @@ secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigne if (flags & SECP256K1_FLAGS_BIT_CONTEXT_VERIFY) { secp256k1_ecmult_context_build(&ret->ecmult_ctx, &prealloc); } + ret->declassify = !!(flags & SECP256K1_FLAGS_BIT_CONTEXT_DECLASSIFY); return (secp256k1_context*) ret; } @@ -215,6 +222,20 @@ void secp256k1_scratch_space_destroy(const secp256k1_context *ctx, secp256k1_scr secp256k1_scratch_destroy(&ctx->error_callback, scratch); } +/* Mark memory as no-longer-secret for the purpose of analysing constant-time behaviour + * of the software. This is setup for use with valgrind but could be substituted with + * the appropriate instrumentation for other analysis tools. + */ +static SECP256K1_INLINE void secp256k1_declassify(const secp256k1_context* ctx, void *p, size_t len) { +#if defined(VALGRIND) + if (EXPECT(ctx->declassify,0)) VALGRIND_MAKE_MEM_DEFINED(p, len); +#else + (void)ctx; + (void)p; + (void)len; +#endif +} + static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge, const secp256k1_pubkey* pubkey) { if (sizeof(secp256k1_ge_storage) == 64) { /* When the secp256k1_ge_storage type is exactly 64 byte, use its @@ -474,8 +495,14 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature break; } secp256k1_scalar_set_b32(&non, nonce32, &koverflow); - if (!koverflow && !secp256k1_scalar_is_zero(&non)) { - if (secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, NULL)) { + koverflow |= secp256k1_scalar_is_zero(&non); + /* The nonce is still secret here, but it overflowing or being zero is is less likely than 1:2^255. */ + secp256k1_declassify(ctx, &koverflow, sizeof(koverflow)); + if (!koverflow) { + ret = secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, NULL); + /* The final signature is no longer a secret, nor is the fact that we were successful or not. */ + secp256k1_declassify(ctx, &ret, sizeof(ret)); + if (ret) { break; } }