Re: [Cfrg] Comments on draft-irtf-cfrg-curves-11

[Adam Langley <[e-mail]>]

On Wed, Jan 13, 2016 at 4:36 AM, John Mattsson
<[e-mail]> wrote:
> I took some time during Christmas to review draft-irtf-cfrg-curves-11.
> Very good. I have some comments, mostly editorial:

Thank you for your detailed review! I /think/ I can still fix these
nits, although if the RFC Editor disagrees then I'm sorry.

> - Sec 1: The equation “y^2 = x^3 + A*x^2 + x” is used in Section 1 and A
> even though the notation in section 3 says that x, y are coordinates on
> Edwards curves. Shouldn’t this be “v^2 = u^3 + A*u^2 + u”?

Agreed.

> -Sec 3: The notation ‘P’ for the generator base point is not used except
> in ‘X(P)’ and ‘Y(P)’, I think you should either use the ‘P’ notation or
> drop it.
>
> - Sec 3: I do not see the point of the ‘X(P)’ and ‘Y(P)’ notations. It is
> only used twice. Why not just say that “The base point is x =
> 1511222134953540...., y = 46316835694926478169...”?

Since these numbers are so long it's useful to have them in the table.
In light of that, you might well ask why, for the Montgomery form, I
put them in prose. I wondered that too and so moved the Montgomery
form into the tables with the notation U(P) and V(P).

> - Sec 4: You should point out that the ‘order’ is the order of the
> prime-order subgroup and not the order of E(F_p).

Done.

> It would also be good to
> point out that this subgroup has prime order. In appendix A, the notation
> ‘r’ is used for the order of the prime-order subgroup.

Good point. Changed to "order".

> Furthermore in the
> findCurve function, ‘order’ means #E(F_p) and in the findBasepoint
> function ‘order’ means point order. Would be good with some clearer
> notation for the different orders.

Renamed these groupOrder and pointOrder, respectively.

> - Sec 4.1, 4.2, I think it would be good with some indention or
> subsubsections to make the separation of e.g. curve25519 and edwards25519
> clearer.

I would like to be able to put a floating section heading in here, but
the XML doesn't really allow it. I can split it into two sections, but
I tried that and it didn't really work either. I've ended up leaving
this as it is.

> - Sec 5: “MUST mask the most-significant bit in the final byte”
> I think the ‘mask’ operation should be explained.

Changed to "ignore".

> - Sec 5: The heading capitalization is different in section 5, i.e. the
> first letters in “functions”, “considerations”, “vectors” are non-capital.

Thanks. IETF seems to want to capitalise each word so I've done that.

> - Sec 6.1: “key-derivation function that includes K, K_A and K_B to derive
> a key”
> Probably helpful for some readers to specify that the key is a “symmetric
> key”,

Done.

> - Sec 6.1: “the co-factor , h”
> The spelling “cofactor” is used in the rest of the document and the
> notation ‘h’ is not used anywhere else.

Done.

> - Sec 6.1: Calling Bob’s private key ‘g’ may confuse some readers as ‘g’
> is very often used to denote the generator in Diffie-Hellman protocols.
> I.e. here Bob computes 5^g and not g^5.

Fair point. I've renamed these values 'a' and 'b', which I think is
more typically for Alice and Bob anyway.

> - Sec 6.1: If implementors must calculate the ORing of all the bytes, it
> might be good to force them to do so by recommend the ORing of all the
> bytes to be an input to the KDF.

That's not a bad idea but, this late in the process, I'm afraid that
I'm going to shy away from making semantic changes in the document.

> - Sec 6.1: “The check for the all-zero value results from the fact that
> the X25519 function produces that value if it operates on an input
> corresponding to a point with order dividing the co-factor, h, of the
> curve”.
> Am I missing something or doesn’t X25519 only produce the all-zero value
> if the point order divides the input scalar? In this case the text should
> say something like “may produce” or “produces with high probability”.

The scalars are, by construction, a multiple of the cofactor. So an
input point of order, say, 4, will result in the all zero value
because the scalar is a multiple of 8.

So what that sentence is trying to say is that input points of order
2, 4 and 8 will produce the all-zero value. By testing for it, one can
avoid non-contributory behaviour. Protocols shouldn't depend on that
but, historically, some have.

> - App A: the variable ‘r’ (order of the basepoint/order prime order
> subgroup) is not defined. The variable ‘k’ (embedding degree) has already
> been used to denote a scalar in section 5.

Thanks. Changed 'r' to 'order' since that's now defined. Removed 'k'
since it's not referenced anywhere else.

> - Unclear why some values are given in hex and others in decimal. E.g. the
> order Sec. 4,1 and 4,2 is given in hex, while the base point is given in
> decimal. Some hex values use the ‘0x’ prefix, others do not.

I'll admit that some of these might just be my quirks, but the
thinking is something along the lines that values that are inputs to
the X25519 and X448 functions are bytestrings, thus given in hex
without 0x. Values that are numbers are given in decimal (because
that's common), except when the bit-pattern is useful to recognise, in
which case I'll use hex with 0x to make that clear.

> - Most of the document seem to have adopted the style of having space
> around ‘+’ and ‘-’, but this is not followed in a few places e.g.
> s4.1: “2^255-19” -> “2^255 - 19”
> s4.1: “1+y” -> “1 + y”
> s4.2: “2^448-2^224-1” -> “2^448 - 2^224 - 1”

Good point. I've kept the tight spacing in code where it makes sense
but the prose should be spaced consistently now.

The current version can be seen here:
https://cdn.rawgit.com/agl/cfrgcurve/2429fd5b5b3be3362178bc59360d60ffc7f2a8b2/cfrgcurve.xml


Cheers

AGL