Merge #428: Exhaustive recovery

sipa · sipa · commit 9d560f992db2 · 2016-12-28T04:35:23.000-08:00
2cee5fd exhaustive tests: add recovery module (Andrew Poelstra) 678b0e5 exhaustive tests: remove erroneous comment from ecdsa_sig_sign (Andrew Poelstra) 03ff8c2 group_impl.h: remove unused `secp256k1_ge_set_infinity` function (Andrew Poelstra) a724d72 configure: add --enable-coverage to set options for coverage analysis (Andrew Poelstra) b595163 recovery: add tests to cover API misusage (Andrew Poelstra) 6f8ae2f ecdh: test NULL-checking of arguments (Andrew Poelstra) 25e3cfb ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign (Andrew Poelstra)
diff --git a/Makefile.am b/Makefile.am
@@ -93,7 +93,10 @@ TESTS =
 if USE_TESTS
 noinst_PROGRAMS += tests
 tests_SOURCES = src/tests.c
-tests_CPPFLAGS = -DSECP256K1_BUILD -DVERIFY -I$(top_srcdir)/src -I$(top_srcdir)/include $(SECP_INCLUDES) $(SECP_TEST_INCLUDES)
+tests_CPPFLAGS = -DSECP256K1_BUILD -I$(top_srcdir)/src -I$(top_srcdir)/include $(SECP_INCLUDES) $(SECP_TEST_INCLUDES)
+if !ENABLE_COVERAGE
+tests_CPPFLAGS += -DVERIFY
+endif
 tests_LDADD = $(SECP_LIBS) $(SECP_TEST_LIBS) $(COMMON_LIB)
 tests_LDFLAGS = -static
 TESTS += tests
@@ -102,7 +105,10 @@ endif
 if USE_EXHAUSTIVE_TESTS
 noinst_PROGRAMS += exhaustive_tests
 exhaustive_tests_SOURCES = src/tests_exhaustive.c
-exhaustive_tests_CPPFLAGS = -DSECP256K1_BUILD -DVERIFY -I$(top_srcdir)/src $(SECP_INCLUDES)
+exhaustive_tests_CPPFLAGS = -DSECP256K1_BUILD -I$(top_srcdir)/src $(SECP_INCLUDES)
+if !ENABLE_COVERAGE
+exhaustive_tests_CPPFLAGS += -DVERIFY
+endif
 exhaustive_tests_LDADD = $(SECP_LIBS)
 exhaustive_tests_LDFLAGS = -static
 TESTS += exhaustive_tests
diff --git a/configure.ac b/configure.ac
@@ -20,7 +20,7 @@ AC_PATH_TOOL(STRIP, strip)
 AX_PROG_CC_FOR_BUILD
 
 if test "x$CFLAGS" = "x"; then
-  CFLAGS="-O3 -g"
+  CFLAGS="-g"
 fi
 
 AM_PROG_CC_C_O
@@ -89,6 +89,11 @@ AC_ARG_ENABLE(benchmark,
     [use_benchmark=$enableval],
     [use_benchmark=no])
 
+AC_ARG_ENABLE(coverage,
+    AS_HELP_STRING([--enable-coverage],[enable compiler flags to support kcov coverage analysis]),
+    [enable_coverage=$enableval],
+    [enable_coverage=no])
+
 AC_ARG_ENABLE(tests,
     AS_HELP_STRING([--enable-tests],[compile tests (default is yes)]),
     [use_tests=$enableval],
@@ -154,6 +159,14 @@ AC_COMPILE_IFELSE([AC_LANG_SOURCE([[void myfunc() {__builtin_expect(0,0);}]])],
     [ AC_MSG_RESULT([no])
     ])
 
+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"
+    LDFLAGS="--coverage"
+else
+    CFLAGS="$CFLAGS -O3"
+fi
+
 if test x"$use_ecmult_static_precomputation" != x"no"; then
   save_cross_compiling=$cross_compiling
   cross_compiling=no
@@ -434,6 +447,7 @@ AC_MSG_NOTICE([Using field implementation: $set_field])
 AC_MSG_NOTICE([Using bignum implementation: $set_bignum])
 AC_MSG_NOTICE([Using scalar implementation: $set_scalar])
 AC_MSG_NOTICE([Using endomorphism optimizations: $use_endomorphism])
+AC_MSG_NOTICE([Building for coverage analysis: $enable_coverage])
 AC_MSG_NOTICE([Building ECDH module: $enable_module_ecdh])
 AC_MSG_NOTICE([Building ECDSA pubkey recovery module: $enable_module_recovery])
 AC_MSG_NOTICE([Using jni: $use_jni])
@@ -460,6 +474,7 @@ AC_SUBST(SECP_INCLUDES)
 AC_SUBST(SECP_LIBS)
 AC_SUBST(SECP_TEST_LIBS)
 AC_SUBST(SECP_TEST_INCLUDES)
+AM_CONDITIONAL([ENABLE_COVERAGE], [test x"$enable_coverage" = x"yes"])
 AM_CONDITIONAL([USE_TESTS], [test x"$use_tests" != x"no"])
 AM_CONDITIONAL([USE_EXHAUSTIVE_TESTS], [test x"$use_exhaustive_tests" != x"no"])
 AM_CONDITIONAL([USE_BENCHMARK], [test x"$use_benchmark" = x"yes"])
diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h
@@ -225,14 +225,12 @@ static int secp256k1_ecdsa_sig_verify(const secp256k1_ecmult_context *ctx, const
 #if defined(EXHAUSTIVE_TEST_ORDER)
 {
     secp256k1_scalar computed_r;
-    int overflow = 0;
     secp256k1_ge pr_ge;
     secp256k1_ge_set_gej(&pr_ge, &pr);
     secp256k1_fe_normalize(&pr_ge.x);
 
     secp256k1_fe_get_b32(c, &pr_ge.x);
-    secp256k1_scalar_set_b32(&computed_r, c, &overflow);
-    /* we fully expect overflow */
+    secp256k1_scalar_set_b32(&computed_r, c, NULL);
     return secp256k1_scalar_eq(sigr, &computed_r);
 }
 #else
@@ -285,14 +283,10 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
     secp256k1_fe_normalize(&r.y);
     secp256k1_fe_get_b32(b, &r.x);
     secp256k1_scalar_set_b32(sigr, b, &overflow);
-    if (secp256k1_scalar_is_zero(sigr)) {
-        /* P.x = order is on the curve, so technically sig->r could end up zero, which would be an invalid signature.
-         * This branch is cryptographically unreachable as hitting it requires finding the discrete log of P.x = N.
-         */
-        secp256k1_gej_clear(&rp);
-        secp256k1_ge_clear(&r);
-        return 0;
-    }
+    /* These two conditions should be checked before calling */
+    VERIFY_CHECK(!secp256k1_scalar_is_zero(sigr));
+    VERIFY_CHECK(overflow == 0);
+
     if (recid) {
         /* The overflow condition is cryptographically unreachable as hitting it requires finding the discrete log
          * of some P where P.x >= order, and only 1 in about 2^127 points meet this criteria.
diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h
@@ -38,10 +38,6 @@ static void secp256k1_fe_verify(const secp256k1_fe *a) {
     }
     VERIFY_CHECK(r == 1);
 }
-#else
-static void secp256k1_fe_verify(const secp256k1_fe *a) {
-    (void)a;
-}
 #endif
 
 static void secp256k1_fe_normalize(secp256k1_fe *r) {
diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h
@@ -49,10 +49,6 @@ static void secp256k1_fe_verify(const secp256k1_fe *a) {
     }
     VERIFY_CHECK(r == 1);
 }
-#else
-static void secp256k1_fe_verify(const secp256k1_fe *a) {
-    (void)a;
-}
 #endif
 
 static void secp256k1_fe_normalize(secp256k1_fe *r) {
diff --git a/src/group_impl.h b/src/group_impl.h
@@ -200,12 +200,6 @@ static void secp256k1_gej_set_infinity(secp256k1_gej *r) {
     secp256k1_fe_clear(&r->z);
 }
 
-static void secp256k1_ge_set_infinity(secp256k1_ge *r) {
-    r->infinity = 1;
-    secp256k1_fe_clear(&r->x);
-    secp256k1_fe_clear(&r->y);
-}
-
 static void secp256k1_gej_clear(secp256k1_gej *r) {
     r->infinity = 0;
     secp256k1_fe_clear(&r->x);
diff --git a/src/modules/ecdh/main_impl.h b/src/modules/ecdh/main_impl.h
@@ -16,10 +16,10 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *result, const se
     secp256k1_gej res;
     secp256k1_ge pt;
     secp256k1_scalar s;
+    VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(result != NULL);
     ARG_CHECK(point != NULL);
     ARG_CHECK(scalar != NULL);
-    (void)ctx;
 
     secp256k1_pubkey_load(ctx, &pt, point);
     secp256k1_scalar_set_b32(&s, scalar, &overflow);
diff --git a/src/modules/ecdh/tests_impl.h b/src/modules/ecdh/tests_impl.h
@@ -7,6 +7,35 @@
 #ifndef _SECP256K1_MODULE_ECDH_TESTS_
 #define _SECP256K1_MODULE_ECDH_TESTS_
 
+void test_ecdh_api(void) {
+    /* Setup context that just counts errors */
+    secp256k1_context *tctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
+    secp256k1_pubkey point;
+    unsigned char res[32];
+    unsigned char s_one[32] = { 0 };
+    int32_t ecount = 0;
+    s_one[31] = 1;
+
+    secp256k1_context_set_error_callback(tctx, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_illegal_callback(tctx, counting_illegal_callback_fn, &ecount);
+    CHECK(secp256k1_ec_pubkey_create(tctx, &point, s_one) == 1);
+
+    /* Check all NULLs are detected */
+    CHECK(secp256k1_ecdh(tctx, res, &point, s_one) == 1);
+    CHECK(ecount == 0);
+    CHECK(secp256k1_ecdh(tctx, NULL, &point, s_one) == 0);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdh(tctx, res, NULL, s_one) == 0);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdh(tctx, res, &point, NULL) == 0);
+    CHECK(ecount == 3);
+    CHECK(secp256k1_ecdh(tctx, res, &point, s_one) == 1);
+    CHECK(ecount == 3);
+
+    /* Cleanup */
+    secp256k1_context_destroy(tctx);
+}
+
 void test_ecdh_generator_basepoint(void) {
     unsigned char s_one[32] = { 0 };
     secp256k1_pubkey point[2];
@@ -68,6 +97,7 @@ void test_bad_scalar(void) {
 }
 
 void run_ecdh_tests(void) {
+    test_ecdh_api();
     test_ecdh_generator_basepoint();
     test_bad_scalar();
 }
diff --git a/src/modules/recovery/main_impl.h b/src/modules/recovery/main_impl.h
@@ -179,7 +179,7 @@ int secp256k1_ecdsa_recover(const secp256k1_context* ctx, secp256k1_pubkey *pubk
     ARG_CHECK(pubkey != NULL);
 
     secp256k1_ecdsa_recoverable_signature_load(ctx, &r, &s, &recid, signature);
-    ARG_CHECK(recid >= 0 && recid < 4);
+    VERIFY_CHECK(recid >= 0 && recid < 4);  /* should have been caught in parse_compact */
     secp256k1_scalar_set_b32(&m, msg32, NULL);
     if (secp256k1_ecdsa_sig_recover(&ctx->ecmult_ctx, &r, &s, &q, &m, recid)) {
         secp256k1_pubkey_save(pubkey, &q);
diff --git a/src/modules/recovery/tests_impl.h b/src/modules/recovery/tests_impl.h
@@ -7,6 +7,146 @@
 #ifndef _SECP256K1_MODULE_RECOVERY_TESTS_
 #define _SECP256K1_MODULE_RECOVERY_TESTS_
 
+static int recovery_test_nonce_function(unsigned char *nonce32, const unsigned char *msg32, const unsigned char *key32, const unsigned char *algo16, void *data, unsigned int counter) {
+    (void) msg32;
+    (void) key32;
+    (void) algo16;
+    (void) data;
+
+    /* On the first run, return 0 to force a second run */
+    if (counter == 0) {
+        memset(nonce32, 0, 32);
+        return 1;
+    }
+    /* On the second run, return an overflow to force a third run */
+    if (counter == 1) {
+        memset(nonce32, 0xff, 32);
+        return 1;
+    }
+    /* On the next run, return a valid nonce, but flip a coin as to whether or not to fail signing. */
+    memset(nonce32, 1, 32);
+    return secp256k1_rand_bits(1);
+}
+
+void test_ecdsa_recovery_api(void) {
+    /* Setup contexts that just count errors */
+    secp256k1_context *none = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
+    secp256k1_context *sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
+    secp256k1_context *vrfy = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY);
+    secp256k1_context *both = secp256k1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
+    secp256k1_pubkey pubkey;
+    secp256k1_pubkey recpubkey;
+    secp256k1_ecdsa_signature normal_sig;
+    secp256k1_ecdsa_recoverable_signature recsig;
+    unsigned char privkey[32] = { 1 };
+    unsigned char message[32] = { 2 };
+    int32_t ecount = 0;
+    int recid = 0;
+    unsigned char sig[74];
+    unsigned char zero_privkey[32] = { 0 };
+    unsigned char over_privkey[32] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                                       0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                                       0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+                                       0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+
+    secp256k1_context_set_error_callback(none, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_error_callback(sign, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_error_callback(vrfy, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_error_callback(both, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_illegal_callback(none, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_illegal_callback(sign, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_illegal_callback(vrfy, counting_illegal_callback_fn, &ecount);
+    secp256k1_context_set_illegal_callback(both, counting_illegal_callback_fn, &ecount);
+
+    /* Construct and verify corresponding public key. */
+    CHECK(secp256k1_ec_seckey_verify(ctx, privkey) == 1);
+    CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, privkey) == 1);
+
+    /* Check bad contexts and NULLs for signing */
+    ecount = 0;
+    CHECK(secp256k1_ecdsa_sign_recoverable(none, &recsig, message, privkey, NULL, NULL) == 0);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdsa_sign_recoverable(sign, &recsig, message, privkey, NULL, NULL) == 1);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdsa_sign_recoverable(vrfy, &recsig, message, privkey, NULL, NULL) == 0);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, privkey, NULL, NULL) == 1);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, NULL, message, privkey, NULL, NULL) == 0);
+    CHECK(ecount == 3);
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, NULL, privkey, NULL, NULL) == 0);
+    CHECK(ecount == 4);
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, NULL, NULL, NULL) == 0);
+    CHECK(ecount == 5);
+    /* This will fail or succeed randomly, and in either case will not ARG_CHECK failure */
+    secp256k1_ecdsa_sign_recoverable(both, &recsig, message, privkey, recovery_test_nonce_function, NULL);
+    CHECK(ecount == 5);
+    /* These will all fail, but not in ARG_CHECK way */
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, zero_privkey, NULL, NULL) == 0);
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, over_privkey, NULL, NULL) == 0);
+    /* This one will succeed. */
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, privkey, NULL, NULL) == 1);
+    CHECK(ecount == 5);
+
+    /* Check signing with a goofy nonce function */
+
+    /* Check bad contexts and NULLs for recovery */
+    ecount = 0;
+    CHECK(secp256k1_ecdsa_recover(none, &recpubkey, &recsig, message) == 0);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdsa_recover(sign, &recpubkey, &recsig, message) == 0);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_recover(vrfy, &recpubkey, &recsig, message) == 1);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_recover(both, &recpubkey, &recsig, message) == 1);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_recover(both, NULL, &recsig, message) == 0);
+    CHECK(ecount == 3);
+    CHECK(secp256k1_ecdsa_recover(both, &recpubkey, NULL, message) == 0);
+    CHECK(ecount == 4);
+    CHECK(secp256k1_ecdsa_recover(both, &recpubkey, &recsig, NULL) == 0);
+    CHECK(ecount == 5);
+
+    /* Check NULLs for conversion */
+    CHECK(secp256k1_ecdsa_sign(both, &normal_sig, message, privkey, NULL, NULL) == 1);
+    ecount = 0;
+    CHECK(secp256k1_ecdsa_recoverable_signature_convert(both, NULL, &recsig) == 0);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdsa_recoverable_signature_convert(both, &normal_sig, NULL) == 0);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_recoverable_signature_convert(both, &normal_sig, &recsig) == 1);
+
+    /* Check NULLs for de/serialization */
+    CHECK(secp256k1_ecdsa_sign_recoverable(both, &recsig, message, privkey, NULL, NULL) == 1);
+    ecount = 0;
+    CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(both, NULL, &recid, &recsig) == 0);
+    CHECK(ecount == 1);
+    CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(both, sig, NULL, &recsig) == 0);
+    CHECK(ecount == 2);
+    CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(both, sig, &recid, NULL) == 0);
+    CHECK(ecount == 3);
+    CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(both, sig, &recid, &recsig) == 1);
+
+    CHECK(secp256k1_ecdsa_recoverable_signature_parse_compact(both, NULL, sig, recid) == 0);
+    CHECK(ecount == 4);
+    CHECK(secp256k1_ecdsa_recoverable_signature_parse_compact(both, &recsig, NULL, recid) == 0);
+    CHECK(ecount == 5);
+    CHECK(secp256k1_ecdsa_recoverable_signature_parse_compact(both, &recsig, sig, -1) == 0);
+    CHECK(ecount == 6);
+    CHECK(secp256k1_ecdsa_recoverable_signature_parse_compact(both, &recsig, sig, 5) == 0);
+    CHECK(ecount == 7);
+    /* overflow in signature will fail but not affect ecount */
+    memcpy(sig, over_privkey, 32);
+    CHECK(secp256k1_ecdsa_recoverable_signature_parse_compact(both, &recsig, sig, recid) == 0);
+    CHECK(ecount == 7);
+
+    /* cleanup */
+    secp256k1_context_destroy(none);
+    secp256k1_context_destroy(sign);
+    secp256k1_context_destroy(vrfy);
+    secp256k1_context_destroy(both);
+}
+
 void test_ecdsa_recovery_end_to_end(void) {
     unsigned char extra[32] = {0x00};
     unsigned char privkey[32];
@@ -241,6 +381,9 @@ void test_ecdsa_recovery_edge_cases(void) {
 
 void run_recovery_tests(void) {
     int i;
+    for (i = 0; i < count; i++) {
+        test_ecdsa_recovery_api();
+    }
     for (i = 0; i < 64*count; i++) {
         test_ecdsa_recovery_end_to_end();
     }
diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c
diff --git a/src/util.h b/src/util.h

Original file line number	Diff line number	Diff line change
`@@ -38,10 +38,6 @@ static void secp256k1_fe_verify(const secp256k1_fe *a) {`
`38`	`38`	`}`
`39`	`39`	`VERIFY_CHECK(r == 1);`
`40`	`40`	`}`
`41`		`-#else`
`42`		`-static void secp256k1_fe_verify(const secp256k1_fe *a) {`
`43`		`- (void)a;`
`44`		`-}`
`45`	`41`	`#endif`
`46`	`42`
`47`	`43`	`static void secp256k1_fe_normalize(secp256k1_fe *r) {`
Original file line number	Diff line number	Diff line change
`@@ -49,10 +49,6 @@ static void secp256k1_fe_verify(const secp256k1_fe *a) {`
`49`	`49`	`}`
`50`	`50`	`VERIFY_CHECK(r == 1);`
`51`	`51`	`}`
`52`		`-#else`
`53`		`-static void secp256k1_fe_verify(const secp256k1_fe *a) {`
`54`		`- (void)a;`
`55`		`-}`
`56`	`52`	`#endif`
`57`	`53`
`58`	`54`	`static void secp256k1_fe_normalize(secp256k1_fe *r) {`