Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` #1642

pull theStack wants to merge 1 commits into bitcoin-core:master from theStack:202412-verify-compressed-argument-in-_eckey_pubkey_serialize changing 2 files +4 −1

theStack commented at 5:28 PM on December 2, 2024: contributor

Due to similarity to the public API function secp256k1_ec_pubkey_serialize, public API flags like SECP256K1_EC_COMPRESSED are sometimes mistakingly passed to secp256k1_eckey_pubkey_serialize in newly proposed code (this is currently the case for several modules in secp256k1-zkp, see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/300), which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that the compressed argument is either 0 or 1.

Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize`

Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`,
public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly
passed to newly proposed code (this is currently the case for several modules in
secp256k1-zkp, see https://github.com/BlockstreamResearch/secp256k1-zkp/pull/300).
which is currently not detected. To avoid this in the future, a VERIFY_CHECK
is added to check that the `compressed` argument is either 0 or 1.

1823594761

real-or-random added the label assurance on Dec 2, 2024
real-or-random added the label refactor/smell on Dec 2, 2024

in src/eckey_impl.h:39 in 1823594761

  35 | @@ -35,6 +36,8 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char
  36 |  }
  37 |  
  38 |  static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed) {
  39 | +    VERIFY_CHECK(compressed == 0 || compressed == 1);

real-or-random commented at 8:30 AM on December 9, 2024:

In other places, we simply rely on C's int-as-bool convention, so I suggest adding a comment that explains why we enforce 0/1 in this case.

real-or-random approved
real-or-random commented at 8:30 AM on December 9, 2024: contributor

utACK 1823594761046ae38e002781d65e9538dd211d27
stratospher commented at 8:38 AM on February 24, 2025: contributor

ACK 1823594. Got tests failures when passing public API flags to secp256k1_eckey_pubkey_serialize.
real-or-random merged this on Feb 24, 2025
real-or-random closed this on Feb 24, 2025

Contributors

theStack

real-or-random

stratospher

Labels

assurance refactor/smell