cfrg / draft-irtf-cfrg-hash-to-curve

Hashing to Elliptic Curves
Other
79 stars 27 forks source link

respond to a few other comments #283

Closed kwantam closed 4 years ago

kwantam commented 4 years ago

This issue responds to a comment from Michael Scott and one from Marek Jankowski.

The next message in this thread includes all of Marek's review, and some notes. We can discuss further here or in the issues that Chris opened.

kwantam commented 4 years ago

a. I think Table 1 (Sec. 2.1) could be better formatted, using less line breaks. Please consider redistributing the widths of the columns.

Issue #279 --- probably needs to be handled manually.

b. The second paragraph in 2.2.1 can be simplified: it suffices to say it needs not be bijective/surjective/injective. Also I'm not entirely sure it's best practice to use the term 'invertible' for just efficiently reversible functions.

I think the examples here may be useful, and certainly don't hurt clarity.

Not clear re 'invertible'---my understanding is that this is indeed the term used in the crypto literature...

c. I think a better formulation in the first paragraph in 2.2.5 is something like "...assuming the attacker accesses RO only through the protocol, viz. the RO is not used elsewhere."

I think the first part of this statement isn't quite true---generally a random oracle is assumed to be public, so adversaries are allowed to query it. The important thing is that other protocols are not simultaneously trying to query it, because otherwise, e.g., the challenger in the security game can't program the random oracle when it needs to.

I've updated with a cite to BR93 and a clarification that the adversary is allowed to query the oracle, to make this statement more precise.

d. The word 'encode' (Sec. 3) implies that its output can be decoded. Perhaps there's a better word for noninvertible maps.

No action for now---is there a word that Marek recommends?

e. Domain separation (Sec. 3.1): I believe that if needed for backwards compatibility, it is not a disaster if we omit the version number from the Tag. Maybe it is worth a "NOT RECOMMENDED".

Wait---are we now NOT RECOMMENDing a version number after all? Can we ask for clarification on this? Maybe we can discuss more in #280.

f. It says that expand_message MUST NOT use rejection sampling (5.3.4). To the best of my understanding, rejection sampling is to be avoided for the sole purpose of mitigating side channel attacks; this is defined (Sec. 4) as a SHOULD, so I believe this SHOULD should (no pun intended :-) propagate there.

I made an edit in PR #282 to address this. See comments there and in #281.

g. Do we really want to approve as many curves as in [Table 2, Sec. 8]? We are standardizing something new here, so we can allow ourselves to stick to just the best curves. In my opinion Curve25519 is in every way superior to the NIST curve of the same strength, so the latter can be omitted. Again, I am very supportive of the draft and will be happy to further help in the process.

Inclined to keep, per Chris's response on the mailing list.

chris-wood commented 4 years ago

Thanks! @armfazh, please merge if you're OK with the change. I think this adequately addresses Marek's comments.

kwantam commented 4 years ago

Sorry @armfazh somehow the UI ignored me when I asked it to request a review from you earlier :)