Consensys / teku

Open-source Ethereum consensus client written in Java
https://consensys.io/teku
Apache License 2.0
650 stars 265 forks source link

Public key aggregation ambiguous infinite points handling #4111

Closed veorq closed 2 years ago

veorq commented 3 years ago

BlstPublicKey.aggregate() will return infinitePublicKey in two cases:

  1. If one of the public keys to aggregate is invalid (not in the subgroup or infinity point)
  2. If the sum of (valid) public keys happens to be zero

https://github.com/ConsenSys/teku/blob/a0270026d4e70db13b0042eb057ffbdb5709e4ca/bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstPublicKey.java#L56-L74

The two cases should be distinguished, so that the caller can handle them differently.

benjaminion commented 3 years ago

Thank you for raising this.

While the distinction between the two cases is true, for all practical purposes (in Teku, at least) it makes no difference. In the astronomically unlikely event that an aggregate public key results in the point at infinity, it would be invalid for signature verification. This is because the standard's CoreVerify function (arguably) requires the secret key not to be zero. This is maybe an issue with the standard, but that's how it is.

In other applications, getting an infinite public key from aggregation likely suggests a rogue public key attack or similar, and granted that it might be useful to know about this.

I'll leave this open for a while for discussion, but we probably won't be making changes here unless/until we are doing some further work. However, if this is a blocker to another project, we can modify to return an optional, or throw an exception if invalid individual keys are present. Lmk.

veorq commented 3 years ago

Thanks @benjaminion for the quick and clear response. I agree with your evaluation of the risk.

I was less worried about the zero sum than about a single invalid key not being detected, as you suggest what about throwing an exception, as BlstSignature.aggregate() is doing in

https://github.com/ConsenSys/teku/blob/a0270026d4e70db13b0042eb057ffbdb5709e4ca/bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstSignature.java#L71-L78

?