Deposit Signature Subgroup Check

[kirk-baird]

Description

The deposit signature needs to be verified individually and check both the signature subgroup and the public key subgroup.

Version

master

Present Behaviour

PublicKey subgroups are not checked in the function fast_aggregate_verify_pre_aggregated() which is the function currently called by verify_deposit_signature().

Expected Behaviour

PublicKey subgroups should be checked, this is done in Signature::Verify().

Steps to resolve

Update the function verify_deposit_signature() in consensus/state_processing/src/per_block_processing/verify_deposit.rs such that it calls
Signature::verify() in crypto/bls/src/signature.rs.

[paulhauner]

Hey @kirk-baird is this still relevant? :)

[kirk-baird]

Hey @kirk-baird is this still relevant? :)

Yep this is still relevant, instead of using a signature set for the deposit it would be safer to verify the signature directly.

i.e. We currently have

    let deposit_signature_message = deposit_pubkey_signature_message(&deposit_data, spec)
        .ok_or_else(|| error(DepositInvalid::BadBlsBytes))?;

    verify!(
        deposit_signature_set(&deposit_signature_message).verify(),
        DepositInvalid::BadSignature
    );

Where deposit_signature_message is (PublicKey, SIgnature, Message)
The simplest solution is

    let (pubkey, signature, message) = deposit_pubkey_signature_message(&deposit_data, spec)
        .ok_or_else(|| error(DepositInvalid::BadBlsBytes))?;

    verify!(
        signature.verify(&pubkey, message).verify(),
        DepositInvalid::BadSignature
    );

This will mean we are no longer parallelising deposit signatures though which would be a little slower.

The other option is what is being discussed with the blst guys and I'll ping Andy about our current status. It is to do the subgroup checks when we first deserialise a point then we don't need to worry cause all public keys and signatures will have their subgroup checked by this point.

[paulhauner]

I believe @blacktemplar is working on this now :)

[blacktemplar]

I started looking into this and I am not sure why @kirk-baird's proposed simple solution should solve the problem.

I am currently looking into the BLST implementations and there we have the following call stack:
signature.verify(&pubkey, message)
-> signature.point.unwrap().verify(pubkey.point(), message)
-> signature.point.unwrap().verify(message.as_bytes(), DST, &[], pubkey.point())
-> signature.point.unwrap().aggregate_verify(&[message.as_bytes().as_slice()], DST, &[pubkey.point()])

Besides some infinity checks nothing else happens in those calls.

Following the call stack on the currently used method deposit_signature_set(&deposit_signature_message).verify() shows that it will also use the aggregate_verify function, so without going into detail of that function, either both approaches do subgroup checks or none of them do it. Maybe also relevant this comment: https://github.com/supranational/blst/blob/ce25c7202dacab8fa64a939f05f6efa232b25ea8/bindings/rust/src/lib.rs#L601.

@kirk-baird do you have any updates about the second solution that checks subgroups on serialization?

[kirk-baird]

Yep this is an open issue on BLST. It depends where we end up doing the subgroup check.

That solution will CURRENTLY works for milagro_bls but I will update milagro_bls to match blst after they've decided where subgroup checks will be done!

Basically it comes down to a potential optimisation in the BLS libraries using Proof Of Possession that after we've verified ownership of the public key we do not need to check the subgroup again (as we've already done it).

Deposit (signature, public key) pairs are where we verify a user has knowledge of the secret key for that public key so we need to enforce public key subgroups are checked.

On the other hand these public keys subgroups do not need to be checked for fast_aggregate_verify() or batch_fast_aggregate_verify() as we assume that the public key is already verified and so the implementation may optimise them out, we just need to confirm whether BLST will optimise these out or not.

[paulhauner]

Pushing this back to A1 since we're blocked on BLST and it's not going to happen before the audit.

[paulhauner]

Hey @kirk-baird wondering if this is still relevant 😆

[kirk-baird]

Yep although we technically now do a subgroup check, we are using an aggregate verify function which we aren't allowed to use on unverified PublicKeys. It would be better practice and safer to avoid SignatureSets i.e.

pub fn verify_deposit_signature(deposit_data: &DepositData, spec: &ChainSpec) -> Result<()> {
    let (public_key, signature, msg) = deposit_pubkey_signature_message(&deposit_data, spec)
        .ok_or_else(|| error(DepositInvalid::BadBlsBytes))?;

    verify!(
        signature.verify(&public_key, msg)
        DepositInvalid::BadSignature
    );

    Ok(())
}