subgroup checks on ecpoint's upon construction

[sidhujag]

This work attempts to fix https://github.com/dashpay/dash/issues/3898 by checking ecpoint's fall within subgroups. Thanks to @omershlo for the discovery of the vulnerability. We can check for the validity upon construction of PublicKey or PrivateKey from external data, as the data buffer is held privately to the class we don't need to worry about checking on the copy constructors or copy members as it is assumed the object's being copied would have been checked as well. However note that due to not fully analyzing the calling logic in every code path I simply initialized the buffer how the default constructor does to be specific:

PublicKey::PublicKey() {
    g1_set_infty(q);
}

Is the default constructor for PublicKey and if upon construction from external data it is invalid (PublicKey::FromBytes, PublicKey::FromG1):

if(!pk.CheckValid()) {
    g1_set_infty(pk.q);
}

I set it to the default state. I do not know if the consuming application logic has issues with this or will detect this key to be invalid when doing dkg or aggregated signatures but i assume it resolves because the key is essentially invalid.

PrivateKey:

void PrivateKey::AllocateKeyData() {
    keydata = Util::SecAlloc<bn_t>(1);
    bn_new(*keydata);  // Freed in destructor
    bn_zero(*keydata);
}

and during creation from external data (PrivateKey::FromSeed, PrivateKey::FromBytes):

if(!k.CheckValid()){
    bn_zero(*k.keydata);
}

[sidhujag]

Thanks for the PR but PrivateKey::CheckValid() - checking that the actual private key in keydata is inside the G2 subgroup - doesn't make a lot sense. PrivateKey::keydata, the actual private key data, is a bignum pointer bn_t* and you are just casting it to a G2 element pointer g2_t* which represents a different data structure so thats pointless and not a good idea at all.

The whole thing about the subgroup checks is to make sure public keys and signatures are valid subgroup elements. In the new library version they currently only use the minimum-pubkey-size variant of the schemes which means public keys are elements on G1 (G1Element in the new lib version) and signatures are elements on G2 (G2Element in the new lib version).

Short: The code in PrivateKey::CheckValid which comes from G2Element::CheckValid should be a InsecureSignature::CheckValid instead and the checks should accordingly happen in InsecureSignature::FromBytes and InsecureSignature::FromG2.

Thanks, it should have been obvious but was late at night thanks for the catch! Anyways I moved the checks to InsecureSignature.

[xdustinface]

I guess this can be closed now because we are in the process of moving to the new (restructured) version of the library.