From 19be9a6db90bb79b11c7c077164cb31d0c319c15 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sat, 26 Oct 2024 22:26:41 +0200 Subject: [PATCH] fix and verify `compressed` argument in _eckey_pubkey_serialize calls In several calls of the internal function `secp256k1_eckey_pubkey_serialize`, the public API flag `SECP256K1_EC_COMPRESSED` is passed, which is meant to be only used for the public function `secp256k1_ec_pubkey_serialize`. It works as intended in all of those cases (it wouldn't for `..._UNCOMPRESSED` though), but it's still kind of a type mismatch that can't be detected by the compiler. To avoid cases like this in the future, a VERIFY_CHECK is added that the `compressed` parameter needs to be either 0 or 1. --- src/eckey_impl.h | 3 +++ src/modules/ecdsa_adaptor/main_impl.h | 2 +- src/modules/musig/session_impl.h | 2 +- src/modules/whitelist/whitelist_impl.h | 8 ++++---- src/secp256k1.c | 2 +- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/eckey_impl.h b/src/eckey_impl.h index 121966f8b..a88a5964d 100644 --- a/src/eckey_impl.h +++ b/src/eckey_impl.h @@ -9,6 +9,7 @@ #include "eckey.h" +#include "util.h" #include "scalar.h" #include "field.h" #include "group.h" @@ -35,6 +36,8 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char } static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed) { + VERIFY_CHECK(compressed == 0 || compressed == 1); + if (secp256k1_ge_is_infinity(elem)) { return 0; } diff --git a/src/modules/ecdsa_adaptor/main_impl.h b/src/modules/ecdsa_adaptor/main_impl.h index 77f1627b2..1004c344b 100644 --- a/src/modules/ecdsa_adaptor/main_impl.h +++ b/src/modules/ecdsa_adaptor/main_impl.h @@ -339,7 +339,7 @@ int secp256k1_ecdsa_adaptor_recover(const secp256k1_context* ctx, unsigned char /* We declassify non-secret enckey_expected_ge to allow using it as a * branch point. */ secp256k1_declassify(ctx, &enckey_expected_ge, sizeof(enckey_expected_ge)); - if (!secp256k1_eckey_pubkey_serialize(&enckey_expected_ge, enckey_expected33, &size, SECP256K1_EC_COMPRESSED)) { + if (!secp256k1_eckey_pubkey_serialize(&enckey_expected_ge, enckey_expected33, &size, 1)) { /* Unreachable from tests (and other VERIFY builds) and therefore this * branch should be ignored in test coverage analysis. * diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h index ff87f2fd3..796e2c3ac 100644 --- a/src/modules/musig/session_impl.h +++ b/src/modules/musig/session_impl.h @@ -368,7 +368,7 @@ int secp256k1_musig_nonce_gen(const secp256k1_context* ctx, secp256k1_musig_secn 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); + pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, 1); #ifdef VERIFY /* A pubkey cannot be the point at infinity */ diff --git a/src/modules/whitelist/whitelist_impl.h b/src/modules/whitelist/whitelist_impl.h index 8d6911271..fd4352a1d 100644 --- a/src/modules/whitelist/whitelist_impl.h +++ b/src/modules/whitelist/whitelist_impl.h @@ -18,7 +18,7 @@ static int secp256k1_whitelist_hash_pubkey(secp256k1_scalar* output, secp256k1_g secp256k1_ge_set_gej(&ge, pubkey); secp256k1_sha256_initialize(&sha); - if (!secp256k1_eckey_pubkey_serialize(&ge, c, &size, SECP256K1_EC_COMPRESSED)) { + if (!secp256k1_eckey_pubkey_serialize(&ge, c, &size, 1)) { return 0; } secp256k1_sha256_write(&sha, c, size); @@ -94,7 +94,7 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context* secp256k1_pubkey_load(ctx, &subkey_ge, sub_pubkey); /* commit to sub-key */ - if (!secp256k1_eckey_pubkey_serialize(&subkey_ge, c, &size, SECP256K1_EC_COMPRESSED)) { + if (!secp256k1_eckey_pubkey_serialize(&subkey_ge, c, &size, 1)) { return 0; } secp256k1_sha256_write(&sha, c, size); @@ -105,12 +105,12 @@ static int secp256k1_whitelist_compute_keys_and_message(const secp256k1_context* /* commit to fixed keys */ secp256k1_pubkey_load(ctx, &offline_ge, &offline_pubkeys[i]); - if (!secp256k1_eckey_pubkey_serialize(&offline_ge, c, &size, SECP256K1_EC_COMPRESSED)) { + if (!secp256k1_eckey_pubkey_serialize(&offline_ge, c, &size, 1)) { return 0; } secp256k1_sha256_write(&sha, c, size); secp256k1_pubkey_load(ctx, &online_ge, &online_pubkeys[i]); - if (!secp256k1_eckey_pubkey_serialize(&online_ge, c, &size, SECP256K1_EC_COMPRESSED)) { + if (!secp256k1_eckey_pubkey_serialize(&online_ge, c, &size, 1)) { return 0; } secp256k1_sha256_write(&sha, c, size); diff --git a/src/secp256k1.c b/src/secp256k1.c index 4c5782693..97e3145a7 100644 --- a/src/secp256k1.c +++ b/src/secp256k1.c @@ -300,7 +300,7 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o ARG_CHECK(pubkey != NULL); ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION); if (secp256k1_pubkey_load(ctx, &Q, pubkey)) { - ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, flags & SECP256K1_FLAGS_BIT_COMPRESSION); + ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, !!(flags & SECP256K1_FLAGS_BIT_COMPRESSION)); if (ret) { *outputlen = len; }