diff --git a/include/secp256k1_whitelist.h b/include/secp256k1_whitelist.h index 20d495d3..e2f82173 100644 --- a/include/secp256k1_whitelist.h +++ b/include/secp256k1_whitelist.h @@ -68,6 +68,10 @@ SECP256K1_API int secp256k1_whitelist_signature_parse( * * Returns: the number of keys for the given signature * In: sig: pointer to a signature object + * + * This count is a property of the signature only. It might not match the + * number of public keys the caller has, and the caller should not truncate + * their key set to match it. */ SECP256K1_API size_t secp256k1_whitelist_signature_n_keys( const secp256k1_whitelist_signature *sig @@ -109,6 +113,8 @@ SECP256K1_API int secp256k1_whitelist_signature_serialize( * online_i + H(offline_i + whitelist)(offline_i + whitelist) * for each public key pair (offline_i, offline_i). Here H means sha256 of the * compressed serialization of the key. + * + * See secp256k1_whitelist_verify for the rationale on the degenerate destination W = -P_i. */ SECP256K1_API int secp256k1_whitelist_sign( const secp256k1_context *ctx, @@ -131,6 +137,13 @@ SECP256K1_API int secp256k1_whitelist_sign( * offline_pubkeys: list of all offline pubkeys * n_keys: the number of entries in each of the above two arrays * sub_pubkey: the key to be whitelisted + * + * When the destination W equals -P_i for a whitelisted offline key, the tweak + * degenerates and the ring key collapses to K_i = Q_i, so the online key alone + * produces a valid proof for that destination. This is accepted deliberately: + * the output is spendable only by the holder of p_i (the discrete log of -P_i + * is -p_i), i.e. the offline half of the same whitelist entry, so no funds can + * be diverted. */ SECP256K1_API int secp256k1_whitelist_verify( const secp256k1_context *ctx, diff --git a/src/modules/bppp/main_impl.h b/src/modules/bppp/main_impl.h index 8c4d1eab..bc7c281b 100644 --- a/src/modules/bppp/main_impl.h +++ b/src/modules/bppp/main_impl.h @@ -28,6 +28,11 @@ secp256k1_bppp_generators *secp256k1_bppp_generators_create(const secp256k1_cont if (ret == NULL) { return NULL; } + /* Ensure that multiplication will not wrap around */ + if (n > SIZE_MAX / sizeof(*ret->gens)) { + free(ret); + return NULL; + } ret->gens = checked_malloc(&ctx->error_callback, n * sizeof(*ret->gens)); if (ret->gens == NULL) { free(ret); diff --git a/src/modules/bppp/tests_impl.h b/src/modules/bppp/tests_impl.h index 47a00c6d..0c5ed1e9 100644 --- a/src/modules/bppp/tests_impl.h +++ b/src/modules/bppp/tests_impl.h @@ -28,6 +28,8 @@ static void test_bppp_generators_api(void) { CHECK(gens != NULL); gens_orig = gens; /* Preserve for round-trip test */ + CHECK(secp256k1_bppp_generators_create(CTX, SIZE_MAX / sizeof(secp256k1_ge) + 1) == NULL); + /* Serialize */ CHECK_ILLEGAL(CTX, secp256k1_bppp_generators_serialize(CTX, NULL, gens_ser, &len)); CHECK_ILLEGAL(CTX, secp256k1_bppp_generators_serialize(CTX, gens, NULL, &len)); diff --git a/src/modules/whitelist/main_impl.h b/src/modules/whitelist/main_impl.h index 28c563b2..6c66eb2d 100644 --- a/src/modules/whitelist/main_impl.h +++ b/src/modules/whitelist/main_impl.h @@ -133,19 +133,28 @@ size_t secp256k1_whitelist_signature_n_keys(const secp256k1_whitelist_signature } int secp256k1_whitelist_signature_parse(const secp256k1_context* ctx, secp256k1_whitelist_signature *sig, const unsigned char *input, size_t input_len) { + size_t n_keys; VERIFY_CHECK(ctx != NULL); ARG_CHECK(sig != NULL); ARG_CHECK(input != NULL); + /* The header guarantees sig is initialized on every path and that a failed + * parse fails validation for any key set. MAX_KEYS + 1 is that canonical + * invalid state: _verify rejects it on sig->n_keys > MAX_KEYS regardless of + * the count the caller supplies. */ + memset(sig, 0, sizeof(*sig)); + sig->n_keys = MAX_KEYS + 1; + if (input_len == 0) { return 0; } - sig->n_keys = input[0]; - if (sig->n_keys > MAX_KEYS || input_len != 1 + 32 * (sig->n_keys + 1)) { + n_keys = input[0]; + if (n_keys > MAX_KEYS || input_len != 1 + 32 * (n_keys + 1)) { return 0; } - memcpy(&sig->data[0], &input[1], 32 * (sig->n_keys + 1)); + sig->n_keys = n_keys; + memcpy(&sig->data[0], &input[1], 32 * (n_keys + 1)); return 1; } @@ -156,6 +165,10 @@ int secp256k1_whitelist_signature_serialize(const secp256k1_context* ctx, unsign ARG_CHECK(output_len != NULL); ARG_CHECK(sig != NULL); + /* Do not trust n_keys to have come from _parse. */ + if (sig->n_keys > MAX_KEYS) { + return 0; + } if (*output_len < 1 + 32 * (sig->n_keys + 1)) { return 0; } diff --git a/src/modules/whitelist/tests_impl.h b/src/modules/whitelist/tests_impl.h index dad8a479..b7cd1517 100644 --- a/src/modules/whitelist/tests_impl.h +++ b/src/modules/whitelist/tests_impl.h @@ -23,12 +23,17 @@ static void test_whitelist_end_to_end_internal(const unsigned char *summed_secke /* Serialization round trip */ CHECK(secp256k1_whitelist_signature_serialize(CTX, serialized, &slen, &sig) == 1); CHECK(slen == 33 + 32 * n_keys); - CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen) == 1); /* (Check various bad-length conditions) */ CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen + 32) == 0); CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen + 1) == 0); CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen - 1) == 0); CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, 0) == 0); + /* A failed parse must leave a signature that fails validation for any + * key set, as documented on secp256k1_whitelist_signature_parse. */ + CHECK(secp256k1_whitelist_signature_n_keys(&sig1) > SECP256K1_WHITELIST_MAX_N_KEYS); + CHECK(secp256k1_whitelist_verify(CTX, &sig1, online_pubkeys, offline_pubkeys, n_keys, sub_pubkey) == 0); + /* Re-parse to restore a valid state. */ + CHECK(secp256k1_whitelist_signature_parse(CTX, &sig1, serialized, slen) == 1); CHECK(secp256k1_whitelist_verify(CTX, &sig1, online_pubkeys, offline_pubkeys, n_keys, sub_pubkey) == 1); CHECK(secp256k1_whitelist_verify(CTX, &sig1, offline_pubkeys, online_pubkeys, n_keys, sub_pubkey) != 1); diff --git a/src/modules/whitelist/whitelist_impl.h b/src/modules/whitelist/whitelist_impl.h index f26c255c..10b6e6d4 100644 --- a/src/modules/whitelist/whitelist_impl.h +++ b/src/modules/whitelist/whitelist_impl.h @@ -117,6 +117,9 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context* /* compute tweaked keys */ secp256k1_gej_set_ge(&tweaked_gej, &offline_ge); secp256k1_gej_add_ge_var(&tweaked_gej, &tweaked_gej, &subkey_ge, NULL); + /* Fails only for the degenerate destination W = -P_i, where the ring + * key intentionally collapses to Q_i. See the rationale on + * secp256k1_whitelist_verify in include/secp256k1_whitelist.h. */ secp256k1_whitelist_tweak_pubkey(hash_ctx, &tweaked_gej); secp256k1_gej_add_ge_var(&keys[i], &tweaked_gej, &online_ge, NULL); }