Review of SECP256K1Wrapper

[mratsim]

Hey @kayabaNerve,

Here is my review of SECP256K1Wrapper.nim as of https://github.com/kayabaNerve/Ember/blob/f3da08adf7ef8d819416a9e87e1a4d07bfecd779/src/lib/SECP256K1Wrapper.nim#L17

General notes:

Make sure to check or Ethereum keys wrapper: https://github.com/status-im/nim-eth-keys/blob/master/eth_keys/libsecp256k1.nim

and also the old API that I wrote: https://github.com/status-im/nim-eth-keys/blob/master/old_api/backend_libsecp256k1/libsecp256k1.nim

(License MIT/Apache v2)

Note that both are untested yet and not audited.

secpPublicKey

https://github.com/kayabaNerve/Ember/blob/f3da08adf7ef8d819416a9e87e1a4d07bfecd779/src/lib/SECP256K1Wrapper.nim#L17-L21

Security

There should be a doAssert / check size of the secpPublicKey string.
Also Public Keys have several serialized representations depending if they start with a:

• 0x04 (uncompressed - 64 bytes)
• 0x02 or 0x03 (compressed - 33 bytes)
• 0x06 or 0x07 (hybrid - 65 bytes)

You should use secp256k1_ec_pubkey_parse instead:

• It will handle all those cases
• secp256k1 operations are constant time (for division for example) and will protect the wallet against timing attacks.
• The secp256k1_pubkey data structure is an array[64, byte] but what is inside is implementation defined. It can change depending on platform (x86, ARM), endianness and libsecp256k1 version.

https://github.com/status-im/secp256k1/blob/be6f5385330905bf1d7cc441be6703cfa7aef847/include/secp256k1.h#L281-L300

/** Parse a variable-length public key into the pubkey object.
 *
 *  Returns: 1 if the public key was fully valid.
 *           0 if the public key could not be parsed or is invalid.
 *  Args: ctx:      a secp256k1 context object.
 *  Out:  pubkey:   pointer to a pubkey object. If 1 is returned, it is set to a
 *                  parsed version of input. If not, its value is undefined.
 *  In:   input:    pointer to a serialized public key
 *        inputlen: length of the array pointed to by input
 *
 *  This function supports parsing compressed (33 bytes, header byte 0x02 or
 *  0x03), uncompressed (65 bytes, header byte 0x04), or hybrid (65 bytes, header
 *  byte 0x06 or 0x07) format public keys.
 */
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_parse(
    const secp256k1_context* ctx,
    secp256k1_pubkey* pubkey,
    const unsigned char *input,
    size_t inputlen
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

So in memory you work with the libsecp256k1 representation, but you use the serialized version for storage or communication.

Example - new Eth-key API:

proc recoverPublicKey*(data: openarray[byte],
                       pubkey: var PublicKey): EthKeysStatus =
  ## Unserialize public key from `data`.
  let ctx = getSecpContext()
  let length = len(data)
  if length < RawPublicKeySize:
    setErrorMsg(InvalidPublicKey)
    return(EthKeysStatus.Error)
  var rawkey: array[RawPublicKeySize + 1, byte]
  rawkey[0] = 0x04'u8 # mark key with UNCOMPRESSED flag
  copyMem(addr rawkey[1], unsafeAddr data[0], RawPublicKeySize)
  if secp256k1_ec_pubkey_parse(ctx, addr pubkey,
                               cast[ptr cuchar](addr rawkey),
                               RawPublicKeySize + 1) != 1:
    return(EthKeysStatus.Error)
  result = EthKeysStatus.Success

proc initPublicKey*(data: openarray[byte]): PublicKey =
  ## Create new public key from binary data blob.
  if recoverPublicKey(data, result) != EthKeysStatus.Success:
    raise newException(EthKeysException, InvalidPublicKey)

or old API

proc parsePublicKeyWithPrefix(data: openarray[byte], result: var PublicKey) =
  ## Parse a variable-length public key into the PublicKey object
  if secp256k1_ec_pubkey_parse(ctx, result.asPtrPubKey, cast[ptr cuchar](unsafeAddr data[0]), data.len.csize) != 1:
    raise newException(Exception, "Could not parse public key")

proc parsePublicKey*(data: openarray[byte]): PublicKey =
  ## Parse a variable-length public key into the PublicKey object
  case data.len
  of 65:
    parsePublicKeyWithPrefix(data, result)
  of 64:
    var tmpData: Serialized_PubKey
    copyMem(addr tmpData[1], unsafeAddr data[0], 64)
    tmpData[0] = 0x04
    parsePublicKeyWithPrefix(tmpData, result)
  else: # TODO: Support other lengths
    raise newException(Exception, "Wrong public key length")

Performance:

From a performance point of view you should change (uint8) parseHexInt(pubKey[i .. i + 1]) to (uint8) parseHexInt(pubKey.toOpenarray(i, i+1)) because slicing will create a new seq allocated on the heap while toOpenarray provides a view. Note that toopenarray requires devel. In any case this doesn't really matter because the string should be passed to libsecp256k1 anyway.

secpSignature

https://github.com/kayabaNerve/Ember/blob/f3da08adf7ef8d819416a9e87e1a4d07bfecd779/src/lib/SECP256K1Wrapper.nim#L23-L32

Security

Similar to public key you should use secp256k1_ecdsa_recoverable_signature_parse_compact like in the new API:

proc recoverSignature*(data: openarray[byte],
                       signature: var Signature): EthKeysStatus =
  ## Unserialize signature from `data`.
  let ctx = getSecpContext()
  let length = len(data)
  if length < RawSignatureSize:
    setErrorMsg(InvalidSignature)
    return(EthKeysStatus.Error)
  var recid = cint(data[KeyLength * 2])
  if secp256k1_ecdsa_recoverable_signature_parse_compact(ctx, addr signature,
                                           cast[ptr cuchar](unsafeAddr data[0]),
                                                         recid) != 1:
    return(EthKeysStatus.Error)
  result = EthKeysStatus.Success

proc initSignature*(hexstr: string): Signature =
  ## Create new signature from hexadecimal string representation.
  var o = fromHex(stripSpaces(hexstr))
  if recoverSignature(o, result) != EthKeysStatus.Success:
    raise newException(EthKeysException, libsecp256k1ErrorMsg())

or the old one

proc parseSignature*(data: openarray[byte], fromIdx: int = 0): Signature =
  ## Parse a compact ECDSA signature. Bytes [fromIdx .. fromIdx + 63] of `data`
  ## should contain the signature, byte [fromIdx + 64] should contain the recovery id.
  assert(data.len - fromIdx >= 65)
  if secp256k1_ecdsa_recoverable_signature_parse_compact(ctx,
      result.asPtrRecoverableSignature,
      cast[ptr cuchar](unsafeAddr data[fromIdx]),
      cint(data[fromIdx + 64])) != 1:
    raise newException(ValueError, "Signature data is invalid")

Note: when trying to do a pure Nim secp256k1 compatible lib, in my tests I couldn't understand the in-memory ECDSA signature representation so just use libsecp256k1 proc.

Serialization of Public key and Signature

A serialized public key or signature is what we use "openly".

You should use the corresponding secp256k1_ec_pubkey_serialize and secp256k1_ecdsa_recoverable_signature_serialize_compact (for a recoverable 65 bytes signature) or secp256k1_ec_pubkey_serialize (for a 64 bytes signature). We use the recoverable signature for Ethereum.

New API

proc toRaw*(pubkey: PublicKey, data: var openarray[byte]) =
  ## Converts public key `pubkey` to serialized form and store it in `data`.
  var key: array[RawPublicKeySize + 1, byte]
  assert(len(data) >= RawPublicKeySize)
  var length = csize(sizeof(key))
  let ctx = getSecpContext()
  if secp256k1_ec_pubkey_serialize(ctx, cast[ptr cuchar](addr key),
                                   addr length, unsafeAddr pubkey,
                                   SECP256K1_EC_UNCOMPRESSED) != 1:
    raiseSecp256k1Error()
  assert(length == RawPublicKeySize + 1)
  assert(key[0] == 0x04'u8)
  copyMem(addr data[0], addr key[1], RawPublicKeySize)

proc getRaw*(pubkey: PublicKey): array[RawPublicKeySize, byte] {.noinit, inline.} =
  ## Converts public key `pubkey` to serialized form.
  toRaw(pubkey, result)

proc toRaw*(s: Signature, data: var openarray[byte]) =
  ## Converts signature `s` to serialized form and store it in `data`.
  let ctx = getSecpContext()
  var recid = cint(0)
  assert(len(data) >= RawSignatureSize)
  if secp256k1_ecdsa_recoverable_signature_serialize_compact(
    ctx, cast[ptr cuchar](addr data[0]), addr recid, unsafeAddr s) != 1:
    raiseSecp256k1Error()
  data[64] = uint8(recid)

proc getRaw*(s: Signature): array[RawSignatureSize, byte] {.noinit, inline.} =
  ## Converts signature `s` to serialized form.
  toRaw(s, result)

Old API Signature and Public Key

proc serialize*(s: Signature, output: var openarray[byte], fromIdx: int = 0) =
  ## Serialize an ECDSA signature in compact format, 65 bytes long
  ## (64 bytes + recovery id). The output is written starting from `fromIdx`.
  assert(output.len - fromIdx >= 65)
  var v: cint
  discard secp256k1_ecdsa_recoverable_signature_serialize_compact(ctx,
    cast[ptr cuchar](addr output[fromIdx]), addr v, s.asPtrRecoverableSignature)
  output[fromIdx + 64] = byte(v)

proc serialize*(key: PublicKey, output: var openarray[byte], addPrefix = false) =
  ## Exports a publicKey to `output` buffer so that it can be
  var
    tmp{.noInit.}: Serialized_PubKey
    tmp_len: csize = 65

  # Proc always return 1
  discard secp256k1_ec_pubkey_serialize(
    ctx,
    tmp.asPtrCuchar,
    addr tmp_len,
    key.asPtrPubKey,
    SECP256K1_EC_UNCOMPRESSED
  )

  assert tmp_len == 65 # header 0x04 (uncompressed) + 128 hex char
  if addPrefix:
    assert(output.len >= 65)
    copyMem(addr output[0], addr tmp[0], 65)
  else:
    assert(output.len >= 64)
    copyMem(addr output[0], addr tmp[1], 64) # Skip the 0x04 prefix

proc toString*(key: PublicKey): string =
  var data: array[64, byte]
  key.serialize(data)
  result = data.toHex

proc toStringWithPrefix*(key: PublicKey): string =
  var data: array[65, byte]
  key.serialize(data, true)
  result = data.toHex

Signing and verify

https://github.com/kayabaNerve/Ember/blob/f3da08adf7ef8d819416a9e87e1a4d07bfecd779/src/lib/SECP256K1Wrapper.nim#L34-L55

You should not convert to cstring for perf, you should do cast[ptr cuchar](hash[0].addr) or cast[ptr cuchar](hash[0].unsafeAddr) instead to avoid extra allocation, the hash does not need to be a var parameter

For verification, alternatively with recoverable signatures you can use secp256k1_ecdsa_recoverable_signature_parse_compact to retrieve the signature the message was signed with like here.

Test vectors

Please refer to nim-eth-keys tests and the eth-keys tests from the Ethereum Foundation repo

Tests in the tests.nim file https://github.com/status-im/nim-eth-keys/blob/master/tests/tests.nim are against the new API, all the others are for the old API (which resembles more the Ethereum Foundation tests).

[kayabaNerve]

Solved with ddea5ac.

Thank you for all your help, from the analysis to the examples.
https://etherscan.io/tx/0x5d74407255062236ae6e72510376ad13098b8bd0b6991d4807837c89609de454