Merge #1779: Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL

8bcda18 test: Add non-NULL checks for "pointer of array" API functions (Sebastian Falbesoner)
5a08c1b Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL (Sebastian Falbesoner)

Pull request description:

  We currently have five public API functions that take an "array of pointers" as input parameter:
  * `secp256k1_ec_pubkey_combine` (`ins`: array of pointers to public keys to add)
  * `secp256k1_ec_pubkey_sort` (`pubkeys`: array of pointers to public keys to sort)
  * `secp256k1_musig_pubkey_agg` (`pubkeys`: array of pointers to public keys to aggregate)
  * `secp256k1_musig_nonce_agg` (`pubnonces`: array of pointers to public nonces to aggregate)
  * `secp256k1_musig_partial_sig_agg` (`partial_sigs`: array of pointers to partial signatures to aggregate)

  Out of these, only `_ec_pubkey_combine` verifies that the individual pointer elements in the array are non-NULL each:
  https://github.com/bitcoin-core/secp256k1/blob/e7f7083b530a55c83ce9089a7244d2d9d67ac8b2/src/secp256k1.c#L774-L775

  This PR adds corresponding `ARG_CHECKS` for the other API functions as well, in order to avoid running into potential UB due to NULL pointer dereference. It seems to me that the tiny run-time overhead is worth it doing this for consistency and to help users in case the arrays are set up incorrectly (I'm thinking e.g. of language binding writers where getting this right might be a bit more involved).

  Looking into this was motivated by a [review of furszy](#1765 (comment)) (thanks!), who pointed out that the non-NULL checks are missing in at least one API function in the silentpayments module PR as well. Happy to add some `CHECK_ILLEGAL` tests if there is conceptual support for this PR.

ACKs for top commit:
  kevkevinpal:
    utACK [8bcda18](8bcda18)
  john-moffett:
    utACK 8bcda18
  real-or-random:
    utACK 8bcda18
  w0xlt:
    ACK 8bcda18

Tree-SHA512: 24acd6606526e3acb994e3361fde15771aa6706a6f3e7a6ae70b9a9ddb81ac1eedaac2025a027b890cecf98dab20dc378b94edde6c726888c44b9d35b7581ee1

Author: real-or-random

src/modules/musig/keyagg_impl.h

@@ -177,6 +177,9 @@ int secp256k1_musig_pubkey_agg(const secp256k1_context* ctx, secp256k1_xonly_pub
     }
     ARG_CHECK(pubkeys != NULL);
     ARG_CHECK(n_pubkeys > 0);
+    for (i = 0; i < n_pubkeys; i++) {
+        ARG_CHECK(pubkeys[i] != NULL);
+    }
 
     ecmult_data.ctx = ctx;
     ecmult_data.pks = pubkeys;

src/modules/musig/session_impl.h

@@ -521,10 +521,15 @@ static int secp256k1_musig_sum_pubnonces(const secp256k1_context* ctx, secp256k1
 int secp256k1_musig_nonce_agg(const secp256k1_context* ctx, secp256k1_musig_aggnonce  *aggnonce, const secp256k1_musig_pubnonce * const* pubnonces, size_t n_pubnonces) {
     secp256k1_gej aggnonce_ptsj[2];
     secp256k1_ge aggnonce_pts[2];
+    size_t i;
+
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(aggnonce != NULL);
     ARG_CHECK(pubnonces != NULL);
     ARG_CHECK(n_pubnonces > 0);
+    for (i = 0; i < n_pubnonces; i++) {
+        ARG_CHECK(pubnonces[i] != NULL);
+    }
 
     if (!secp256k1_musig_sum_pubnonces(ctx, aggnonce_ptsj, pubnonces, n_pubnonces)) {
         return 0;
@@ -782,6 +787,9 @@ int secp256k1_musig_partial_sig_agg(const secp256k1_context* ctx, unsigned char
     ARG_CHECK(session != NULL);
     ARG_CHECK(partial_sigs != NULL);
     ARG_CHECK(n_sigs > 0);
+    for (i = 0; i < n_sigs; i++) {
+        ARG_CHECK(partial_sigs[i] != NULL);
+    }
 
     if (!secp256k1_musig_session_load(ctx, &session_i, session)) {
         return 0;

src/modules/musig/tests_impl.h

@@ -201,6 +201,13 @@ static void musig_api_tests(void) {
     CHECK(secp256k1_musig_pubkey_agg(CTX, &agg_pk, &keyagg_cache, pk_ptr, 2) == 1);
     CHECK(secp256k1_musig_pubkey_agg(CTX, NULL, &keyagg_cache, pk_ptr, 2) == 1);
     CHECK(secp256k1_musig_pubkey_agg(CTX, &agg_pk, NULL, pk_ptr, 2) == 1);
+    /* check that NULL in array of public key pointers is not allowed */
+    for (i = 0; i < 2; i++) {
+        const secp256k1_pubkey *original_ptr = pk_ptr[i];
+        pk_ptr[i] = NULL;
+        CHECK_ILLEGAL(CTX, secp256k1_musig_pubkey_agg(CTX, &agg_pk, NULL, pk_ptr, 2));
+        pk_ptr[i] = original_ptr;
+    }
     CHECK_ILLEGAL(CTX, secp256k1_musig_pubkey_agg(CTX, &agg_pk, &keyagg_cache, NULL, 2));
     CHECK(memcmp_and_randomize(agg_pk.data, zeros132, sizeof(agg_pk.data)) == 0);
     CHECK_ILLEGAL(CTX, secp256k1_musig_pubkey_agg(CTX, &agg_pk, &keyagg_cache, invalid_pk_ptr2, 2));
@@ -350,6 +357,13 @@ static void musig_api_tests(void) {
 
     /** Receive nonces and aggregate **/
     CHECK(secp256k1_musig_nonce_agg(CTX, &aggnonce, pubnonce_ptr, 2) == 1);
+    /* check that NULL in array of public nonce pointers is not allowed */
+    for (i = 0; i < 2; i++) {
+        const secp256k1_musig_pubnonce *original_ptr = pubnonce_ptr[i];
+        pubnonce_ptr[i] = NULL;
+        CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_agg(CTX, &aggnonce, pubnonce_ptr, 2));
+        pubnonce_ptr[i] = original_ptr;
+    }
     CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_agg(CTX, NULL, pubnonce_ptr, 2));
     CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_agg(CTX, &aggnonce, NULL, 2));
     CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_agg(CTX, &aggnonce, pubnonce_ptr, 0));
@@ -474,6 +488,13 @@ static void musig_api_tests(void) {
 
     /** Signature aggregation and verification */
     CHECK(secp256k1_musig_partial_sig_agg(CTX, pre_sig, &session, partial_sig_ptr, 2) == 1);
+    /* check that NULL in array of partial signature pointers is not allowed */
+    for (i = 0; i < 2; i++) {
+        const secp256k1_musig_partial_sig *original_ptr = partial_sig_ptr[i];
+        partial_sig_ptr[i] = NULL;
+        CHECK_ILLEGAL(CTX, secp256k1_musig_partial_sig_agg(CTX, pre_sig, &session, partial_sig_ptr, 2));
+        partial_sig_ptr[i] = original_ptr;
+    }
     CHECK_ILLEGAL(CTX, secp256k1_musig_partial_sig_agg(CTX, NULL, &session, partial_sig_ptr, 2));
     CHECK_ILLEGAL(CTX, secp256k1_musig_partial_sig_agg(CTX, pre_sig, NULL, partial_sig_ptr, 2));
     CHECK_ILLEGAL(CTX, secp256k1_musig_partial_sig_agg(CTX, pre_sig, &invalid_session, partial_sig_ptr, 2));

src/secp256k1.c

@@ -325,8 +325,13 @@ static int secp256k1_ec_pubkey_sort_cmp(const void* pk1, const void* pk2, void *
 }
 
 int secp256k1_ec_pubkey_sort(const secp256k1_context* ctx, const secp256k1_pubkey **pubkeys, size_t n_pubkeys) {
+    size_t i;
+
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(pubkeys != NULL);
+    for (i = 0; i < n_pubkeys; i++) {
+        ARG_CHECK(pubkeys[i] != NULL);
+    }
 
     /* Suppress wrong warning (fixed in MSVC 19.33) */
     #if defined(_MSC_VER) && (_MSC_VER < 1933)

src/tests.c

@@ -6052,6 +6052,7 @@ static void run_eckey_edge_case_test(void) {
     secp256k1_pubkey pubkey_negone;
     const secp256k1_pubkey *pubkeys[3];
     size_t len;
+    int i;
     /* Group order is too large, reject. */
     CHECK(secp256k1_ec_seckey_verify(CTX, orderc) == 0);
     SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
@@ -6245,6 +6246,14 @@ static void run_eckey_edge_case_test(void) {
     CHECK(secp256k1_ec_pubkey_combine(CTX, &pubkey, pubkeys, 3) == 1);
     SECP256K1_CHECKMEM_CHECK(&pubkey, sizeof(secp256k1_pubkey));
     CHECK(secp256k1_memcmp_var(&pubkey, zeros, sizeof(secp256k1_pubkey)) > 0);
+    /* check that NULL in array of pubkey pointers is not allowed */
+    for (i = 0; i < 3; i++) {
+        const secp256k1_pubkey *original_ptr = pubkeys[i];
+        secp256k1_pubkey result;
+        pubkeys[i] = NULL;
+        CHECK_ILLEGAL(CTX, secp256k1_ec_pubkey_combine(CTX, &result, pubkeys, 3));
+        pubkeys[i] = original_ptr;
+    }
     len = 33;
     CHECK(secp256k1_ec_pubkey_serialize(CTX, ctmp, &len, &pubkey, SECP256K1_EC_COMPRESSED) == 1);
     CHECK(secp256k1_ec_pubkey_serialize(CTX, ctmp2, &len, &pubkey_one, SECP256K1_EC_COMPRESSED) == 1);
@@ -6640,6 +6649,7 @@ static void permute(size_t *arr, size_t n) {
 static void test_sort_api(void) {
     secp256k1_pubkey pks[2];
     const secp256k1_pubkey *pks_ptr[2];
+    int i;
 
     pks_ptr[0] = &pks[0];
     pks_ptr[1] = &pks[1];
@@ -6648,6 +6658,13 @@ static void test_sort_api(void) {
     testutil_random_pubkey_test(&pks[1]);
 
     CHECK(secp256k1_ec_pubkey_sort(CTX, pks_ptr, 2) == 1);
+    /* check that NULL in array of public key pointers is not allowed */
+    for (i = 0; i < 2; i++) {
+        const secp256k1_pubkey *original_ptr = pks_ptr[i];
+        pks_ptr[i] = NULL;
+        CHECK_ILLEGAL(CTX, secp256k1_ec_pubkey_sort(CTX, pks_ptr, 2));
+        pks_ptr[i] = original_ptr;
+    }
     CHECK_ILLEGAL(CTX, secp256k1_ec_pubkey_sort(CTX, NULL, 2));
     CHECK(secp256k1_ec_pubkey_sort(CTX, pks_ptr, 0) == 1);
     /* Test illegal public keys */