w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
310 stars 93 forks source link

[PSA] Merlin transcripts should have distinct non-prefix labels. #39

Closed WildCryptoFox closed 5 years ago

WildCryptoFox commented 5 years ago

In vrf.rs the transcript label h is a prefix of h^r and h^sk. The lengths of labels are not included in the hash, allowing for distinct transcripts to converge with malicious inputs. However, this particular use is fine as (a) the protocol is linear without branches and (b) the inputs are constant size - with their length included in the input; I have not done a pass over the remaining transcripts to verify they are immune to this subtle detail.

Simple fix: reserve a character for termination. \x00 or . perhaps. IMO this is a bug in merlin, but when I brought up the issue in #merlin on dalek-cryptography on slack they updated their documentation, defining a transcript protocol; emphasis mine.

This consists of two parts:

Defining a proof-specific domain separator and a set of labels for the messages specific to the protocol. The labels should be fixed, not defined at runtime, as runtime data should be in the message body, while the labels are part of the protocol definition. A sufficient condition for the transcript to be parseable is that the labels should be distinct and none should be a prefix of any other.

Defining how mathematical objects are encoded as bytes: for instance, how to encode a curve point as a byte string, or how to convert uniformly distributed challenge bytes to a uniformly-distributed scalar.

t.proto_name(b"DLEQProof");
t.commit_point(b"h", p.input.as_compressed());

t.commit_point(b"R=g^r", &self.R);
t.commit_point(b"h^r", &self.Hr);

t.commit_point(b"pk", public.as_compressed());
t.commit_point(b"h^sk", p.output.as_compressed());
burdges commented 5 years ago

I doubt this matters so much for schnorrkel, but we've just pushed a breaking change, so addressing any worries here makes sense. Thanks!

I added labels for the challenges and witnesses too. I'll go ahead and publish 0.3 but let me know if you notice anything else worth fixing around the hashing or whatever. :)

WildCryptoFox commented 5 years ago

@burdges Looks good, but you forgot the null terminators on the new witness and challenge labels. I doubt as well, but better safe than sorry!

burdges commented 5 years ago

Meh, that really does not impact anything, but for consistency.

I've also yanked all versions prior to 0.4 except those being used by parity in their test net.. and those will be yanked once parity updates.