Merge pull request #313 from LedgerHQ/musig2-fixes

Improvements from code review

src/common/wallet.h

@@ -290,7 +290,7 @@ typedef struct policy_node_ext_info_s {
 DEFINE_REL_PTR(uint16, uint16_t)
 
 typedef struct {
-    int16_t n;                  // number of key indexes
+    uint16_t n;                 // number of key indexes
     rptr_uint16_t key_indexes;  // pointer to an array of exactly n key indexes
 } musig_aggr_key_info_t;
 
@@ -323,14 +323,14 @@ typedef struct {
     union {
         // type == 0
         struct {
-            int16_t key_index;  // index of the key (common between V1 and V2)
+            uint16_t key_index;  // index of the key (common between V1 and V2)
         } k;
         // type == 1
         struct {
             rptr_musig_aggr_key_info_t musig_info;  // only used in V2
         } m;
     };
-    int16_t
+    uint16_t
         keyexpr_index;  // index of the key expression in the descriptor template, in parsing order
 } policy_node_keyexpr_t;
 
@@ -377,8 +377,8 @@ typedef struct {
 // 12 bytes
 typedef struct {
     struct policy_node_s base;        // type is TOKEN_MULTI or TOKEN_SORTEDMULTI
-    int16_t k;                        // threshold
-    int16_t n;                        // number of keys
+    uint16_t k;                       // threshold
+    uint16_t n;                       // number of keys
     rptr_policy_node_keyexpr_t keys;  // pointer to array of exactly n key expressions
 } policy_node_multisig_t;
 
@@ -395,8 +395,8 @@ typedef struct policy_node_scriptlist_s {
 // 12 bytes, (+ 8 bytes for every script)
 typedef struct {
     struct policy_node_s base;  // type is TOKEN_THRESH
-    int16_t k;                  // threshold
-    int16_t n;                  // number of child scripts
+    uint16_t k;                 // threshold
+    uint16_t n;                 // number of child scripts
     rptr_policy_node_scriptlist_t
         scriptlist;  // pointer to array of exactly n pointers to child scripts
 } policy_node_thresh_t;

src/handler/lib/policy.c

@@ -489,7 +489,9 @@ __attribute__((warn_unused_result)) static int get_derived_pubkey(
         qsort(keys, musig_info->n, sizeof(plain_pk_t), compare_plain_pk);
 
         musig_keyagg_context_t musig_ctx;
-        musig_key_agg(keys, musig_info->n, &musig_ctx);
+        if (0 > musig_key_agg(keys, musig_info->n, &musig_ctx)) {
+            return -1;
+        }
 
         // compute the aggregated extended pubkey
         memset(&ext_pubkey, 0, sizeof(ext_pubkey));

src/handler/sign_psbt.c

@@ -659,7 +659,9 @@ static bool fill_keyexpr_info_if_internal(dispatcher_context_t *dc,
             qsort(keys, musig_info->n, sizeof(plain_pk_t), compare_plain_pk);
 
             musig_keyagg_context_t musig_ctx;
-            musig_key_agg(keys, musig_info->n, &musig_ctx);
+            if (0 > musig_key_agg(keys, musig_info->n, &musig_ctx)) {
+                return false;
+            }
 
             // compute the aggregated extended pubkey
             memset(&keyexpr_info->pubkey, 0, sizeof(keyexpr_info->pubkey));

src/handler/sign_psbt/musig_signing.c

@@ -270,6 +270,7 @@ bool produce_and_yield_pubnonce(dispatcher_context_t *dc,
     musig_secnonce_t secnonce;
     musig_pubnonce_t pubnonce;
     if (0 > musig_nonce_gen(rand_i_j,
+                            sizeof(rand_i_j),
                             keyexpr_info->internal_pubkey.compressed_pubkey,
                             musig_per_input_info.agg_key_tweaked.compressed_pubkey + 1,
                             &secnonce,
@@ -401,6 +402,7 @@ bool __attribute__((noinline)) sign_sighash_musig_and_yield(dispatcher_context_t
     musig_pubnonce_t pubnonce;
 
     if (0 > musig_nonce_gen(rand_i_j,
+                            sizeof(rand_i_j),
                             keyexpr_info->internal_pubkey.compressed_pubkey,
                             musig_per_input_info.agg_key_tweaked.compressed_pubkey + 1,
                             &secnonce,

src/musig/musig.c

@@ -73,9 +73,9 @@ static int point_negate(const point_t *P, point_t *out) {
         set_point_infinite(out);
         return 0;
     }
-    memmove(out->x, P->x, 32);
+    memmove(out->x, P->x, sizeof(out->x));
 
-    if (CX_OK != cx_math_sub_no_throw(out->y, secp256k1_p, P->y, 32)) return -1;
+    if (CX_OK != cx_math_sub_no_throw(out->y, secp256k1_p, P->y, sizeof(out->y))) return -1;
 
     out->prefix = 4;
     return 0;
@@ -114,9 +114,9 @@ static bool is_array_zero(const uint8_t buffer[], size_t buffer_len) {
     return acc == 0;
 }
 
-int cpoint_ext(const uint8_t x[static 33], point_t *out) {
+static int cpoint_ext(const uint8_t x[static sizeof(plain_pk_t)], point_t *out) {
     // Check if the point is at infinity (all bytes zero)
-    if (is_array_zero(x, 33)) {
+    if (is_array_zero(x, sizeof(plain_pk_t))) {
         set_point_infinite(out);
         return 0;
     }
@@ -135,15 +135,17 @@ static void musig_get_second_key(const plain_pk_t pubkeys[], size_t n_keys, plai
     memset(out, 0, sizeof(plain_pk_t));
 }
 
-static void musig_hash_keys(const plain_pk_t pubkeys[], size_t n_keys, uint8_t out[static 32]) {
+static void musig_hash_keys(const plain_pk_t pubkeys[],
+                            size_t n_keys,
+                            uint8_t out[static CX_SHA256_SIZE]) {
     cx_sha256_t hash_context;
     crypto_tr_tagged_hash_init(&hash_context,
                                BIP0327_keyagg_list_tag,
                                sizeof(BIP0327_keyagg_list_tag));
     for (size_t i = 0; i < n_keys; i++) {
         crypto_hash_update(&hash_context.header, pubkeys[i], sizeof(plain_pk_t));
     }
-    crypto_hash_digest(&hash_context.header, out, 32);
+    crypto_hash_digest(&hash_context.header, out, CX_SHA256_SIZE);
 }
 
 static void musig_key_agg_coeff_internal(const plain_pk_t pubkeys[],
@@ -207,13 +209,20 @@ int musig_key_agg(const plain_pk_t pubkeys[], size_t n_keys, musig_keyagg_contex
 
         point_add(&ctx->Q, &P, &ctx->Q);
     }
+
+    if (is_point_infinite(&ctx->Q)) {
+        PRINTF("Musig key aggregation resulted in an infinite point\n");
+        return -1;
+    }
+
     memset(ctx->tacc, 0, sizeof(ctx->tacc));
     memset(ctx->gacc, 0, sizeof(ctx->gacc));
     ctx->gacc[31] = 1;
     return 0;
 }
 
 static void musig_nonce_hash(const uint8_t *rand,
+                             size_t rand_len,
                              const plain_pk_t pk,
                              const xonly_pk_t aggpk,
                              uint8_t i,
@@ -226,7 +235,7 @@ static void musig_nonce_hash(const uint8_t *rand,
     crypto_tr_tagged_hash_init(&hash_context, BIP0327_nonce_tag, sizeof(BIP0327_nonce_tag));
 
     // rand
-    crypto_hash_update(&hash_context.header, rand, 32);
+    crypto_hash_update(&hash_context.header, rand, rand_len);
 
     // len(pk) + pk
     crypto_hash_update_u8(&hash_context.header, sizeof(plain_pk_t));
@@ -252,19 +261,26 @@ static void musig_nonce_hash(const uint8_t *rand,
 
 // same as nonce_gen_internal from the reference, removing the optional arguments sk, msg and
 // extra_in, and making aggpk compulsory
-int musig_nonce_gen(const uint8_t rand[32],
+int musig_nonce_gen(const uint8_t *rand,
+                    size_t rand_len,
                     const plain_pk_t pk,
                     const xonly_pk_t aggpk,
                     musig_secnonce_t *secnonce,
                     musig_pubnonce_t *pubnonce) {
     uint8_t msg[] = {0x00};
 
-    musig_nonce_hash(rand, pk, aggpk, 0, msg, 1, NULL, 0, secnonce->k_1);
+    musig_nonce_hash(rand, rand_len, pk, aggpk, 0, msg, 1, NULL, 0, secnonce->k_1);
     if (CX_OK != cx_math_modm_no_throw(secnonce->k_1, 32, secp256k1_n, 32)) return -1;
-    musig_nonce_hash(rand, pk, aggpk, 1, msg, 1, NULL, 0, secnonce->k_2);
+    musig_nonce_hash(rand, rand_len, pk, aggpk, 1, msg, 1, NULL, 0, secnonce->k_2);
     if (CX_OK != cx_math_modm_no_throw(secnonce->k_2, 32, secp256k1_n, 32)) return -1;
 
-    memcpy(secnonce->pk, pk, 33);
+    if (is_array_zero(secnonce->k_1, sizeof(secnonce->k_1)) ||
+        is_array_zero(secnonce->k_2, sizeof(secnonce->k_2))) {
+        // this can only happen with negligible probability
+        return -1;
+    }
+
+    memcpy(secnonce->pk, pk, sizeof(secnonce->pk));
 
     point_t R_s1, R_s2;
 
@@ -283,17 +299,17 @@ int musig_nonce_agg(const musig_pubnonce_t pubnonces[], size_t n_keys, musig_pub
         set_point_infinite(&R_j);
         for (size_t i = 0; i < n_keys; i++) {
             point_t R_ij;
-            if (0 > cpoint(&pubnonces[i].raw[(j - 1) * 33], &R_ij)) {
+            if (0 > cpoint(&pubnonces[i].raw[(j - 1) * sizeof(plain_pk_t)], &R_ij)) {
                 PRINTF("Musig2 nonce aggregation: invalid contribution from cosigner %d\n", i);
                 return -i - 1;
             }
             point_add(&R_j, &R_ij, &R_j);
         }
 
         if (is_point_infinite(&R_j)) {
-            memset(&out->raw[(j - 1) * 33], 0, 33);
+            memset(&out->raw[(j - 1) * sizeof(plain_pk_t)], 0, sizeof(plain_pk_t));
         } else {
-            crypto_get_compressed_pubkey(R_j.raw, &out->raw[(j - 1) * 33]);
+            crypto_get_compressed_pubkey(R_j.raw, &out->raw[(j - 1) * sizeof(plain_pk_t)]);
         }
     }
     return 0;
@@ -362,14 +378,18 @@ static int musig_get_session_values(const musig_session_context_t *session_ctx,
                                     point_t *Q,
                                     uint8_t gacc[static 32],
                                     uint8_t tacc[static 32],
-                                    uint8_t b[static 32],
+                                    uint8_t b[static CX_SHA256_SIZE],
                                     point_t *R,
-                                    uint8_t e[static 32]) {
+                                    uint8_t e[static CX_SHA256_SIZE]) {
     cx_sha256_t hash_context;
 
     // Perform key aggregation and tweaking
     musig_keyagg_context_t keyagg_ctx;
-    musig_key_agg(session_ctx->pubkeys, session_ctx->n_keys, &keyagg_ctx);
+
+    if (0 > musig_key_agg(session_ctx->pubkeys, session_ctx->n_keys, &keyagg_ctx)) {
+        return -1;
+    }
+
     for (size_t i = 0; i < session_ctx->n_tweaks; i++) {
         if (0 > apply_tweak(&keyagg_ctx, session_ctx->tweaks[i], session_ctx->is_xonly[i])) {
             return -1;
@@ -383,10 +403,12 @@ static int musig_get_session_values(const musig_session_context_t *session_ctx,
 
     // Calculate b
     crypto_tr_tagged_hash_init(&hash_context, BIP0327_noncecoef_tag, sizeof(BIP0327_noncecoef_tag));
-    crypto_hash_update(&hash_context.header, session_ctx->aggnonce->raw, 66);
-    crypto_hash_update(&hash_context.header, Q->x, 32);
+    crypto_hash_update(&hash_context.header,
+                       session_ctx->aggnonce->raw,
+                       sizeof(session_ctx->aggnonce->raw));
+    crypto_hash_update(&hash_context.header, Q->x, sizeof(Q->x));
     crypto_hash_update(&hash_context.header, session_ctx->msg, session_ctx->msg_len);
-    crypto_hash_digest(&hash_context.header, b, 32);
+    crypto_hash_digest(&hash_context.header, b, CX_SHA256_SIZE);
 
     // Calculate R
     point_t R_1, R_2;
@@ -411,10 +433,10 @@ static int musig_get_session_values(const musig_session_context_t *session_ctx,
 
     // Calculate e
     crypto_tr_tagged_hash_init(&hash_context, BIP0340_challenge_tag, sizeof(BIP0340_challenge_tag));
-    crypto_hash_update(&hash_context.header, R->x, 32);
-    crypto_hash_update(&hash_context.header, Q->x, 32);
+    crypto_hash_update(&hash_context.header, R->x, sizeof(R->x));
+    crypto_hash_update(&hash_context.header, Q->x, sizeof(Q->x));
     crypto_hash_update(&hash_context.header, session_ctx->msg, session_ctx->msg_len);
-    crypto_hash_digest(&hash_context.header, e, 32);
+    crypto_hash_digest(&hash_context.header, e, CX_SHA256_SIZE);
     return 0;
 }
 
@@ -448,9 +470,9 @@ int musig_sign(musig_secnonce_t *secnonce,
     point_t Q;
     uint8_t gacc[32];
     uint8_t tacc[32];
-    uint8_t b[32];
+    uint8_t b[CX_SHA256_SIZE];
     point_t R;
-    uint8_t e[32];
+    uint8_t e[CX_SHA256_SIZE];
 
     int diff;
 
@@ -460,8 +482,8 @@ int musig_sign(musig_secnonce_t *secnonce,
 
     uint8_t k_1[32];
     uint8_t k_2[32];
-    memcpy(k_1, secnonce->k_1, 32);
-    memcpy(k_2, secnonce->k_2, 32);
+    memcpy(k_1, secnonce->k_1, sizeof(k_1));
+    memcpy(k_2, secnonce->k_2, sizeof(k_2));
 
     // paranoia: since reusing nonces is catastrophic, we make sure that they are zeroed out and
     // work with a local copy instead
@@ -522,7 +544,7 @@ int musig_sign(musig_secnonce_t *secnonce,
         plain_pk_t pk;
         crypto_get_compressed_pubkey(secrets.P.raw, pk);
 
-        if (memcmp(pk, secnonce->pk, 33) != 0) {
+        if (memcmp(pk, secnonce->pk, sizeof(pk)) != 0) {
             err = true;
             PRINTF("Public key does not match nonce_gen argument\n");
             break;

src/musig/musig.h

@@ -14,15 +14,13 @@ typedef uint8_t xonly_pk_t[32];
 
 // An uncompressed pubkey, encoded as 04||x||y, where x and y are 32-byte big-endian coordinates.
 // If the first byte (prefix) is 0, encodes the point at infinity.
-typedef struct {
-    union {
-        uint8_t raw[65];
-        struct {
-            uint8_t prefix;  // 0 for the point at infinity, otherwise 4.
-            uint8_t x[32];
-            uint8_t y[32];
-        };
-    };
+typedef union {
+    uint8_t raw[65];
+    struct {
+        uint8_t prefix;  // 0 for the point at infinity, otherwise 4.
+        uint8_t x[32];
+        uint8_t y[32];
+    } __attribute__((packed));
 } point_t;
 
 typedef struct musig_keyagg_context_s {
@@ -37,14 +35,12 @@ typedef struct musig_secnonce_s {
     uint8_t pk[33];
 } musig_secnonce_t;
 
-typedef struct musig_pubnonce_s {
-    union {
-        struct {
-            uint8_t R_s1[33];
-            uint8_t R_s2[33];
-        };
-        uint8_t raw[66];
-    };
+typedef union {
+    struct {
+        uint8_t R_s1[33];
+        uint8_t R_s2[33];
+    } __attribute__((packed));
+    uint8_t raw[66];
 } musig_pubnonce_t;
 
 typedef struct musig_session_context_s {
@@ -83,6 +79,8 @@ int musig_key_agg(const plain_pk_t pubkeys[], size_t n_keys, musig_keyagg_contex
  *
  * @param[in]  rand
  *   The randomness to use.
+ * @param[in]  rand_len
+ *  The length of the randomness.
  * @param[in]  pk
  *   The 33-byte public key of the signer.
  * @param[in]  aggpk
@@ -94,7 +92,8 @@ int musig_key_agg(const plain_pk_t pubkeys[], size_t n_keys, musig_keyagg_contex
  *
  * @return 0 on success, a negative number in case of error.
  */
-int musig_nonce_gen(const uint8_t rand[32],
+int musig_nonce_gen(const uint8_t *rand,
+                    size_t rand_len,
                     const plain_pk_t pk,
                     const xonly_pk_t aggpk,
                     musig_secnonce_t *secnonce,

src/musig/musig_sessions.c

@@ -132,12 +132,12 @@ const musig_psbt_session_t *musigsession_round2_initialize(
 
 void musigsession_commit(musig_signing_state_t *musig_signing_state) {
     uint8_t acc = 0;
-    for (size_t i = 0; i < sizeof(musig_signing_state->_round1); i++) {
+    for (size_t i = 0; i < sizeof(musig_signing_state->_round1._id); i++) {
         acc |= musig_signing_state->_round1._id[i];
     }
     // If round 1 was not executed, then there is nothing to store.
-    // This assumes that musigsession_initialize_signing_state, therefore the field is zeroed out
-    // if it wasn't used.
+    // This assumes that musigsession_initialize_signing_state zeroes the id, therefore the field is
+    // zeroed out if and only if it wasn't used.
     if (acc != 0) {
         musigsession_store(musig_signing_state->_round1._id, &musig_signing_state->_round1);
     }