Merge bitcoin#755: Recovery signing: add to constant time test, and eliminate non ct operators

real-or-random · real-or-random · commit 2ed54da18add · 2020-06-08T15:45:58.000+02:00
2860950 Add tests for the cmov implementations (Elichai Turkel) 73596a8 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel) 2876af4 Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery (Elichai Turkel) Pull request description: Hi, The recovery module was overlooked in bitcoin#708 and bitcoin#710, so this adds it to the `valgrind_ctime_test` and replaces the secret dependent branching with the cmovs, I created a new function `secp256k1_ecdsa_sign_inner` (feel free to bikeshed) which does the logic both for ecdsa_sign and for ecdsa_sign_recoverable, such that next time when things get changed/improved in ecdsa it will affect the recoverable signing too. ACKs for top commit: jonasnick: ACK 2860950 real-or-random: ACK 2860950 read the diff, tested with valgrind including ctime tests Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59
diff --git a/src/modules/recovery/main_impl.h b/src/modules/recovery/main_impl.h
@@ -122,48 +122,15 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_ecmult_context *ctx, cons
 
 int secp256k1_ecdsa_sign_recoverable(const secp256k1_context* ctx, secp256k1_ecdsa_recoverable_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
     secp256k1_scalar r, s;
-    secp256k1_scalar sec, non, msg;
-    int recid;
-    int ret = 0;
-    int overflow = 0;
+    int ret, recid;
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
     ARG_CHECK(msg32 != NULL);
     ARG_CHECK(signature != NULL);
     ARG_CHECK(seckey != NULL);
-    if (noncefp == NULL) {
-        noncefp = secp256k1_nonce_function_default;
-    }
 
-    secp256k1_scalar_set_b32(&sec, seckey, &overflow);
-    /* Fail if the secret key is invalid. */
-    if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
-        unsigned char nonce32[32];
-        unsigned int count = 0;
-        secp256k1_scalar_set_b32(&msg, msg32, NULL);
-        while (1) {
-            ret = noncefp(nonce32, msg32, seckey, NULL, (void*)noncedata, count);
-            if (!ret) {
-                break;
-            }
-            secp256k1_scalar_set_b32(&non, nonce32, &overflow);
-            if (!overflow && !secp256k1_scalar_is_zero(&non)) {
-                if (secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, &recid)) {
-                    break;
-                }
-            }
-            count++;
-        }
-        memset(nonce32, 0, 32);
-        secp256k1_scalar_clear(&msg);
-        secp256k1_scalar_clear(&non);
-        secp256k1_scalar_clear(&sec);
-    }
-    if (ret) {
-        secp256k1_ecdsa_recoverable_signature_save(signature, &r, &s, recid);
-    } else {
-        memset(signature, 0, sizeof(*signature));
-    }
+    ret = secp256k1_ecdsa_sign_inner(ctx, &r, &s, &recid, msg32, seckey, noncefp, noncedata);
+    secp256k1_ecdsa_recoverable_signature_save(signature, &r, &s, recid);
     return ret;
 }
 
diff --git a/src/secp256k1.c b/src/secp256k1.c
@@ -467,19 +467,18 @@ static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *m
 const secp256k1_nonce_function secp256k1_nonce_function_rfc6979 = nonce_function_rfc6979;
 const secp256k1_nonce_function secp256k1_nonce_function_default = nonce_function_rfc6979;
 
-int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
-    /* Default initialization here is important so we won't pass uninit values to the cmov in the end */
-    secp256k1_scalar r = secp256k1_scalar_zero, s = secp256k1_scalar_zero;
+static int secp256k1_ecdsa_sign_inner(const secp256k1_context* ctx, secp256k1_scalar* r, secp256k1_scalar* s, int* recid, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
     secp256k1_scalar sec, non, msg;
     int ret = 0;
     int is_sec_valid;
     unsigned char nonce32[32];
     unsigned int count = 0;
-    VERIFY_CHECK(ctx != NULL);
-    ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
-    ARG_CHECK(msg32 != NULL);
-    ARG_CHECK(signature != NULL);
-    ARG_CHECK(seckey != NULL);
+    /* Default initialization here is important so we won't pass uninit values to the cmov in the end */
+    *r = secp256k1_scalar_zero;
+    *s = secp256k1_scalar_zero;
+    if (recid) {
+        *recid = 0;
+    }
     if (noncefp == NULL) {
         noncefp = secp256k1_nonce_function_default;
     }
@@ -498,7 +497,7 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
         /* The nonce is still secret here, but it being invalid is is less likely than 1:2^255. */
         secp256k1_declassify(ctx, &is_nonce_valid, sizeof(is_nonce_valid));
         if (is_nonce_valid) {
-            ret = secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, NULL);
+            ret = secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, r, s, &sec, &msg, &non, recid);
             /* 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) {
@@ -515,8 +514,25 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
     secp256k1_scalar_clear(&msg);
     secp256k1_scalar_clear(&non);
     secp256k1_scalar_clear(&sec);
-    secp256k1_scalar_cmov(&r, &secp256k1_scalar_zero, !ret);
-    secp256k1_scalar_cmov(&s, &secp256k1_scalar_zero, !ret);
+    secp256k1_scalar_cmov(r, &secp256k1_scalar_zero, !ret);
+    secp256k1_scalar_cmov(s, &secp256k1_scalar_zero, !ret);
+    if (recid) {
+        const int zero = 0;
+        secp256k1_int_cmov(recid, &zero, !ret);
+    }
+    return ret;
+}
+
+int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
+    secp256k1_scalar r, s;
+    int ret;
+    VERIFY_CHECK(ctx != NULL);
+    ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
+    ARG_CHECK(msg32 != NULL);
+    ARG_CHECK(signature != NULL);
+    ARG_CHECK(seckey != NULL);
+
+    ret = secp256k1_ecdsa_sign_inner(ctx, &r, &s, NULL, msg32, seckey, noncefp, noncedata);
     secp256k1_ecdsa_signature_save(signature, &r, &s);
     return ret;
 }
diff --git a/src/tests.c b/src/tests.c
@@ -3118,7 +3118,7 @@ void test_ecmult_multi_batching(void) {
     data.pt = pt;
     secp256k1_gej_neg(&r2, &r2);
 
-    /* Test with empty scratch space. It should compute the correct result using 
+    /* Test with empty scratch space. It should compute the correct result using
      * ecmult_mult_simple algorithm which doesn't require a scratch space. */
     scratch = secp256k1_scratch_create(&ctx->error_callback, 0);
     CHECK(secp256k1_ecmult_multi_var(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &scG, ecmult_multi_callback, &data, n_points));
@@ -5292,6 +5292,161 @@ void run_memczero_test(void) {
     CHECK(memcmp(buf1, buf2, sizeof(buf1)) == 0);
 }
 
+void int_cmov_test(void) {
+    int r = INT_MAX;
+    int a = 0;
+
+    secp256k1_int_cmov(&r, &a, 0);
+    CHECK(r == INT_MAX);
+
+    r = 0; a = INT_MAX;
+    secp256k1_int_cmov(&r, &a, 1);
+    CHECK(r == INT_MAX);
+
+    a = 0;
+    secp256k1_int_cmov(&r, &a, 1);
+    CHECK(r == 0);
+
+    a = 1;
+    secp256k1_int_cmov(&r, &a, 1);
+    CHECK(r == 1);
+
+    r = 1; a = 0;
+    secp256k1_int_cmov(&r, &a, 0);
+    CHECK(r == 1);
+
+}
+
+void fe_cmov_test(void) {
+    static const secp256k1_fe zero = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 0);
+    static const secp256k1_fe one = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 1);
+    static const secp256k1_fe max = SECP256K1_FE_CONST(
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
+    );
+    secp256k1_fe r = max;
+    secp256k1_fe a = zero;
+
+    secp256k1_fe_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    r = zero; a = max;
+    secp256k1_fe_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    a = zero;
+    secp256k1_fe_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &zero, sizeof(r)) == 0);
+
+    a = one;
+    secp256k1_fe_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+
+    r = one; a = zero;
+    secp256k1_fe_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+}
+
+void fe_storage_cmov_test(void) {
+    static const secp256k1_fe_storage zero = SECP256K1_FE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 0);
+    static const secp256k1_fe_storage one = SECP256K1_FE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 1);
+    static const secp256k1_fe_storage max = SECP256K1_FE_STORAGE_CONST(
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
+    );
+    secp256k1_fe_storage r = max;
+    secp256k1_fe_storage a = zero;
+
+    secp256k1_fe_storage_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    r = zero; a = max;
+    secp256k1_fe_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    a = zero;
+    secp256k1_fe_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &zero, sizeof(r)) == 0);
+
+    a = one;
+    secp256k1_fe_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+
+    r = one; a = zero;
+    secp256k1_fe_storage_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+}
+
+void scalar_cmov_test(void) {
+    static const secp256k1_scalar zero = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0);
+    static const secp256k1_scalar one = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 1);
+    static const secp256k1_scalar max = SECP256K1_SCALAR_CONST(
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
+    );
+    secp256k1_scalar r = max;
+    secp256k1_scalar a = zero;
+
+    secp256k1_scalar_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    r = zero; a = max;
+    secp256k1_scalar_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    a = zero;
+    secp256k1_scalar_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &zero, sizeof(r)) == 0);
+
+    a = one;
+    secp256k1_scalar_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+
+    r = one; a = zero;
+    secp256k1_scalar_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+}
+
+void ge_storage_cmov_test(void) {
+    static const secp256k1_ge_storage zero = SECP256K1_GE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+    static const secp256k1_ge_storage one = SECP256K1_GE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1);
+    static const secp256k1_ge_storage max = SECP256K1_GE_STORAGE_CONST(
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
+        0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
+    );
+    secp256k1_ge_storage r = max;
+    secp256k1_ge_storage a = zero;
+
+    secp256k1_ge_storage_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    r = zero; a = max;
+    secp256k1_ge_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &max, sizeof(r)) == 0);
+
+    a = zero;
+    secp256k1_ge_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &zero, sizeof(r)) == 0);
+
+    a = one;
+    secp256k1_ge_storage_cmov(&r, &a, 1);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+
+    r = one; a = zero;
+    secp256k1_ge_storage_cmov(&r, &a, 0);
+    CHECK(memcmp(&r, &one, sizeof(r)) == 0);
+}
+
+void run_cmov_tests(void) {
+    int_cmov_test();
+    fe_cmov_test();
+    fe_storage_cmov_test();
+    scalar_cmov_test();
+    ge_storage_cmov_test();
+}
+
 int main(int argc, char **argv) {
     unsigned char seed16[16] = {0};
     unsigned char run32[32] = {0};
@@ -5431,6 +5586,8 @@ int main(int argc, char **argv) {
     /* util tests */
     run_memczero_test();
 
+    run_cmov_tests();
+
     secp256k1_rand256(run32);
     printf("random run = %02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x\n", run32[0], run32[1], run32[2], run32[3], run32[4], run32[5], run32[6], run32[7], run32[8], run32[9], run32[10], run32[11], run32[12], run32[13], run32[14], run32[15]);
 
diff --git a/src/util.h b/src/util.h
@@ -194,4 +194,18 @@ static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
     }
 }
 
+/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time.  Both *r and *a must be initialized and non-negative.*/
+static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag) {
+    unsigned int mask0, mask1, r_masked, a_masked;
+    /* Casting a negative int to unsigned and back to int is implementation defined behavior */
+    VERIFY_CHECK(*r >= 0 && *a >= 0);
+
+    mask0 = (unsigned int)flag + ~0u;
+    mask1 = ~mask0;
+    r_masked = ((unsigned int)*r & mask0);
+    a_masked = ((unsigned int)*a & mask1);
+
+    *r = (int)(r_masked | a_masked);
+}
+
 #endif /* SECP256K1_UTIL_H */
diff --git a/src/valgrind_ctime_test.c b/src/valgrind_ctime_test.c
@@ -12,6 +12,10 @@
 # include "include/secp256k1_ecdh.h"
 #endif
 
+#if ENABLE_MODULE_RECOVERY
+# include "include/secp256k1_recovery.h"
+#endif
+
 int main(void) {
     secp256k1_context* ctx;
     secp256k1_ecdsa_signature signature;
@@ -24,6 +28,10 @@ int main(void) {
     unsigned char key[32];
     unsigned char sig[74];
     unsigned char spubkey[33];
+#if ENABLE_MODULE_RECOVERY
+    secp256k1_ecdsa_recoverable_signature recoverable_signature;
+    int recid;
+#endif
 
     if (!RUNNING_ON_VALGRIND) {
         fprintf(stderr, "This test can only usefully be run inside valgrind.\n");
@@ -67,6 +75,17 @@ int main(void) {
     CHECK(ret == 1);
 #endif
 
+#if ENABLE_MODULE_RECOVERY
+    /* Test signing a recoverable signature. */
+    VALGRIND_MAKE_MEM_UNDEFINED(key, 32);
+    ret = secp256k1_ecdsa_sign_recoverable(ctx, &recoverable_signature, msg, key, NULL, NULL);
+    VALGRIND_MAKE_MEM_DEFINED(&recoverable_signature, sizeof(recoverable_signature));
+    VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
+    CHECK(ret);
+    CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(ctx, sig, &recid, &recoverable_signature));
+    CHECK(recid >= 0 && recid <= 3);
+#endif
+
     VALGRIND_MAKE_MEM_UNDEFINED(key, 32);
     ret = secp256k1_ec_seckey_verify(ctx, key);
     VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
