Skip to content

Commit 5b71a3f

Browse files
gmaxwellgmaxwell
committed
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.
1 parent 3b7bc69 commit 5b71a3f

File tree

2 files changed

+78
-10
lines changed

2 files changed

+78
-10
lines changed

‎src/secp256k1.c‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,15 +167,25 @@ int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pu
167167

168168
int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *output, size_t *outputlen, const secp256k1_pubkey* pubkey, unsigned int flags) {
169169
secp256k1_ge Q;
170+
size_t len;
171+
int ret = 0;
170172

171173
(void)ctx;
172174
VERIFY_CHECK(ctx != NULL);
173-
ARG_CHECK(output != NULL);
174175
ARG_CHECK(outputlen != NULL);
176+
len = *outputlen;
177+
*outputlen = 0;
178+
ARG_CHECK(output != NULL);
179+
memset(output, 0, len);
175180
ARG_CHECK(pubkey != NULL);
176181
ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION);
177-
return (secp256k1_pubkey_load(ctx, &Q, pubkey) &&
178-
secp256k1_eckey_pubkey_serialize(&Q, output, outputlen, flags & SECP256K1_FLAGS_BIT_COMPRESSION));
182+
if (secp256k1_pubkey_load(ctx, &Q, pubkey)) {
183+
ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, flags & SECP256K1_FLAGS_BIT_COMPRESSION);
184+
if (ret) {
185+
*outputlen = len;
186+
}
187+
}
188+
return ret;
179189
}
180190

181191
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
402412
int overflow;
403413
int ret = 0;
404414
VERIFY_CHECK(ctx != NULL);
405-
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
406415
ARG_CHECK(pubkey != NULL);
416+
memset(pubkey, 0, sizeof(*pubkey));
417+
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
407418
ARG_CHECK(seckey != NULL);
408419

409420
secp256k1_scalar_set_b32(&sec, seckey, &overflow);
410421
ret = (!overflow) & (!secp256k1_scalar_is_zero(&sec));
411-
memset(pubkey, 0, sizeof(*pubkey));
412422
if (ret) {
413423
secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pj, &sec);
414424
secp256k1_ge_set_gej(&p, &pj);

‎src/tests.c‎

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,8 @@ void run_context_tests(void) {
232232
secp256k1_context_destroy(sign);
233233
secp256k1_context_destroy(vrfy);
234234
secp256k1_context_destroy(both);
235+
/* Defined as no-op. */
236+
secp256k1_context_destroy(NULL);
235237
}
236238

237239
/***** HASH TESTS *****/
@@ -2166,9 +2168,11 @@ void run_ec_pubkey_parse_test(void) {
21662168
0xA8, 0xFD, 0x17, 0xB4, 0x48, 0xA6, 0x85, 0x54, 0x19, 0x9C, 0x47, 0xD0, 0x8F, 0xFB, 0x10, 0xD4,
21672169
0xB8, 0x00
21682170
};
2171+
unsigned char sout[65];
21692172
unsigned char shortkey[2];
21702173
secp256k1_ge ge;
21712174
secp256k1_pubkey pubkey;
2175+
size_t len;
21722176
int32_t i;
21732177
int32_t ecount;
21742178
int32_t ecount2;
@@ -2265,6 +2269,30 @@ void run_ec_pubkey_parse_test(void) {
22652269
VG_CHECK(&ge.infinity, sizeof(ge.infinity));
22662270
ge_equals_ge(&secp256k1_ge_const_g, &ge);
22672271
CHECK(ecount == 0);
2272+
/* secp256k1_ec_pubkey_serialize illegal args. */
2273+
ecount = 0;
2274+
len = 65;
2275+
CHECK(secp256k1_ec_pubkey_serialize(ctx, NULL, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0);
2276+
CHECK(ecount == 1);
2277+
CHECK(len == 0);
2278+
CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, NULL, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 0);
2279+
CHECK(ecount == 2);
2280+
len = 65;
2281+
VG_UNDEF(sout, 65);
2282+
CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, NULL, SECP256K1_EC_UNCOMPRESSED) == 0);
2283+
VG_CHECK(sout, 65);
2284+
CHECK(ecount == 3);
2285+
CHECK(len == 0);
2286+
len = 65;
2287+
CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, ~0) == 0);
2288+
CHECK(ecount == 4);
2289+
CHECK(len == 0);
2290+
len = 65;
2291+
VG_UNDEF(sout, 65);
2292+
CHECK(secp256k1_ec_pubkey_serialize(ctx, sout, &len, &pubkey, SECP256K1_EC_UNCOMPRESSED) == 1);
2293+
VG_CHECK(sout, 65);
2294+
CHECK(ecount == 4);
2295+
CHECK(len == 65);
22682296
/* Multiple illegal args. Should still set arg error only once. */
22692297
ecount = 0;
22702298
ecount2 = 11;
@@ -2453,7 +2481,7 @@ void run_eckey_edge_case_test(void) {
24532481
memset(&pubkey, 1, sizeof(pubkey));
24542482
CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0);
24552483
CHECK(ecount == 2);
2456-
CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) > 0);
2484+
CHECK(memcmp(&pubkey, zeros, sizeof(secp256k1_pubkey)) == 0);
24572485
secp256k1_context_set_illegal_callback(ctx, NULL, NULL);
24582486
}
24592487

@@ -2648,6 +2676,7 @@ void test_ecdsa_end_to_end(void) {
26482676
CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5]));
26492677
CHECK(secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5]));
26502678
CHECK(!secp256k1_ecdsa_signature_normalize(ctx, NULL, &signature[5]));
2679+
CHECK(!secp256k1_ecdsa_signature_normalize(ctx, &signature[5], &signature[5]));
26512680
CHECK(secp256k1_ecdsa_verify(ctx, &signature[5], message, &pubkey) == 1);
26522681
secp256k1_scalar_negate(&s, &s);
26532682
secp256k1_ecdsa_signature_save(&signature[5], &r, &s);
@@ -3286,17 +3315,46 @@ void test_ecdsa_edge_cases(void) {
32863315
CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, NULL) == 0);
32873316
CHECK(ecount == 6);
32883317
CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 1);
3318+
CHECK(ecount == 6);
3319+
CHECK(secp256k1_ec_pubkey_create(ctx, &pubkey, NULL) == 0);
3320+
CHECK(ecount == 7);
3321+
/* That pubkeyload fails via an ARGCHECK is a little odd but makes sense because pubkeys are an opaque data type. */
3322+
CHECK(secp256k1_ecdsa_verify(ctx, &sig, msg, &pubkey) == 0);
3323+
CHECK(ecount == 8);
32893324
siglen = 72;
32903325
CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, NULL, &siglen, &sig) == 0);
3291-
CHECK(ecount == 7);
3326+
CHECK(ecount == 9);
32923327
CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, NULL, &sig) == 0);
3293-
CHECK(ecount == 8);
3328+
CHECK(ecount == 10);
32943329
CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, NULL) == 0);
3295-
CHECK(ecount == 9);
3330+
CHECK(ecount == 11);
32963331
CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 1);
3332+
CHECK(ecount == 11);
3333+
CHECK(secp256k1_ecdsa_signature_parse_der(ctx, NULL, signature, siglen) == 0);
3334+
CHECK(ecount == 12);
3335+
CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, NULL, siglen) == 0);
3336+
CHECK(ecount == 13);
3337+
CHECK(secp256k1_ecdsa_signature_parse_der(ctx, &sig, signature, siglen) == 1);
3338+
CHECK(ecount == 13);
32973339
siglen = 10;
3340+
/* Too little room for a signature does not fail via ARGCHECK. */
32983341
CHECK(secp256k1_ecdsa_signature_serialize_der(ctx, signature, &siglen, &sig) == 0);
3299-
CHECK(ecount == 9);
3342+
CHECK(ecount == 13);
3343+
ecount = 0;
3344+
CHECK(secp256k1_ecdsa_signature_normalize(ctx, NULL, NULL) == 0);
3345+
CHECK(ecount == 1);
3346+
CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, NULL, &sig) == 0);
3347+
CHECK(ecount == 2);
3348+
CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, NULL) == 0);
3349+
CHECK(ecount == 3);
3350+
CHECK(secp256k1_ecdsa_signature_serialize_compact(ctx, signature, &sig) == 1);
3351+
CHECK(ecount == 3);
3352+
CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, NULL, signature) == 0);
3353+
CHECK(ecount == 4);
3354+
CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, NULL) == 0);
3355+
CHECK(ecount == 5);
3356+
CHECK(secp256k1_ecdsa_signature_parse_compact(ctx, &sig, signature) == 1);
3357+
CHECK(ecount == 5);
33003358
secp256k1_context_set_illegal_callback(ctx, NULL, NULL);
33013359
}
33023360

0 commit comments

Comments
 (0)