search

ZenGo-X / class

Rust library for building IQC: cryptography based on class groups of imaginary quadratic orders
GNU General Public License v3.0
63 stars 31 forks source link

Extract cldl with public setup, improve API and fix security bugs #34

Closed LLFourn closed 4 years ago

LLFourn commented 4 years ago

The motivation for moving it to its own file was that I couldn't get the lcm trick tests to run (PARI errors) and didn't want to have to replicate API changes for the lcm version when I couldn't test them. Here are the major API changes:

  1. The class group definition and verification of the public setup are separated from the public key and verification of proofs. In our setting we assume this had been done beforehand. Now you create a new ClassGroup with a setup seed.

  2. There is no more q inside the class group definition. Previous code was very inconsistent about this. Sometimes it used FE::q() and somtimes it would use the one inside the public key. Now we only use FE::q.

Security bugs:

  1. Fiat-Shamir challenge space was only 128. Now it's 2 ** 128 as intended.
  2. Fiat-Shamir challenge now includes the statement

Thanks for this code and thanks in advance for considering this big PR :). We were doing PoC concept work with it and these are the changes we had to make for our setting (see https://github.com/comit-network/a2l-poc). Please feel free to reject/modify the ideas if they are not to your liking. It would be nice if you were to integrate them so we don't have to maintain our own fork for the PoC to continue to work.

PS Sorry this is all in one commit. I did not intentionally change anything about the actual cryptographic algorithms (except for fixing the Fiat-Shamir challenge).

omershlo commented 4 years ago

My main issue here is rather minor and relates to Naming. two examples: 1) the struct ClassGroup is not a description of a class group 2) Original HSMCL struct contained public key and secret key as defined in figure 7 of https://eprint.iacr.org/2019/503.pdf. This is not maintained anymore in this PR.

Do you think you can change the structs to have more cryptographic meaning following the paper?

LLFourn commented 4 years ago

Thanks. On (1) I agree. I was unknowingly conflating two different things. I think it makes sense to call the group a CLGroup. I s/ClassGroup/CLGroup/ and made a more in depth description of the module: https://github.com/KZen-networks/class/pull/34/commits/369306082deced207863e674a7c974671c9049a4#diff-b8c77d3dd6d70a8a9abbda62822932f6R1-R16

On (2) I am following https://eprint.iacr.org/2020/084.pdf section 5.2:

If we assume a standardised set up process, which allowed to provide a description of Gb, of the subgroups F and Gq and of a random generator gq of Gq, one could completely omit the interactive set up phase for the CL encryption scheme and have all parties use the output of this standardised process. This significantly improves the IKeyGen protocol, as mentionned in Sec. 6.

Thus we separate the CL-group definition from the CL-encryption key-pair. Does this make sense? If you're saying you'd prefer it if there was a KeyPair struct which has the CL-encryption secret_key and public_key in the same struct I can certainly do that (actually that's what I had in an earlier version but the struct felt useless so I removed it).

omershlo commented 4 years ago

Hi, Can you keep an interface for encryption with predefined randomness? I see it was being removed, however it is needed for various applications (I use it in multi-party-edcsa)

LLFourn commented 4 years ago

@omershlo I added this back. Looking at it I think I can see why I removed it in favour of encrypt which just always returning the randomness used. This looks like it's what you need in multi-party-ecdsa: https://github.com/KZen-networks/multi-party-ecdsa/blob/master/src/protocols/two_party_ecdsa/cclst_2019/party_one.rs#L247-L253 (since you just pass in randomness you generated).

verifiably_encrypt is also there to do the whole thing in one go so you don't need to deal with the randomness at all.

omershlo commented 4 years ago

I think verifiably_encrypt is a nice addition. Should solve for multi-party-ecdsa.

I still think that you should expose SK,PK kind of structs to consumers. I intended for binaryQF to be internal to Class, however in this new version I must use it in the consuming library.

LLFourn commented 4 years ago

The types are now wrapped in PK and SK newtypes. Note that BinaryQF is still part of the public api since it's in the structs and the structs fields are marked public but I'll leave fixing that as a future improvement.