Skip to content

Commit

Permalink
Merge #215: musig: include pubkey in secnonce and compare when signing
Browse files Browse the repository at this point in the history 
a1ec2bb musig: add test for signing with wrong secnonce for a keypair (Jonas Nick)
bd57a01 musig: include pubkey in secnonce and compare when signing (Jonas Nick)

Pull request description:

  Builds on #211.

  This PR implements a defense-in-depth measure that is specified in BIP-MuSig2. In fact, it revealed a bug in the `scriptless_atomic_swap` test.

ACKs for top commit:
  real-or-random:
    ACK a1ec2bb

Tree-SHA512: dfd54a07c13648e6a7163962bb516cc4ec3a25e4534da2c14a593e2da0f3779eb9b84bfa12ffd94676bb3f6ab86a323e7ec7dee938fd870f36882fee0181ca05
  • Loading branch information
@real-or-random
real-or-random committed Mar 3, 2023
2 parents 4f57024 + a1ec2bb commit c4862f6
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 51 deletions.
17 changes: 13 additions & 4 deletions include/secp256k1_musig.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ typedef struct {

/** Opaque data structure that holds a signer's _secret_ nonce.
*
* Guaranteed to be 68 bytes in size.
* Guaranteed to be 132 bytes in size.
*
* WARNING: This structure MUST NOT be copied or read or written to directly. A
* signer who is online throughout the whole process and can keep this
Expand All @@ -57,7 +57,7 @@ typedef struct {
* leak the secret signing key.
*/
typedef struct {
unsigned char data[68];
unsigned char data[132];
} secp256k1_musig_secnonce;

/** Opaque data structure that holds a signer's public nonce.
Expand Down Expand Up @@ -351,7 +351,9 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_pubkey_xonly_twea
* unless you really know what you are doing.
* seckey: the 32-byte secret key that will later be used for signing, if
* already known (can be NULL)
* pubkey: public key of the signer creating the nonce
* pubkey: public key of the signer creating the nonce. The secnonce
* output of this function cannot be used to sign for any
* other public key.
* msg32: the 32-byte message that will later be signed, if already known
* (can be NULL)
* keyagg_cache: pointer to the keyagg_cache that was used to create the aggregate
Expand Down Expand Up @@ -432,13 +434,20 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_nonce_process(
* reuse. However, this is of course easily defeated if the secnonce has been
* copied (or serialized). Remember that nonce reuse will leak the secret key!
*
* For signing to succeed, the secnonce provided to this function must have
* been generated for the provided keypair. This means that when signing for a
* keypair consisting of a seckey and pubkey, the secnonce must have been
* created by calling musig_nonce_gen with that pubkey. Otherwise, the
* illegal_callback is called.
*
* Returns: 0 if the arguments are invalid or the provided secnonce has already
* been used for signing, 1 otherwise
* Args: ctx: pointer to a context object
* Out: partial_sig: pointer to struct to store the partial signature
* In/Out: secnonce: pointer to the secnonce struct created in
* musig_nonce_gen that has been never used in a
* partial_sign call before
* partial_sign call before and has been created for the
* keypair
* In: keypair: pointer to keypair to sign the message with
* keyagg_cache: pointer to the keyagg_cache that was output when the
* aggregate public key for this session
Expand Down
31 changes: 21 additions & 10 deletions src/modules/musig/session_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,19 @@

static const unsigned char secp256k1_musig_secnonce_magic[4] = { 0x22, 0x0e, 0xdc, 0xf1 };

static void secp256k1_musig_secnonce_save(secp256k1_musig_secnonce *secnonce, secp256k1_scalar *k) {
static void secp256k1_musig_secnonce_save(secp256k1_musig_secnonce *secnonce, const secp256k1_scalar *k, secp256k1_ge *pk) {
memcpy(&secnonce->data[0], secp256k1_musig_secnonce_magic, 4);
secp256k1_scalar_get_b32(&secnonce->data[4], &k[0]);
secp256k1_scalar_get_b32(&secnonce->data[36], &k[1]);
secp256k1_point_save(&secnonce->data[68], pk);
}

static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1_scalar *k, secp256k1_musig_secnonce *secnonce) {
static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1_scalar *k, secp256k1_ge *pk, secp256k1_musig_secnonce *secnonce) {
int is_zero;
ARG_CHECK(secp256k1_memcmp_var(&secnonce->data[0], secp256k1_musig_secnonce_magic, 4) == 0);
secp256k1_scalar_set_b32(&k[0], &secnonce->data[4], NULL);
secp256k1_scalar_set_b32(&k[1], &secnonce->data[36], NULL);
secp256k1_point_load(pk, &secnonce->data[68]);
/* We make very sure that the nonce isn't invalidated by checking the values
* in addition to the magic. */
is_zero = secp256k1_scalar_is_zero(&k[0]) & secp256k1_scalar_is_zero(&k[1]);
Expand All @@ -44,10 +46,12 @@ static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1
/* If flag is true, invalidate the secnonce; otherwise leave it. Constant-time. */
static void secp256k1_musig_secnonce_invalidate(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, int flag) {
secp256k1_memczero(secnonce->data, sizeof(secnonce->data), flag);
/* The flag argument is usually classified. So, above code makes the magic
* classified. However, we need the magic to be declassified to be able to
* compare it during secnonce_load. */
/* The flag argument is usually classified. So, the line above makes the
* magic and public key classified. However, we need both to be
* declassified. Note that we don't declassify the entire object, because if
* flag is 0, then k[0] and k[1] have not been zeroed. */
secp256k1_declassify(ctx, secnonce->data, sizeof(secp256k1_musig_secnonce_magic));
secp256k1_declassify(ctx, &secnonce->data[68], 64);
}

static const unsigned char secp256k1_musig_pubnonce_magic[4] = { 0xf5, 0x7a, 0x3d, 0xa0 };
Expand Down Expand Up @@ -355,6 +359,8 @@ int secp256k1_musig_nonce_gen(const secp256k1_context* ctx, secp256k1_musig_secn
size_t pk_ser_len = sizeof(pk_ser);
unsigned char aggpk_ser[32];
unsigned char *aggpk_ser_ptr = NULL;
secp256k1_ge pk;
int pk_serialize_success;
int ret = 1;

VERIFY_CHECK(ctx != NULL);
Expand Down Expand Up @@ -393,16 +399,19 @@ int secp256k1_musig_nonce_gen(const secp256k1_context* ctx, secp256k1_musig_secn
VERIFY_CHECK(ret_tmp);
aggpk_ser_ptr = aggpk_ser;
}
if (!secp256k1_ec_pubkey_serialize(ctx, pk_ser, &pk_ser_len, pubkey, SECP256K1_EC_COMPRESSED)) {
if (!secp256k1_pubkey_load(ctx, &pk, pubkey)) {
return 0;
}
pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, SECP256K1_EC_COMPRESSED);
/* A pubkey cannot be the point at infinity */
VERIFY_CHECK(pk_serialize_success);
VERIFY_CHECK(pk_ser_len == sizeof(pk_ser));

secp256k1_nonce_function_musig(k, session_id32, msg32, seckey, pk_ser, aggpk_ser_ptr, extra_input32);
VERIFY_CHECK(!secp256k1_scalar_is_zero(&k[0]));
VERIFY_CHECK(!secp256k1_scalar_is_zero(&k[1]));
VERIFY_CHECK(!secp256k1_scalar_eq(&k[0], &k[1]));
secp256k1_musig_secnonce_save(secnonce, k);
secp256k1_musig_secnonce_save(secnonce, k, &pk);
secp256k1_musig_secnonce_invalidate(ctx, secnonce, !ret);

for (i = 0; i < 2; i++) {
Expand Down Expand Up @@ -562,7 +571,7 @@ static void secp256k1_musig_partial_sign_clear(secp256k1_scalar *sk, secp256k1_s

int secp256k1_musig_partial_sign(const secp256k1_context* ctx, secp256k1_musig_partial_sig *partial_sig, secp256k1_musig_secnonce *secnonce, const secp256k1_keypair *keypair, const secp256k1_musig_keyagg_cache *keyagg_cache, const secp256k1_musig_session *session) {
secp256k1_scalar sk;
secp256k1_ge pk;
secp256k1_ge pk, keypair_pk;
secp256k1_scalar k[2];
secp256k1_scalar mu, s;
secp256k1_keyagg_cache_internal cache_i;
Expand All @@ -573,7 +582,7 @@ int secp256k1_musig_partial_sign(const secp256k1_context* ctx, secp256k1_musig_p

ARG_CHECK(secnonce != NULL);
/* Fails if the magic doesn't match */
ret = secp256k1_musig_secnonce_load(ctx, k, secnonce);
ret = secp256k1_musig_secnonce_load(ctx, k, &pk, secnonce);
/* Set nonce to zero to avoid nonce reuse. This will cause subsequent calls
* of this function to fail */
memset(secnonce, 0, sizeof(*secnonce));
Expand All @@ -587,10 +596,12 @@ int secp256k1_musig_partial_sign(const secp256k1_context* ctx, secp256k1_musig_p
ARG_CHECK(keyagg_cache != NULL);
ARG_CHECK(session != NULL);

if (!secp256k1_keypair_load(ctx, &sk, &pk, keypair)) {
if (!secp256k1_keypair_load(ctx, &sk, &keypair_pk, keypair)) {
secp256k1_musig_partial_sign_clear(&sk, k);
return 0;
}
ARG_CHECK(secp256k1_fe_equal_var(&pk.x, &keypair_pk.x)
&& secp256k1_fe_equal_var(&pk.y, &keypair_pk.y));
if (!secp256k1_keyagg_cache_load(ctx, &cache_i, keyagg_cache)) {
secp256k1_musig_partial_sign_clear(&sk, k);
return 0;
Expand Down
Loading

0 comments on commit c4862f6

Please sign in to comment.