Skip to content

Commit

Permalink
Merge bitcoin#428: Exhaustive recovery
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
sipa committed Dec 28, 2016
2 parents 8225239 + 2cee5fd commit 9d560f9
Show file tree
Hide file tree
Showing 12 changed files with 350 additions and 32 deletions.
10 changes: 8 additions & 2 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
17 changes: 16 additions & 1 deletion configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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])
Expand All @@ -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"])
Expand Down
16 changes: 5 additions & 11 deletions src/ecdsa_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 0 additions & 4 deletions src/field_10x26_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions src/field_5x52_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 0 additions & 6 deletions src/group_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/modules/ecdh/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
30 changes: 30 additions & 0 deletions src/modules/ecdh/tests_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -68,6 +97,7 @@ void test_bad_scalar(void) {
}

void run_ecdh_tests(void) {
test_ecdh_api();
test_ecdh_generator_basepoint();
test_bad_scalar();
}
Expand Down
2 changes: 1 addition & 1 deletion src/modules/recovery/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
143 changes: 143 additions & 0 deletions src/modules/recovery/tests_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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();
}
Expand Down
Loading

0 comments on commit 9d560f9

Please sign in to comment.