From 5b71a3f460050ef755ff97b570f49f5bd7e92425 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Fri, 30 Oct 2015 09:16:40 +0000 Subject: [PATCH] Better error case handling for pubkey_create & pubkey_serialize, more tests. Makes secp256k1_ec_pubkey_serialize set the length to zero on failure, also makes secp256k1_ec_pubkey_create set the pubkey to zeros when the key argument is NULL. Also adds many additional ARGCHECK tests. --- src/secp256k1.c | 20 +++++++++++---- src/tests.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/secp256k1.c b/src/secp256k1.c index a4eb34504f9..e06f7d942e5 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -167,15 +167,25 @@ int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pu int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *output, size_t *outputlen, const secp256k1_pubkey* pubkey, unsigned int flags) { secp256k1_ge Q; + size_t len; + int ret = 0; (void)ctx; VERIFY_CHECK(ctx != NULL); - ARG_CHECK(output != NULL); ARG_CHECK(outputlen != NULL); + len = *outputlen; + *outputlen = 0; + ARG_CHECK(output != NULL); + memset(output, 0, len); ARG_CHECK(pubkey != NULL); ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION); - return (secp256k1_pubkey_load(ctx, &Q, pubkey) && - secp256k1_eckey_pubkey_serialize(&Q, output, outputlen, flags & SECP256K1_FLAGS_BIT_COMPRESSION)); + if (secp256k1_pubkey_load(ctx, &Q, pubkey)) { + ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, flags & SECP256K1_FLAGS_BIT_COMPRESSION); + if (ret) { + *outputlen = len; + } + } + return ret; } static void secp256k1_ecdsa_signature_load(const secp256k1_context* ctx, secp256k1_scalar* r, secp256k1_scalar* s, const secp256k1_ecdsa_signature* sig) { @@ -402,13 +412,13 @@ int secp256k1_ec_pubkey_create(const secp256k1_context* ctx, secp256k1_pubkey *p int overflow; int ret = 0; VERIFY_CHECK(ctx != NULL); - ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)); ARG_CHECK(pubkey != NULL); + memset(pubkey, 0, sizeof(*pubkey)); + ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)); ARG_CHECK(seckey != NULL); secp256k1_scalar_set_b32(&sec, seckey, &overflow); ret = (!overflow) & (!secp256k1_scalar_is_zero(&sec)); - memset(pubkey, 0, sizeof(*pubkey)); if (ret) { secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pj, &sec); secp256k1_ge_set_gej(&p, &pj); diff --git a/src/tests.c b/src/tests.c index 1e63659dbf3..115a8320234 100644 --- a/src/tests.c +++ b/src/tests.c @@ -232,6 +232,8 @@ void run_context_tests(void) { secp256k1_context_destroy(sign); secp256k1_context_destroy(vrfy); secp256k1_context_destroy(both); + /* Defined as no-op. */ + secp256k1_context_destroy(NULL); } /***** HASH TESTS *****/ @@ -2166,9 +2168,11 @@ void run_ec_pubkey_parse_test(void) { 0xA8, 0xFD, 0x17, 0xB4, 0x48, 0xA6, 0x85, 0x54, 0x19, 0x9C, 0x47, 0xD0, 0x8F, 0xFB, 0x10, 0xD4, 0xB8, 0x00 }; + unsigned char sout[65]; unsigned char shortkey[2]; secp256k1_ge ge; secp256k1_pubkey pubkey; + size_t len; int32_t i; int32_t ecount; int32_t ecount2; @@ -2265,6 +2269,30 @@ void run_ec_pubkey_parse_test(void) { VG_CHECK(&ge.infinity, sizeof(ge.infinity)); ge_equals_ge(&secp256k1_ge_const_g, &ge); CHECK(ecount == 0); + /* secp256k1_ec_pubkey_serialize illegal args. */ + ecount = 0; + len = 65; + CHECK(secp256k1_ec_pubkey_serialize(ctx, NULL, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0); + CHECK(ecount == 1); + CHECK(len == 0); + CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, NULL, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0); + CHECK(ecount == 2); + len = 65; + VG_UNDEF(sout, 65); + CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, NULL, SECP256K1_EC_UNCOMPRESSED) == 0); + VG_CHECK(sout, 65); + CHECK(ecount == 3); + CHECK(len == 0); + len = 65; + CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, ~0) == 0); + CHECK(ecount == 4); + CHECK(len == 0); + len = 65; + VG_UNDEF(sout, 65); + CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 1); + VG_CHECK(sout, 65); + CHECK(ecount == 4); + CHECK(len == 65); /* Multiple illegal args. Should still set arg error only once. */ ecount = 0; ecount2 = 11; @@ -2453,7 +2481,7 @@ void run_eckey_edge_case_test(void) { memset(&pubkey, 1, sizeof(pubkey)); CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0); CHECK(ecount == 2); - CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) > 0); + CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) == 0); secp256k1_context_set_illegal_callback(ctx, NULL, NULL); } @@ -2648,6 +2676,7 @@ void test_ecdsa_end_to_end(void) { CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5])); CHECK(secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5])); CHECK(!secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5])); + CHECK(!secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5])); CHECK(secp256k1_ecdsa_verify(ctx, &signature[5], message, &pubkey) == 1); secp256k1_scalar_negate(&s, &s); secp256k1_ecdsa_signature_save(&signature[5], &r, &s); @@ -3286,17 +3315,46 @@ void test_ecdsa_edge_cases(void) { CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, NULL) == 0); CHECK(ecount == 6); CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 1); + CHECK(ecount == 6); + CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0); + CHECK(ecount == 7); + /* That pubkeyload fails via an ARGCHECK is a little odd but makes sense because pubkeys are an opaque data type. */ + CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 0); + CHECK(ecount == 8); siglen = 72; CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, NULL, &siglen, &sig) == 0); - CHECK(ecount == 7); + CHECK(ecount == 9); CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, NULL, &sig) == 0); - CHECK(ecount == 8); + CHECK(ecount == 10); CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, NULL) == 0); - CHECK(ecount == 9); + CHECK(ecount == 11); CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 1); + CHECK(ecount == 11); + CHECK(secp256k1_ecdsa_signature_parse_der(ctx, NULL, signature, siglen) == 0); + CHECK(ecount == 12); + CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, NULL, siglen) == 0); + CHECK(ecount == 13); + CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, signature, siglen) == 1); + CHECK(ecount == 13); siglen = 10; + /* Too little room for a signature does not fail via ARGCHECK. */ CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 0); - CHECK(ecount == 9); + CHECK(ecount == 13); + ecount = 0; + CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, NULL) == 0); + CHECK(ecount == 1); + CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, NULL, &sig) == 0); + CHECK(ecount == 2); + CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, NULL) == 0); + CHECK(ecount == 3); + CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, &sig) == 1); + CHECK(ecount == 3); + CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, NULL, signature) == 0); + CHECK(ecount == 4); + CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, NULL) == 0); + CHECK(ecount == 5); + CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, signature) == 1); + CHECK(ecount == 5); secp256k1_context_set_illegal_callback(ctx, NULL, NULL); }