search

ethereum / py_ecc

Python implementation of ECC pairing and bn_128 and bls12_381 curve operations
MIT License
191 stars 82 forks source link

Dangerous lack of subgroup check for G2 groups in bls #126

Open Rumata888 opened 2 years ago

Rumata888 commented 2 years ago

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 commented 2 years ago

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

Rumata888 commented 2 years ago

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 commented 2 years ago

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.