From ebc31f1f9d664fa15e0c4ec8af848d7b9de88375 Mon Sep 17 00:00:00 2001 From: Jonas Nick Date: Fri, 22 Nov 2019 13:58:40 +0000 Subject: [PATCH] musig: add ARG_CHECKs to functions to help debuggability --- include/secp256k1_musig.h | 18 +++++++----- src/modules/musig/main_impl.h | 51 +++++++++++----------------------- src/modules/musig/tests_impl.h | 34 +++++++++++------------ 3 files changed, 44 insertions(+), 59 deletions(-) diff --git a/include/secp256k1_musig.h b/include/secp256k1_musig.h index 942e325c..5e0f2ba0 100644 --- a/include/secp256k1_musig.h +++ b/include/secp256k1_musig.h @@ -142,7 +142,7 @@ typedef struct { * In: pubkeys: input array of public keys to combine. The order is important; * a different order will result in a different combined public * key (cannot be NULL) - * n_pubkeys: length of pubkeys array + * n_pubkeys: length of pubkeys array. Must be greater than 0. */ SECP256K1_API int secp256k1_musig_pubkey_combine( const secp256k1_context* ctx, @@ -176,7 +176,8 @@ SECP256K1_API int secp256k1_musig_pubkey_combine( * `musig_pubkey_combine` (cannot be NULL) * n_signers: length of signers array. Number of signers participating in * the MuSig. Must be greater than 0 and at most 2^32 - 1. - * my_index: index of this signer in the signers array + * my_index: index of this signer in the signers array. Must be less + * than `n_signers`. * seckey: the signer's 32-byte secret key (cannot be NULL) */ SECP256K1_API int secp256k1_musig_session_initialize( @@ -193,7 +194,10 @@ SECP256K1_API int secp256k1_musig_session_initialize( const unsigned char *seckey ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5) SECP256K1_ARG_NONNULL(7) SECP256K1_ARG_NONNULL(8) SECP256K1_ARG_NONNULL(11); -/** Gets the signer's public nonce given a list of all signers' data with commitments +/** Gets the signer's public nonce given a list of all signers' data with + * commitments. Called by participating signers after + * `secp256k1_musig_session_initialize` and after all nonce commitments have + * been collected * * Returns: 1: public nonce is written in nonce * 0: signer data is missing commitments or session isn't initialized @@ -204,7 +208,7 @@ SECP256K1_API int secp256k1_musig_session_initialize( * `musig_session_initialize`. Array length must equal to * `n_commitments` (cannot be NULL) * Out: nonce: the nonce (cannot be NULL) - * In: commitments: array of 32-byte nonce commitments (cannot be NULL) + * In: commitments: array of pointers to 32-byte nonce commitments (cannot be NULL) * n_commitments: the length of commitments and signers array. Must be the total * number of signers participating in the MuSig. * msg32: the 32-byte message to be signed. Must be NULL if already @@ -234,8 +238,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_session_get_publi * pre_session: pointer to a musig_pre_session struct from * `musig_pubkey_combine` (cannot be NULL) * pk_hash32: the 32-byte hash of the signers' individual keys (cannot be NULL) - * commitments: array of 32-byte nonce commitments. Array length must equal to - * `n_signers` (cannot be NULL) + * commitments: array of pointers to 32-byte nonce commitments. Array + * length must equal to `n_signers` (cannot be NULL) * n_signers: length of signers and commitments array. Number of signers * participating in the MuSig. Must be greater than 0 and at most * 2^32 - 1. @@ -369,7 +373,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_partial_sig_verif * * Returns: 1: all partial signatures have values in range. Does NOT mean the * resulting signature verifies. - * 0: some partial signature had s/r out of range + * 0: some partial signature are missing or had s or r out of range * Args: ctx: pointer to a context object (cannot be NULL) * session: initialized session for which the combined nonce has been * computed (cannot be NULL) diff --git a/src/modules/musig/main_impl.h b/src/modules/musig/main_impl.h index 8f9ae305..6d602ad9 100644 --- a/src/modules/musig/main_impl.h +++ b/src/modules/musig/main_impl.h @@ -155,6 +155,10 @@ int secp256k1_musig_session_initialize(const secp256k1_context* ctx, secp256k1_m ARG_CHECK(pre_session->magic == pre_session_magic); ARG_CHECK(seckey != NULL); + ARG_CHECK(n_signers > 0); + ARG_CHECK(n_signers <= UINT32_MAX); + ARG_CHECK(my_index < n_signers); + memset(session, 0, sizeof(*session)); session->magic = session_magic; @@ -167,12 +171,6 @@ int secp256k1_musig_session_initialize(const secp256k1_context* ctx, secp256k1_m memcpy(&session->combined_pk, combined_pk, sizeof(*combined_pk)); session->pre_session = *pre_session; session->has_secret_data = 1; - if (n_signers == 0 || my_index >= n_signers) { - return 0; - } - if (n_signers > UINT32_MAX) { - return 0; - } session->n_signers = (uint32_t) n_signers; secp256k1_musig_signers_init(signers, session->n_signers); @@ -243,19 +241,18 @@ int secp256k1_musig_session_get_public_nonce(const secp256k1_context* ctx, secp2 VERIFY_CHECK(ctx != NULL); ARG_CHECK(session != NULL); + ARG_CHECK(session->magic == session_magic); ARG_CHECK(signers != NULL); ARG_CHECK(nonce != NULL); ARG_CHECK(commitments != NULL); - ARG_CHECK(session->magic == session_magic); + ARG_CHECK(session->round == 0); /* If the message was not set during initialization it must be set now. */ ARG_CHECK(!(!session->msg_is_set && msg32 == NULL)); /* The message can only be set once. */ ARG_CHECK(!(session->msg_is_set && msg32 != NULL)); - - if (!session->has_secret_data || n_commitments != session->n_signers) { - return 0; - } + ARG_CHECK(session->has_secret_data); + ARG_CHECK(n_commitments == session->n_signers); for (i = 0; i < n_commitments; i++) { ARG_CHECK(commitments[i] != NULL); } @@ -289,9 +286,8 @@ int secp256k1_musig_session_initialize_verifier(const secp256k1_context* ctx, se ARG_CHECK(commitments != NULL); /* Check n_signers before checking commitments to allow testing the case where * n_signers is big without allocating the space. */ - if (n_signers > UINT32_MAX) { - return 0; - } + ARG_CHECK(n_signers > 0); + ARG_CHECK(n_signers <= UINT32_MAX); for (i = 0; i < n_signers; i++) { ARG_CHECK(commitments[i] != NULL); } @@ -302,9 +298,6 @@ int secp256k1_musig_session_initialize_verifier(const secp256k1_context* ctx, se session->magic = session_magic; memcpy(&session->combined_pk, combined_pk, sizeof(*combined_pk)); session->pre_session = *pre_session; - if (n_signers == 0) { - return 0; - } session->n_signers = (uint32_t) n_signers; secp256k1_musig_signers_init(signers, session->n_signers); @@ -355,10 +348,8 @@ int secp256k1_musig_session_combine_nonces(const secp256k1_context* ctx, secp256 ARG_CHECK(signers != NULL); ARG_CHECK(session->magic == session_magic); ARG_CHECK(session->round == 1); + ARG_CHECK(n_signers == session->n_signers); - if (n_signers != session->n_signers) { - return 0; - } secp256k1_sha256_initialize(&sha); secp256k1_gej_set_infinity(&combined_noncej); for (i = 0; i < n_signers; i++) { @@ -418,7 +409,7 @@ int secp256k1_musig_partial_signature_parse(const secp256k1_context* ctx, secp25 } /* Compute msghash = SHA256(combined_nonce, combined_pk, msg) */ -static int secp256k1_musig_compute_messagehash(const secp256k1_context *ctx, unsigned char *msghash, const secp256k1_musig_session *session) { +static void secp256k1_musig_compute_messagehash(const secp256k1_context *ctx, unsigned char *msghash, const secp256k1_musig_session *session) { unsigned char buf[32]; secp256k1_ge rp; secp256k1_sha256 sha; @@ -434,7 +425,6 @@ static int secp256k1_musig_compute_messagehash(const secp256k1_context *ctx, uns secp256k1_sha256_write(&sha, buf, 32); secp256k1_sha256_write(&sha, session->msg, 32); secp256k1_sha256_finalize(&sha, msghash); - return 1; } int secp256k1_musig_partial_sign(const secp256k1_context* ctx, const secp256k1_musig_session *session, secp256k1_musig_partial_signature *partial_sig) { @@ -448,15 +438,10 @@ int secp256k1_musig_partial_sign(const secp256k1_context* ctx, const secp256k1_m ARG_CHECK(session != NULL); ARG_CHECK(session->magic == session_magic); ARG_CHECK(session->round == 2); - - if (!session->has_secret_data) { - return 0; - } + ARG_CHECK(session->has_secret_data); /* build message hash */ - if (!secp256k1_musig_compute_messagehash(ctx, msghash, session)) { - return 0; - } + secp256k1_musig_compute_messagehash(ctx, msghash, session); secp256k1_scalar_set_b32(&e, msghash, NULL); secp256k1_scalar_set_b32(&sk, session->seckey, &overflow); @@ -541,17 +526,13 @@ int secp256k1_musig_partial_sig_verify(const secp256k1_context* ctx, const secp2 ARG_CHECK(pubkey != NULL); ARG_CHECK(session->magic == session_magic); ARG_CHECK(session->round == 2); + ARG_CHECK(signer->present); - if (!signer->present) { - return 0; - } secp256k1_scalar_set_b32(&s, partial_sig->data, &overflow); if (overflow) { return 0; } - if (!secp256k1_musig_compute_messagehash(ctx, msghash, session)) { - return 0; - } + secp256k1_musig_compute_messagehash(ctx, msghash, session); secp256k1_scalar_set_b32(&e, msghash, NULL); /* Multiplying the messagehash by the musig coefficient is equivalent diff --git a/src/modules/musig/tests_impl.h b/src/modules/musig/tests_impl.h index cc15bcec..19fb0a5e 100644 --- a/src/modules/musig/tests_impl.h +++ b/src/modules/musig/tests_impl.h @@ -196,18 +196,18 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session_uninitialized, 2, 0, sk[0]) == 0); CHECK(ecount == 9); CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session, 0, 0, sk[0]) == 0); - CHECK(ecount == 9); + CHECK(ecount == 10); /* If more than UINT32_MAX fits in a size_t, test that session_initialize * rejects n_signers that high. */ if (SIZE_MAX > UINT32_MAX) { CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session, ((size_t) UINT32_MAX) + 2, 0, sk[0]) == 0); } - CHECK(ecount == 9); + CHECK(ecount == 11); CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session, 2, 0, NULL) == 0); - CHECK(ecount == 10); + CHECK(ecount == 12); /* secret key overflows */ CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session, 2, 0, ones) == 0); - CHECK(ecount == 10); + CHECK(ecount == 12); CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, &pre_session, 2, 0, sk[0]) == 1); CHECK(secp256k1_musig_session_initialize(sign, &session[1], signer1, nonce_commitment[1], session_id[1], msg, &combined_pk, &pre_session, 2, 1, sk[1]) == 1); @@ -228,11 +228,11 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, &pre_session, NULL, 2) == 0); CHECK(ecount == 5); CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, &pre_session, ncs, 0) == 0); - CHECK(ecount == 5); + CHECK(ecount == 6); if (SIZE_MAX > UINT32_MAX) { CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, &pre_session, ncs, ((size_t) UINT32_MAX) + 2) == 0); } - CHECK(ecount == 5); + CHECK(ecount == 7); CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, &pre_session, ncs, 2) == 1); /** Signing step 0 -- exchange nonce commitments */ @@ -273,7 +273,7 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(ecount == 5); /* Number of commitments and number of signers are different */ CHECK(secp256k1_musig_session_get_public_nonce(none, &session_0_tmp, signer0, &public_nonce[0], ncs, 1, NULL) == 0); - CHECK(ecount == 5); + CHECK(ecount == 6); CHECK(secp256k1_musig_session_get_public_nonce(none, &session[0], signer0, &public_nonce[0], ncs, 2, NULL) == 1); CHECK(secp256k1_musig_session_get_public_nonce(none, &session[1], signer1, &public_nonce[1], ncs, 2, NULL) == 1); @@ -282,12 +282,12 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(secp256k1_musig_set_nonce(none, &signer0[1], &public_nonce[0]) == 0); CHECK(secp256k1_musig_set_nonce(none, &signer0[1], &public_nonce[1]) == 1); CHECK(secp256k1_musig_set_nonce(none, &signer0[1], &public_nonce[1]) == 1); - CHECK(ecount == 5); + CHECK(ecount == 6); CHECK(secp256k1_musig_set_nonce(none, NULL, &public_nonce[0]) == 0); - CHECK(ecount == 6); - CHECK(secp256k1_musig_set_nonce(none, &signer1[0], NULL) == 0); CHECK(ecount == 7); + CHECK(secp256k1_musig_set_nonce(none, &signer1[0], NULL) == 0); + CHECK(ecount == 8); CHECK(secp256k1_musig_set_nonce(none, &signer1[0], &public_nonce[0]) == 1); CHECK(secp256k1_musig_set_nonce(none, &signer1[1], &public_nonce[1]) == 1); @@ -307,9 +307,9 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(ecount == 3); /* Number of signers differs from number during intialization */ CHECK(secp256k1_musig_session_combine_nonces(none, &session_0_tmp, signer0, 1, &nonce_is_negated, &adaptor) == 0); - CHECK(ecount == 3); + CHECK(ecount == 4); CHECK(secp256k1_musig_session_combine_nonces(none, &session_0_tmp, signer0, 2, NULL, &adaptor) == 1); - CHECK(ecount == 3); + CHECK(ecount == 4); memcpy(&session_0_tmp, &session[0], sizeof(session_0_tmp)); CHECK(secp256k1_musig_session_combine_nonces(none, &session_0_tmp, signer0, 2, &nonce_is_negated, NULL) == 1); @@ -334,7 +334,7 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { CHECK(secp256k1_musig_partial_sign(none, &session[1], &partial_sig[1]) == 1); /* observer can't sign */ CHECK(secp256k1_musig_partial_sign(none, &verifier_session, &partial_sig[2]) == 0); - CHECK(ecount == 3); + CHECK(ecount == 4); ecount = 0; CHECK(secp256k1_musig_partial_signature_serialize(none, buf, &partial_sig[0]) == 1); @@ -469,7 +469,7 @@ void musig_api_tests(secp256k1_scratch_space *scratch) { * ones and return the resulting messagehash. This should not result in a different * messagehash because the public keys of the signers are only used during session * initialization. */ -int musig_state_machine_diff_signer_msghash_test(unsigned char *msghash, secp256k1_xonly_pubkey *pks, secp256k1_xonly_pubkey *combined_pk, secp256k1_musig_pre_session *pre_session, const unsigned char * const *nonce_commitments, unsigned char *msg, secp256k1_pubkey *nonce_other, unsigned char *sk, unsigned char *session_id) { +void musig_state_machine_diff_signer_msghash_test(unsigned char *msghash, secp256k1_xonly_pubkey *pks, secp256k1_xonly_pubkey *combined_pk, secp256k1_musig_pre_session *pre_session, const unsigned char * const *nonce_commitments, unsigned char *msg, secp256k1_pubkey *nonce_other, unsigned char *sk, unsigned char *session_id) { secp256k1_musig_session session; secp256k1_musig_session session_tmp; unsigned char nonce_commitment[32]; @@ -498,7 +498,7 @@ int musig_state_machine_diff_signer_msghash_test(unsigned char *msghash, secp256 CHECK(secp256k1_musig_set_nonce(ctx, &signers[1], &nonce) == 1); CHECK(secp256k1_musig_session_combine_nonces(ctx, &session, signers, 2, NULL, NULL) == 1); - return secp256k1_musig_compute_messagehash(ctx, msghash, &session); + secp256k1_musig_compute_messagehash(ctx, msghash, &session); } /* Creates a new session (with a different session id) and tries to use that session @@ -664,8 +664,8 @@ void musig_state_machine_tests(secp256k1_scratch_space *scratch) { /* messagehash should be the same as a session whose get_public_nonce was called * with different signers (i.e. they diff in public keys). This is because the * public keys of the signers is set in stone when initializing the session. */ - CHECK(secp256k1_musig_compute_messagehash(ctx, msghash1, &session[1]) == 1); - CHECK(musig_state_machine_diff_signer_msghash_test(msghash2, pk, &combined_pk, &pre_session, ncs, msg, &nonce[0], sk[1], session_id[1]) == 1); + secp256k1_musig_compute_messagehash(ctx, msghash1, &session[1]); + musig_state_machine_diff_signer_msghash_test(msghash2, pk, &combined_pk, &pre_session, ncs, msg, &nonce[0], sk[1], session_id[1]); CHECK(memcmp(msghash1, msghash2, 32) == 0); CHECK(secp256k1_musig_partial_sign(ctx, &session[1], &partial_sig[1]) == 1);