frost/secp256k1-zkp
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
8bcda186d2 test: Add non-NULL checks for "pointer of array" API functions (Sebastian Falbesoner)
5a08c1bcdc 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:
  e7f7083b53/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](https://github.com/bitcoin-core/secp256k1/pull/1765#discussion_r2590644412) (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](8bcda186d2)
  john-moffett:
    utACK 8bcda186d2
  real-or-random:
    utACK 8bcda186d2
  w0xlt:
    ACK 8bcda186d2

Tree-SHA512: 24acd6606526e3acb994e3361fde15771aa6706a6f3e7a6ae70b9a9ddb81ac1eedaac2025a027b890cecf98dab20dc378b94edde6c726888c44b9d35b7581ee1
This commit is contained in:
merge-script
2025-12-10 09:27:47 +01:00
parent e7f7083b53 8bcda186d2
commit be5e4f02fd
5 changed files with 54 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/modules/musig/keyagg_impl.h
View File

@@ -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;

8
src/modules/musig/session_impl.h
View File

@@ -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;

21
src/modules/musig/tests_impl.h
View File

@@ -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));

5
src/secp256k1.c
View File

@@ -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)

17
src/tests.c
View File

@@ -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 */