diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a13a39991f57..62a89f83c50f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +#### Changed + - Forbade cloning or destroying `secp256k1_context_static`. Create a new context instead of cloning the static context. (If this change breaks your code, your code is probably wrong.) + - Forbade randomizing (copies of) `secp256k1_context_static`. Randomizing a copy of `secp256k1_context_static` did not have any effect and did not provide defense-in-depth protection against side-channel attacks. Create a new context if you want to benefit from randomization. + ## [0.2.0] - 2022-12-12 #### Added diff --git a/include/secp256k1.h b/include/secp256k1.h index 3d169ecce20c2..3a75b05000c3e 100644 --- a/include/secp256k1.h +++ b/include/secp256k1.h @@ -291,8 +291,11 @@ SECP256K1_API secp256k1_context* secp256k1_context_create( * called at most once for every call of this function. If you need to avoid dynamic * memory allocation entirely, see the functions in secp256k1_preallocated.h. * + * Cloning secp256k1_context_static is not possible, and should not be emulated by + * the caller (e.g., using memcpy). Create a new context instead. + * * Returns: a newly created context object. - * Args: ctx: an existing context to copy + * Args: ctx: an existing context to copy (not secp256k1_context_static) */ SECP256K1_API secp256k1_context* secp256k1_context_clone( const secp256k1_context* ctx @@ -310,6 +313,7 @@ SECP256K1_API secp256k1_context* secp256k1_context_clone( * * Args: ctx: an existing context to destroy, constructed using * secp256k1_context_create or secp256k1_context_clone + * (i.e., not secp256k1_context_static). */ SECP256K1_API void secp256k1_context_destroy( secp256k1_context* ctx @@ -820,10 +824,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_tweak_mul( /** Randomizes the context to provide enhanced protection against side-channel leakage. * - * Returns: 1: randomization successful (or called on copy of secp256k1_context_static) + * Returns: 1: randomization successful * 0: error - * Args: ctx: pointer to a context object. - * In: seed32: pointer to a 32-byte random seed (NULL resets to initial state) + * Args: ctx: pointer to a context object (not secp256k1_context_static). + * In: seed32: pointer to a 32-byte random seed (NULL resets to initial state). * * While secp256k1 code is written and tested to be constant-time no matter what * secret values are, it is possible that a compiler may output code which is not, @@ -838,21 +842,17 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_tweak_mul( * functions that perform computations involving secret keys, e.g., signing and * public key generation. It is possible to call this function more than once on * the same context, and doing so before every few computations involving secret - * keys is recommended as a defense-in-depth measure. + * keys is recommended as a defense-in-depth measure. Randomization of the static + * context secp256k1_context_static is not supported. * * Currently, the random seed is mainly used for blinding multiplications of a * secret scalar with the elliptic curve base point. Multiplications of this * kind are performed by exactly those API functions which are documented to - * require a context that is not the secp256k1_context_static. As a rule of thumb, + * require a context that is not secp256k1_context_static. As a rule of thumb, * these are all functions which take a secret key (or a keypair) as an input. * A notable exception to that rule is the ECDH module, which relies on a different * kind of elliptic curve point multiplication and thus does not benefit from * enhanced protection against side-channel leakage currently. - * - * It is safe to call this function on a copy of secp256k1_context_static in writable - * memory (e.g., obtained via secp256k1_context_clone). In that case, this - * function is guaranteed to return 1, but the call will have no effect because - * the static context (or a copy thereof) is not meant to be randomized. */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_context_randomize( secp256k1_context* ctx, diff --git a/include/secp256k1_preallocated.h b/include/secp256k1_preallocated.h index ed846f75f9de6..ffa96dd339f7b 100644 --- a/include/secp256k1_preallocated.h +++ b/include/secp256k1_preallocated.h @@ -88,8 +88,11 @@ SECP256K1_API size_t secp256k1_context_preallocated_clone_size( * the lifetime of this context object, see the description of * secp256k1_context_preallocated_create for details. * + * Cloning secp256k1_context_static is not possible, and should not be emulated by + * the caller (e.g., using memcpy). Create a new context instead. + * * Returns: a newly created context object. - * Args: ctx: an existing context to copy. + * Args: ctx: an existing context to copy (not secp256k1_context_static). * In: prealloc: a pointer to a rewritable contiguous block of memory of * size at least secp256k1_context_preallocated_size(flags) * bytes, as detailed above. @@ -117,7 +120,8 @@ SECP256K1_API secp256k1_context* secp256k1_context_preallocated_clone( * * Args: ctx: an existing context to destroy, constructed using * secp256k1_context_preallocated_create or - * secp256k1_context_preallocated_clone. + * secp256k1_context_preallocated_clone + * (i.e., not secp256k1_context_static). */ SECP256K1_API void secp256k1_context_preallocated_destroy( secp256k1_context* ctx diff --git a/src/secp256k1.c b/src/secp256k1.c index 6c91a76119946..7af333ca900fd 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -109,9 +109,9 @@ size_t secp256k1_context_preallocated_size(unsigned int flags) { } size_t secp256k1_context_preallocated_clone_size(const secp256k1_context* ctx) { - size_t ret = sizeof(secp256k1_context); VERIFY_CHECK(ctx != NULL); - return ret; + ARG_CHECK(secp256k1_context_is_proper(ctx)); + return sizeof(secp256k1_context); } secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigned int flags) { @@ -152,6 +152,7 @@ secp256k1_context* secp256k1_context_preallocated_clone(const secp256k1_context* secp256k1_context* ret; VERIFY_CHECK(ctx != NULL); ARG_CHECK(prealloc != NULL); + ARG_CHECK(secp256k1_context_is_proper(ctx)); ret = (secp256k1_context*)prealloc; *ret = *ctx; @@ -163,6 +164,8 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { size_t prealloc_size; VERIFY_CHECK(ctx != NULL); + ARG_CHECK(secp256k1_context_is_proper(ctx)); + prealloc_size = secp256k1_context_preallocated_clone_size(ctx); ret = (secp256k1_context*)checked_malloc(&ctx->error_callback, prealloc_size); ret = secp256k1_context_preallocated_clone(ctx, ret); @@ -170,17 +173,26 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { } void secp256k1_context_preallocated_destroy(secp256k1_context* ctx) { - ARG_CHECK_VOID(ctx != secp256k1_context_static); - if (ctx != NULL) { - secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx); + ARG_CHECK_VOID(ctx == NULL || secp256k1_context_is_proper(ctx)); + + /* Defined as noop */ + if (ctx == NULL) { + return; } + + secp256k1_ecmult_gen_context_clear(&ctx->ecmult_gen_ctx); } void secp256k1_context_destroy(secp256k1_context* ctx) { - if (ctx != NULL) { - secp256k1_context_preallocated_destroy(ctx); - free(ctx); + ARG_CHECK_VOID(ctx == NULL || secp256k1_context_is_proper(ctx)); + + /* Defined as noop */ + if (ctx == NULL) { + return; } + + secp256k1_context_preallocated_destroy(ctx); + free(ctx); } void secp256k1_context_set_illegal_callback(secp256k1_context* ctx, void (*fun)(const char* message, void* data), const void* data) { @@ -737,6 +749,8 @@ int secp256k1_ec_pubkey_tweak_mul(const secp256k1_context* ctx, secp256k1_pubkey int secp256k1_context_randomize(secp256k1_context* ctx, const unsigned char *seed32) { VERIFY_CHECK(ctx != NULL); + ARG_CHECK(secp256k1_context_is_proper(ctx)); + if (secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)) { secp256k1_ecmult_gen_blind(&ctx->ecmult_gen_ctx, seed32); } diff --git a/src/tests.c b/src/tests.c index 9cc4d1763a5f8..bc5b7cb1f1d05 100644 --- a/src/tests.c +++ b/src/tests.c @@ -32,11 +32,43 @@ static int COUNT = 64; static secp256k1_context *CTX = NULL; static secp256k1_context *STATIC_CTX = NULL; +static int all_bytes_equal(const void* s, unsigned char value, size_t n) { + const unsigned char *p = s; + size_t i; + + for (i = 0; i < n; i++) { + if (p[i] != value) { + return 0; + } + } + return 1; +} + +/* TODO Use CHECK_ILLEGAL(_VOID) everywhere and get rid of the uncounting callback */ +/* CHECK that expr_or_stmt calls the illegal callback of ctx exactly once + * + * For checking functions that use ARG_CHECK_VOID */ +#define CHECK_ILLEGAL_VOID(ctx, expr_or_stmt) do { \ + int32_t _calls_to_illegal_callback = 0; \ + secp256k1_callback _saved_illegal_cb = ctx->illegal_callback; \ + secp256k1_context_set_illegal_callback(ctx, \ + counting_illegal_callback_fn, &_calls_to_illegal_callback); \ + { expr_or_stmt; } \ + ctx->illegal_callback = _saved_illegal_cb; \ + CHECK(_calls_to_illegal_callback == 1); \ +} while(0); + +/* CHECK that expr calls the illegal callback of ctx exactly once and that expr == 0 + * + * For checking functions that use ARG_CHECK */ +#define CHECK_ILLEGAL(ctx, expr) CHECK_ILLEGAL_VOID(ctx, CHECK((expr) == 0)) + static void counting_illegal_callback_fn(const char* str, void* data) { /* Dummy callback function that just counts. */ int32_t *p; (void)str; p = data; + CHECK(*p != INT32_MAX); (*p)++; } @@ -45,6 +77,7 @@ static void uncounting_illegal_callback_fn(const char* str, void* data) { int32_t *p; (void)str; p = data; + CHECK(*p != INT32_MIN); (*p)--; } @@ -229,31 +262,61 @@ static void run_ec_illegal_argument_tests(void) { secp256k1_context_set_illegal_callback(CTX, NULL, NULL); } -static void run_static_context_tests(void) { - int32_t dummy = 0; - +static void run_static_context_tests(int use_prealloc) { /* Check that deprecated secp256k1_context_no_precomp is an alias to secp256k1_context_static. */ CHECK(secp256k1_context_no_precomp == secp256k1_context_static); - /* check if sizes for cloning are consistent */ - CHECK(secp256k1_context_preallocated_clone_size(STATIC_CTX) >= sizeof(secp256k1_context)); + { + unsigned char seed[32] = {0x17}; - /* Verify that setting and resetting illegal callback works */ - secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &dummy); - CHECK(STATIC_CTX->illegal_callback.fn == counting_illegal_callback_fn); - secp256k1_context_set_illegal_callback(STATIC_CTX, NULL, NULL); - CHECK(STATIC_CTX->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + /* Randomizing secp256k1_context_static is not supported. */ + CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_randomize(STATIC_CTX, seed)); + CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_randomize(STATIC_CTX, NULL)); + + /* Destroying or cloning secp256k1_context_static is not supported. */ + if (use_prealloc) { + CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_preallocated_clone_size(STATIC_CTX)); + { + secp256k1_context *my_static_ctx = malloc(sizeof(*STATIC_CTX)); + CHECK(my_static_ctx != NULL); + memset(my_static_ctx, 0x2a, sizeof(*my_static_ctx)); + CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_preallocated_clone(STATIC_CTX, my_static_ctx)); + CHECK(all_bytes_equal(my_static_ctx, 0x2a, sizeof(*my_static_ctx))); + free(my_static_ctx); + } + CHECK_ILLEGAL_VOID(STATIC_CTX, secp256k1_context_preallocated_destroy(STATIC_CTX)); + } else { + CHECK_ILLEGAL(STATIC_CTX, secp256k1_context_clone(STATIC_CTX)); + CHECK_ILLEGAL_VOID(STATIC_CTX, secp256k1_context_destroy(STATIC_CTX)); + } + } + + { + /* Verify that setting and resetting illegal callback works */ + int32_t dummy = 0; + secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &dummy); + CHECK(STATIC_CTX->illegal_callback.fn == counting_illegal_callback_fn); + CHECK(STATIC_CTX->illegal_callback.data == &dummy); + secp256k1_context_set_illegal_callback(STATIC_CTX, NULL, NULL); + CHECK(STATIC_CTX->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + CHECK(STATIC_CTX->illegal_callback.data == NULL); + } } static void run_proper_context_tests(int use_prealloc) { int32_t dummy = 0; - secp256k1_context *my_ctx; + secp256k1_context *my_ctx, *my_ctx_fresh; void *my_ctx_prealloc = NULL; + unsigned char seed[32] = {0x17}; secp256k1_gej pubj; secp256k1_ge pub; secp256k1_scalar msg, key, nonce; secp256k1_scalar sigr, sigs; + + /* Fresh reference context for comparison */ + my_ctx_fresh = secp256k1_context_create(SECP256K1_CONTEXT_NONE); + if (use_prealloc) { my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(my_ctx_prealloc != NULL); @@ -262,6 +325,13 @@ static void run_proper_context_tests(int use_prealloc) { my_ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); } + /* Randomize and reset randomization */ + CHECK(context_eq(my_ctx, my_ctx_fresh)); + CHECK(secp256k1_context_randomize(my_ctx, seed) == 1); + CHECK(!context_eq(my_ctx, my_ctx_fresh)); + CHECK(secp256k1_context_randomize(my_ctx, NULL) == 1); + CHECK(context_eq(my_ctx, my_ctx_fresh)); + /* set error callback (to a function that still aborts in case malloc() fails in secp256k1_context_clone() below) */ secp256k1_context_set_error_callback(my_ctx, secp256k1_default_illegal_callback_fn, NULL); CHECK(my_ctx->error_callback.fn != secp256k1_default_error_callback_fn); @@ -276,16 +346,33 @@ static void run_proper_context_tests(int use_prealloc) { if (use_prealloc) { /* clone into a non-preallocated context and then again into a new preallocated one. */ - ctx_tmp = my_ctx; my_ctx = secp256k1_context_clone(my_ctx); secp256k1_context_preallocated_destroy(ctx_tmp); - free(my_ctx_prealloc); my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(my_ctx_prealloc != NULL); - ctx_tmp = my_ctx; my_ctx = secp256k1_context_preallocated_clone(my_ctx, my_ctx_prealloc); secp256k1_context_destroy(ctx_tmp); + ctx_tmp = my_ctx; + my_ctx = secp256k1_context_clone(my_ctx); + CHECK(context_eq(ctx_tmp, my_ctx)); + secp256k1_context_preallocated_destroy(ctx_tmp); + + free(my_ctx_prealloc); + my_ctx_prealloc = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); + CHECK(my_ctx_prealloc != NULL); + ctx_tmp = my_ctx; + my_ctx = secp256k1_context_preallocated_clone(my_ctx, my_ctx_prealloc); + CHECK(context_eq(ctx_tmp, my_ctx)); + secp256k1_context_destroy(ctx_tmp); } else { /* clone into a preallocated context and then again into a new non-preallocated one. */ void *prealloc_tmp; - prealloc_tmp = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); CHECK(prealloc_tmp != NULL); - ctx_tmp = my_ctx; my_ctx = secp256k1_context_preallocated_clone(my_ctx, prealloc_tmp); secp256k1_context_destroy(ctx_tmp); - ctx_tmp = my_ctx; my_ctx = secp256k1_context_clone(my_ctx); secp256k1_context_preallocated_destroy(ctx_tmp); + prealloc_tmp = malloc(secp256k1_context_preallocated_size(SECP256K1_CONTEXT_NONE)); + CHECK(prealloc_tmp != NULL); + ctx_tmp = my_ctx; + my_ctx = secp256k1_context_preallocated_clone(my_ctx, prealloc_tmp); + CHECK(context_eq(ctx_tmp, my_ctx)); + secp256k1_context_destroy(ctx_tmp); + + ctx_tmp = my_ctx; + my_ctx = secp256k1_context_clone(my_ctx); + CHECK(context_eq(ctx_tmp, my_ctx)); + secp256k1_context_preallocated_destroy(ctx_tmp); free(prealloc_tmp); } } @@ -296,12 +383,16 @@ static void run_proper_context_tests(int use_prealloc) { /* And that it resets back to default. */ secp256k1_context_set_error_callback(my_ctx, NULL, NULL); CHECK(my_ctx->error_callback.fn == secp256k1_default_error_callback_fn); + CHECK(context_eq(my_ctx, my_ctx_fresh)); /* Verify that setting and resetting illegal callback works */ secp256k1_context_set_illegal_callback(my_ctx, counting_illegal_callback_fn, &dummy); CHECK(my_ctx->illegal_callback.fn == counting_illegal_callback_fn); + CHECK(my_ctx->illegal_callback.data == &dummy); secp256k1_context_set_illegal_callback(my_ctx, NULL, NULL); CHECK(my_ctx->illegal_callback.fn == secp256k1_default_illegal_callback_fn); + CHECK(my_ctx->illegal_callback.data == NULL); + CHECK(context_eq(my_ctx, my_ctx_fresh)); /*** attempt to use them ***/ random_scalar_order_test(&msg); @@ -327,6 +418,8 @@ static void run_proper_context_tests(int use_prealloc) { } else { secp256k1_context_destroy(my_ctx); } + secp256k1_context_destroy(my_ctx_fresh); + /* Defined as no-op. */ secp256k1_context_destroy(NULL); secp256k1_context_preallocated_destroy(NULL); @@ -7389,9 +7482,8 @@ int main(int argc, char **argv) { run_selftest_tests(); /* context tests */ - run_proper_context_tests(0); - run_proper_context_tests(1); - run_static_context_tests(); + run_proper_context_tests(0); run_proper_context_tests(1); + run_static_context_tests(0); run_static_context_tests(1); run_deprecated_context_flags_test(); /* scratch tests */