Dangerous lack of subgroup check for G2 groups in bls

[Rumata888]

What is wrong?

G2 point decompression function goes through all the regular checks same as for G1 (checks that coordinates are in field and that the point is on curve). However, there is no subgroup check, which presents a security vulnerability, especially if someone tries to use this code for distributed key generation (then you can mount the baby sharks (https://medium.com/zengo/baby-sharks-a3b9ceb4efe0) attack).

How can it be fixed

Add subgroup checks when decompressing G2 points

[carver]

This is not my specialty. @ChihChengLiang could you take a look?

[Rumata888]

For some reason the link I added to the post to the function got lost. https://github.com/ethereum/py_ecc/blob/a1d18addb439d7659a9cbac861bf1518371f0afd/py_ecc/bls/point_compression.py#L173

[ChihChengLiang]

Hi @Rumata888 and @carver,

We moved the subgroup check outside of the decompress_G2 after the refactor of #116 (cc @hwwhww).

1. For public key (G1), we perform the subgroup check in KeyValidate. We check all core APIs with KeyValidate except for _AggregatePKs. https://github.com/ethereum/py_ecc/blob/033b4ea69a3cbc841c02c9b7d1b4996cd7863585/py_ecc/bls/ciphersuites.py#L115
2. For Signature (G2), the subgroup check is added to all APIs except for Aggregate. https://github.com/ethereum/py_ecc/blob/033b4ea69a3cbc841c02c9b7d1b4996cd7863585/py_ecc/bls/ciphersuites.py#L152

Not sure if AggregateVerify is dangerous if we do the subgroup check for the aggregated instance instead of each instance. The latter is strictly safer. Asking @kevaundray for a second opinion.