Missing pubkey and signature validation for blsVerify()?

[veorq]

Maybe we missed something here:

The verify() used by 'blsVerify()' functions in https://github.com/status-im/nimbus-eth2/blob/stable/beacon_chain/spec/crypto.nim ultimately call coreVerifyNoGroupCheck() from nim-blscurve, adding the warning:

  ## The PublicKey MUST be directly created via
  ## `publicFromPrivate`
  ## or deserialized from `fromBytes` or `fromHex`.
  ## This ensures the precondition that it's not a zero key
  ## and that is has been subgroup checked

https://github.com/status-im/nim-blscurve/blob/fd4956f5d65129e9b475e654903a84303395eb92/blscurve/bls_sig_min_pubkey.nim#L120-L125

However, publicFromPrivate() does not do subgroup check (nor correct infinity point check, as noted in #2693), and the signature does not seem to be checked at all (as required in https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature-04#section-2.7).

[mratsim]

The new document in https://github.com/status-im/nim-blscurve/blob/e9b75abc/docs/bls_types_guarantees.md addresses those concerns.

All paths that create a public key (from a SecretKey type or from deserialization) would create one that is either valid by construction (via scalar multiplication by a 0 < SK < r secret key) or checked when deserializing.

As suggested in cfrg/draft-irtf-cfrg-bls-signature#33 (comment)

At the least, it seems like we should discuss this in Security Considerations, right? And probably it would also make sense to strongly recommend just doing subgroup checks as part of deserialization.

Is there something else we should be recommending in addition or instead?

and also for performance reason, we "cache the subgroup checks and infinity point checks" by only doing them at deserialization instead of redoing them for each single or aggregate verifications, this is mentioned in coreVerifyNoGroupCheck https://github.com/status-im/nim-blscurve/blob/e9b75abc/blscurve/blst/blst_min_pubkey_sig_core.nim#L257-L265

func coreVerifyNoGroupCheck*[T: byte|char](
       publicKey: PublicKey,
       message: openarray[T],
       sig_or_proof: Signature or ProofOfPossession,
       domainSepTag: static string): bool {.noinline.} =
  ## Check that a signature (or proof-of-possession) is valid
  ## for a message (or serialized publickey) under the provided public key
  ## This assumes that the Public Key and Signatures
  ## have been pre group checked (likely on deserialization)

The future draft of the BLS signature spec should make it clearer that KeyValidate can happen either at deserialization or within CoreVerify and that at deserialization is preferred (according to the current consensus).

[veorq]

Makes sense, thanks. I did not find where the deserial checks are done though (on keys and sigs).

[mratsim]

In the fromBytes proc in nim-blscurve:

https://github.com/status-im/nim-blscurve/blob/e9b75abc/blscurve/blst/bls_sig_io.nim#L42-L99

func fromBytes*(
       obj: var (Signature|ProofOfPossession),
       raw: array[96, byte] or array[192, byte]
      ): bool {.inline.} =
  ## Initialize a BLS signature scheme object from
  ## its raw bytes representation.
  ## Returns true on success and false otherwise
  result =
    when raw.len == 96:
      obj.point.blst_p2_uncompress(raw) == BLST_SUCCESS
    elif raw.len == 192:
      obj.point.blst_p2_deserialize(raw) == BLST_SUCCESS
    else: false

  # Infinity signatures are allowed if we receive an empty aggregated signature
  if result:
    result = bool obj.point.blst_p2_affine_in_g2()

func fromBytesKnownOnCurve*(
       obj: var (Signature|ProofOfPossession),
       raw: array[96, byte] or array[192, byte]
      ): bool {.inline.} =
  ## Initialize a BLS signature scheme object from
  ## its raw bytes representation.
  ## Returns true on success and false otherwise
  ##
  ## The point is known to be on curve and is not group checked
  result =
    when raw.len == 96:
      obj.point.blst_p2_uncompress(raw) == BLST_SUCCESS
    elif raw.len == 192:
      obj.point.blst_p2_deserialize(raw) == BLST_SUCCESS
    else: false
  # Infinity signatures are allowed if we receive an empty aggregated signature

  # Skipped - Known on curve
  # if result:
  #   result = bool obj.point.blst_p2_affine_in_g2()

func fromBytes*(
       obj: var PublicKey,
       raw: array[48, byte] or array[96, byte]
      ): bool {.inline.} =
  ## Initialize a BLS signature scheme object from
  ## its raw bytes representation.
  ## Returns true on success and false otherwise
  result =
    when raw.len == 48:
      obj.point.blst_p1_uncompress(raw) == BLST_SUCCESS
    elif raw.len == 96:
      obj.point.blst_p1_deserialize(raw) == BLST_SUCCESS
    else: false

  # Infinity public keys are not allowed
  if result:
    result = not bool obj.point.blst_p1_affine_is_inf()
  if result:
    result = bool obj.point.blst_p1_affine_in_g1()

And they are called in Nimbus by the load functions

nimbus-eth2/beacon_chain/spec/crypto.nim

Lines 107 to 157 in 44f652f

	proc load*(v: ValidatorPubKey): Option[CookedPubKey] =
	## Parse signature blob - this may fail
	var val: blscurve.PublicKey
	if fromBytes(val, v.blob):
	some CookedPubKey(val)
	else:
	none CookedPubKey
	proc load*(v: UncompressedPubKey): Option[CookedPubKey] =
	## Parse signature blob - this may fail
	var val: blscurve.PublicKey
	if fromBytes(val, v.blob):
	some CookedPubKey(val)
	else:
	none CookedPubKey
	func loadValid*(v: UncompressedPubKey | ValidatorPubKey): CookedPubKey {.noinit.} =
	## Parse known-to-be-valid key - this is the case for any key that's passed
	## parsing once and is the output of serialization, such as those keys we
	## keep in the database or state.
	var val: blscurve.PublicKey
	let ok = fromBytesKnownOnCurve(val, v.blob)
	doAssert ok, "Valid key no longer parses, data corrupt? " & $v
	CookedPubKey(val)
	proc loadWithCache*(v: ValidatorPubKey): Option[CookedPubKey] =
	## Parse public key blob - this may fail - this function uses a cache to
	## avoid the expensive deserialization - for now, external public keys only
	## come from deposits in blocks - when more sources are added, the memory
	## usage of the cache should be considered
	var cache {.threadvar.}: Table[typeof(v.blob), CookedPubKey]
	# Try to get parse value from cache - if it's not in there, try to parse it -
	# if that's not possible, it's broken
	cache.withValue(v.blob, key) do:
	return some key[]
	do:
	# Only valid keys are cached
	let cooked = v.load()
	if cooked.isSome():
	cache[v.blob] = cooked.get()
	return cooked
	proc load*(v: ValidatorSig): Option[CookedSig] =
	## Parse signature blob - this may fail
	var parsed: blscurve.Signature
	if fromBytes(parsed, v.blob):
	some(CookedSig(parsed))
	else:
	none(CookedSig)