f set musig limit to 2^32 and hash fixed 4 bytes in musig coefficient

Author: jonasnick

include/secp256k1_musig.h

@@ -1,6 +1,8 @@
 #ifndef SECP256K1_MUSIG_H
 #define SECP256K1_MUSIG_H
 
+#include <stdint.h>
+
 /** This module implements a Schnorr-based multi-signature scheme called MuSig
  * (https://eprint.iacr.org/2018/068.pdf). There's an example C source file in the
  * module's directory (src/modules/musig/example.c) that demonstrates how it can be
@@ -53,7 +55,7 @@
  */
 typedef struct {
     secp256k1_pubkey combined_pk;
-    size_t n_signers;
+    uint32_t n_signers;
     unsigned char pk_hash[32];
     secp256k1_pubkey combined_nonce;
     int nonce_is_set;
@@ -97,7 +99,7 @@ typedef struct {
  */
 typedef struct {
     int present;
-    size_t index;
+    uint32_t index;
     secp256k1_pubkey nonce;
     unsigned char nonce_commitment[32];
 } secp256k1_musig_session_signer_data;
@@ -161,7 +163,7 @@ SECP256K1_API int secp256k1_musig_pubkey_combine(
  *          pk_hash32: the 32-byte hash of the signers' individual keys (cannot be
  *                     NULL)
  *          n_signers: length of signers array. Number of signers participating in
- *                     the MuSig. Must be greater than 0.
+ *                     the MuSig. Must be greater than 0 and smaller than 2^32 - 1.
  *           my_index: index of this signer in the signers array
  *             seckey: the signer's 32-byte secret key (cannot be NULL)
  */
@@ -262,7 +264,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_set_nonce(
  *                    set with `musig_set_nonce`. Array length must equal to `n_signers`
  *                    (cannot be NULL)
  *         n_signers: the length of the signers array. Must be the total number of
- *                    signers participating in the MuSig.
+ *                    signers participating in the MuSig. Must be greater than 0 and
+ *                    smaller than 2^32.
  *  Out: nonce_is_negated: a pointer to an integer that indicates if the combined
  *                    public nonce had to be negated.
  *           adaptor: point to add to the combined public nonce. If NULL, nothing is

src/modules/musig/main_impl.h

@@ -29,11 +29,12 @@ static int secp256k1_musig_compute_ell(const secp256k1_context *ctx, unsigned ch
     return 1;
 }
 
-/* Compute r = SHA256(ell, idx). The serialization of idx is least significant byte
- * first and variable-length such that the last byte is non-zero. */
-static void secp256k1_musig_coefficient(secp256k1_scalar *r, const unsigned char *ell, size_t idx) {
+/* Compute r = SHA256(ell, idx). The four bytes of idx are serialized least significant byte first. */
+static void secp256k1_musig_coefficient(secp256k1_scalar *r, const unsigned char *ell, uint32_t idx) {
     secp256k1_sha256 sha;
     unsigned char buf[32];
+    size_t i;
+
     secp256k1_sha256_initialize(&sha);
     secp256k1_sha256_write(&sha, ell, 32);
     /* We're hashing the index of the signer instead of its public key as specified
@@ -48,13 +49,12 @@ static void secp256k1_musig_coefficient(secp256k1_scalar *r, const unsigned char
      * equivalent to hashing the public key. Because the public key can be
      * identified by the index given the ordered list of public keys (included in
      * ell), the index is just a different encoding of the public key.*/
-    while (idx > 0) {
+    for (i = 0; i < sizeof(uint32_t); i++) {
         unsigned char c = idx;
         secp256k1_sha256_write(&sha, &c, 1);
-        idx /= 0x100;
+        idx >>= 8;
     }
     secp256k1_sha256_finalize(&sha, buf);
-
     secp256k1_scalar_set_b32(r, buf, NULL);
 }
 
@@ -72,8 +72,8 @@ static int secp256k1_musig_pubkey_combine_callback(secp256k1_scalar *sc, secp256
 }
 
 
-static void secp256k1_musig_signers_init(secp256k1_musig_session_signer_data *signers, size_t n_signers) {
-    size_t i;
+static void secp256k1_musig_signers_init(secp256k1_musig_session_signer_data *signers, uint32_t n_signers) {
+    uint32_t i;
     for (i = 0; i < n_signers; i++) {
         memset(&signers[i], 0, sizeof(signers[i]));
         signers[i].index = i;
@@ -144,8 +144,11 @@ int secp256k1_musig_session_initialize(const secp256k1_context* ctx, secp256k1_m
     if (n_signers == 0 || my_index >= n_signers) {
         return 0;
     }
-    session->n_signers = n_signers;
-    secp256k1_musig_signers_init(signers, n_signers);
+    if (n_signers > UINT32_MAX) {
+        return 0;
+    }
+    session->n_signers = (uint32_t) n_signers;
+    secp256k1_musig_signers_init(signers, session->n_signers);
     session->nonce_commitments_hash_is_set = 0;
 
     /* Compute secret key */
@@ -154,7 +157,7 @@ int secp256k1_musig_session_initialize(const secp256k1_context* ctx, secp256k1_m
         secp256k1_scalar_clear(&secret);
         return 0;
     }
-    secp256k1_musig_coefficient(&mu, pk_hash32, my_index);
+    secp256k1_musig_coefficient(&mu, pk_hash32, (uint32_t) my_index);
     secp256k1_scalar_mul(&secret, &secret, &mu);
     secp256k1_scalar_get_b32(session->seckey, &secret);
 
@@ -238,6 +241,11 @@ int secp256k1_musig_session_initialize_verifier(const secp256k1_context* ctx, se
     ARG_CHECK(combined_pk != NULL);
     ARG_CHECK(pk_hash32 != NULL);
     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;
+    }
     for (i = 0; i < n_signers; i++) {
         ARG_CHECK(commitments[i] != NULL);
     }
@@ -249,8 +257,8 @@ int secp256k1_musig_session_initialize_verifier(const secp256k1_context* ctx, se
     if (n_signers == 0) {
         return 0;
     }
-    session->n_signers = n_signers;
-    secp256k1_musig_signers_init(signers, n_signers);
+    session->n_signers = (uint32_t) n_signers;
+    secp256k1_musig_signers_init(signers, session->n_signers);
 
     memcpy(session->pk_hash, pk_hash32, 32);
     session->nonce_is_set = 0;

src/modules/musig/tests_impl.h

@@ -122,6 +122,12 @@ void musig_api_tests(secp256k1_scratch_space *scratch) {
     CHECK(ecount == 8);
     CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, pk_hash, 0, 0, sk[0]) == 0);
     CHECK(ecount == 8);
+    /* If more than UINT32_MAX fits in a size_t, test that session_initialize
+     * rejects n_signers that high. */
+    if (SIZE_MAX > ((size_t) UINT32_MAX) + 1) {
+        CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, pk_hash, ((size_t) UINT32_MAX) + 2, 0, sk[0]) == 0);
+    }
+    CHECK(ecount == 8);
     CHECK(secp256k1_musig_session_initialize(sign, &session[0], signer0, nonce_commitment[0], session_id[0], msg, &combined_pk, pk_hash, 2, 0, NULL) == 0);
     CHECK(ecount == 9);
     /* secret key overflows */
@@ -155,6 +161,10 @@ void musig_api_tests(secp256k1_scratch_space *scratch) {
     CHECK(ecount == 4);
     CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, pk_hash, ncs, 0) == 0);
     CHECK(ecount == 4);
+    if (SIZE_MAX > ((size_t) UINT32_MAX) + 1) {
+        CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, pk_hash, ncs, ((size_t) UINT32_MAX) + 2) == 0);
+    }
+    CHECK(ecount == 4);
     CHECK(secp256k1_musig_session_initialize_verifier(none, &verifier_session, verifier_signer_data, msg, &combined_pk, pk_hash, ncs, 2) == 1);
 
     CHECK(secp256k1_musig_compute_messagehash(none, msghash, &verifier_session) == 0);