Merge #1170: contexts: Forbid destroying, cloning 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 #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: 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

Author: sipa

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

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,

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

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,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) {
@@ -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);
     }

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 */