boltlabs-inc / libzkchannels-crypto

Zero knowledge signatures and more
MIT License
4 stars 0 forks source link

Update challenge generation formatting to match proposed standard #62

Open marsella opened 3 years ago

marsella commented 3 years ago

The current implementation is not in line with https://datatracker.ietf.org/doc/rfc8235/. This specification standardizes Schnorr NIZKs.

In particular, we are not following the recommended practice during Fiat-Shamir challenge generation. We should clearly delineate the boundary between concatenated items fed to the hash function, and prepend each item with a 4-byte integer that represents the length of that item, as specified in RFC 8235 section 2.3.

marsella commented 3 years ago

Per discussion with @gijsvl: We should check whether other zk proof libraries actually use this formatting (Merlin? others?). RFCs are not standards, so it worth checking whether this is the de facto standard or if others have converged around some other formatting.

marsella commented 3 years ago

Check what the underlying default operation is for the hasher type we're using. They might already do something like this.

marsella commented 3 years ago

Per discussion with @indomitableSwan: Although it's not obvious that adding boundary-checking / lengths to the hash is actively preventing an attack, there's no reason why we wouldn't. In general, this is good practice when concatenating any data, and there are some cases where it can prevent protocol-level attacks. We can check to see if others are doing it for reference but we will add it either way.

indomitableSwan commented 2 years ago

Looking at the implementation of Merlin (https://merlin.cool/problem.html), they go even further than the approach suggested in the RFC in generating transcripts used for challenge generation. In particular, they use a domain separator as well, which makes a lot of sense to me.

marsella commented 2 years ago

Shrinking the scope of this issue: the standard also makes recommendations about using transcripts, etc. This is currently addressed in boltlabs-inc/zeekoe#27, and we will add issues under that epic if the implementation will include changes to this repo.