Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1170: contexts: Forbid destroying, cloni…
Browse files Browse the repository at this point in the history 
…ng and randomizing the static context

e39d954 tests: Add CHECK_ILLEGAL(_VOID) macros and use in static ctx tests (Tim Ruffing)
61841fc contexts: Forbid randomizing secp256k1_context_static (Tim Ruffing)
4b6df5e contexts: Forbid cloning/destroying secp256k1_context_static (Tim Ruffing)

Pull request description:

  As discussed in bitcoin#1126.

  For randomization, this has a history. Initially, this threw the illegal callback but then we changed it to be a no-op on non-signing contexts: bitcoin-core/secp256k1@6198375 But this was with (non-static) none/verification contexts in mind, not with the static context. If we anyway forbid cloning the static context, you should never a way to randomize a copy of the static context. (You need a copy because the static context itself is not writable. But you cannot obtain a copy except when using memcpy etc.)

ACKs for top commit:
  sipa:
    utACK e39d954
  apoelstra:
    ACK e39d954

Tree-SHA512: dc804b15652d536b5d67db7297ac0e65eab3a64cbb35a9856329cb87e7ea0fe8ea733108104b3bba580077fe03d6ad6b161c797cf866a74722bab7849f0bb60c
  • Loading branch information
@sipa
sipa committed Jan 19, 2023
2 parents 233822d + e39d954 commit 5fbff5d
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 41 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 11 additions & 11 deletions include/secp256k1.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
8 changes: 6 additions & 2 deletions include/secp256k1_preallocated.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
30 changes: 22 additions & 8 deletions src/secp256k1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -163,24 +164,35 @@ 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);
return ret;
}

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) {
Expand Down Expand Up @@ -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);
}
Expand Down
132 changes: 112 additions & 20 deletions src/tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)++;
}

Expand All @@ -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)--;
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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 */
Expand Down

0 comments on commit 5fbff5d

Please sign in to comment.