Chris P.'s review: Protocol

[cjpatton]

Some comments on the spec itself. These are all suggestions for clarification, none of them are breaking changes.

2.1

• Quote:

  Such groups are uniquely determined by the choice of the prime p that defines the order of the group.

  This statement needs to be sharpened a bit. In particular, the order of the group doesn't determine the encoding of group elements or the group action. For example, it's possible to have multiple representations of the same elliptic curve group, something that's exploited in the ed25519 paper.

2.2.1

• In ComputeCompositesFast:
  • String contextString is undefined. I don't think it's clear from context where this comes from. This is only made clear later on in the next section.
  • The notiation for for-loops is for i = 0 to m-1. Is the range inclusive or exclusive? To make this unambiguous, I woiuld write this in 100% Python, e.g.: for i in range(m) for i from 0 to m (exclusive). (Here and in the remainder.)

3.2

• In the definition of SetupOPRFServer(), the function OPRFServerContext() is used but not defineed. However, I also noticed that SetupOPRFServer() is never actually used. Is it appropriate to just remove it? (Similarly below.)

3.3

• Quote:

  Applications serialize protocol messages between client and server for transmission. Specifically, values of type Element are serialized to SerializedElement values, and values of type Proof are serialized as the concatenation of two SerializedScalar values. Deserializing these values can fail, in which case the application MUST abort the protocol with a DeserializeError failure.

  This statement seems to imply that serialization is out-of-scope for this document, but in Section 1.3 there is a paragraph that says all data structures are represented in TLS-syntax. If the draft does intend to specify the encoding of messages exchanged in the online protocol, then it should do so unambiguously.

  For example, in the definition of Evaluate() for VOPRF (Section 3.3.2), the return value is the pair evaluatedElement, proof. Then the text says "The server sends both evaulatedElement and proof back to the client." The text quoted above says how to encode these individually, but I'd argue it's not clear what order they should be sent in.

  Finally, TLS-syntax is not really used in this document.

3.3.1

• nit: The text makes clear where skS comes from, but I find it a bit jarring that this isn't passed to Evaluate() as an explicit parameter. (Similarly below, and similarly for pkS.)

• The expression blind^(-1) isn't explicitly defined anywhere. To someone with some basic algebra background, this is clearly the multiplicative inverse of blind modulo p (the order of the group); but based on Section 2, my impression is that we're not assuming the reader has this background.

3.3.3

• nit: context is similar to contextString, which is a bit confusing. Consider giving it a different name, like framed_info or something.

4.3

• I'm curious why you went for SHA-512 for Ristretto255 but are using SHA-256 for P-256.

6.5

• This section specifies deserialization and input validation for each ciphersuite. As such I'm not sure it belongs in security considerations. I'd lift this text to Section 4.

6.6

• Similarly, this section specifies hash-to-curve for each of ciphersuite and ought to be lifted to Setion 4.