`tbls::verify_aggregate` accepts invalid public keys that cancel

[iamquang95]

Problem

tbls::verify_aggregate validates only the sum of the supplied public keys, never the individual keys. This lets a caller pass keys that are not valid public keys — points outside the G1 subgroup, or the point at infinity — and have verification succeed, as long as the invalid contributions cancel in the sum.

The code (crates/crypto/src/tbls.rs:288, current main):

let pks: Vec<BlstPublicKey> = public_keys
    .iter()
    .map(|pk_bytes| {
        // from_bytes is an ENCODING check only: on-curve, correctly compressed.
        // It does NOT check subgroup membership and does NOT reject infinity.
        BlstPublicKey::from_bytes(pk_bytes).map_err(|e| Error::InvalidPublicKey(e.into()))
    })
    .collect::<Result<Vec<_>, _>>()?;

let agg_pk = math::aggregate_public_keys(&pks)?;      // sums the points
let result = sig.verify(true, data, ETH2_DST, &[], &agg_pk, true);
//                      ^^^^ pk_validate = true — but only the SUM is validated here

The subgroup check happens once, on agg_pk. Subgroup membership survives addition, but the converse does not hold: a valid sum tells you nothing about the summands. BLS12-381's curve group has a cofactor, so points outside the order-r subgroup G1 exist and encode legally. For any such point Q, both Q and -Q pass from_bytes, and

sum([pk, Q, -Q]) = pk + Q - Q = pk

The sum is a valid G1 point, so verification returns Ok(()) — reporting that a signature was produced by a three-key set when it was produced by pk alone. The point at infinity is the degenerate case: a legal encoding that contributes nothing to the sum, so [pk, INFINITY] also passes.

Impact

verify_aggregate's job is to attest which set of keys produced a signature. A caller trusting an Ok result believes participants exist who do not. The two production call sites both take the key list from data crossing a trust boundary:

• crates/cluster/src/lock.rs:313 — Lock::verify, keys read from the cluster lock file's distributed_validators[].pub_shares, parsed with a bare length check.
• crates/dkg/src/signing.rs:367 — aggregate lock-hash verification during DKG.

This is a validation/soundness bug in middleware, not a key-extraction issue, but it undermines the guarantee the function exists to provide.

How to reproduce

Drop this into mod tests in crates/crypto/src/tbls.rs and run cargo test -p pluto-crypto --lib verify_aggregate_accepts_cancelling_keys. It passes on current main, which is the bug — verification returns Ok(()) for key sets it should reject.

#[test]
fn verify_aggregate_accepts_cancelling_keys() {
    // A point on E(Fp) with x = 4: on the curve, but OUTSIDE the G1 subgroup.
    // 0x80 = compression flag, sign bit clear.
    let mut q = [0u8; PUBLIC_KEY_LENGTH];
    q[0] = 0x80;
    q[PUBLIC_KEY_LENGTH - 1] = 4;

    // Its negation: same x, opposite y. 0xa0 = compression flag + sign bit.
    let mut neg_q = q;
    neg_q[0] = 0xa0;

    // The compressed G1 point at infinity: 0xc0 = compression + infinity bits.
    let mut inf = [0u8; PUBLIC_KEY_LENGTH];
    inf[0] = 0xc0;

    let data = b"hello obol!";
    let sk = generate_secret_key(rand::rngs::OsRng).unwrap();
    let pk = secret_to_public_key(&sk).unwrap();
    let sig = sign(&sk, data).unwrap();

    // BUG: all of these return Ok(()). Q and -Q are not valid public keys;
    // the infinity point is not a valid public key. They should be rejected.
    assert!(verify_aggregate(&[pk, q, neg_q], sig, data).is_ok());
    assert!(verify_aggregate(&[q, neg_q, pk], sig, data).is_ok());
    assert!(verify_aggregate(&[pk, inf], sig, data).is_ok());

    // Sanity: q alone (uncancelled) IS rejected today — but as a
    // VerificationFailed, because blst validates the sum inside verify(),
    // not as a public-key error where it belongs.
    assert!(verify_aggregate(&[q], sig, data).is_err());
}

Expected vs actual

input	expected	actual on main
[pk, Q, -Q], Q off-subgroup	Err (invalid key)	Ok(())
[pk, INFINITY]	Err (invalid key)	Ok(())
[Q] alone	Err(InvalidPublicKey)	Err(VerificationFailed) (wrong error)

This diverges from Charon, which deserializes each share individually in tbls/herumi.go (VerifyAggregate) and reports a key failure distinctly from a verification failure, and from the IETF BLS FastAggregateVerify precondition, which requires KeyValidate to have succeeded for every input key.