Subject: [Cfrg] comments on draft-irtf-cfrg-hpke

From: "Riad S. Wahby" <[email-masked]>
Date: Tue, 2 Jun 2020 13:08:21 -0700

Hello HPKE authors,

This is a great document, and I am looking forward to its publication.
I've attached a few small comments and suggestions below my signature.
Thanks for the hard work! and please let me know if any of my feedback
is unclear.

Best regards and stay safe,

-=rsw

(Note: these comments pertain to the version on GitHub with commit
ID d1e58f3d89c3790fef15c756c6279d0811c6f7ab.)

- Section 3: encode_big_endian seems to be the same as I2OSP from RFC8017.
  Would it make sense to reference the existing primitive instead, or at
  least to note that the two are identical? 

- Section 4: the definitions of AuthEncap and AuthDecap contain words to the
  effect, 'the KEM shared secret key is known only to the holder of the
  private key "skS".' It would be more accurate to say , 'the KEM shared
  secret key was generated by the holder of the private key "skS"'.

- Section 4 and 4.1: it's not totally obvious to me that this is necessary,
  but might it make sense to note that Unmarshal can fail?
  
- Section 4.1: this may be paranoia, but it would be slightly nicer to include
  the DH group name in the label arguments of LabeledExtract and LabeledExpand
  to ensure that invocations from different DHKEM instantiations are orthogonal.
  
- Section 5.1.3: it would be nice to include a reference or citation for
  unknown key share attacks.
  
- Section 5.2: is there a reason to put the word "amortize" in quotes? 

- Section 7.1.2: it might be worth mentioning here that [keyagreement] also
  includes checking that the public key is not the identity point.
  
- Section 7.1.2: is there a reason to recommend either checking for a nonzero 
  scalar or checking for a non-identity DH output? Checking the latter covers
  the former and also covers the check from my prior comment. Moreover, it is
  not clear to me that checking the scalar is useful for the recipient, since
  this is essentially just checking that their long-term secret is nonzero.
  
- Section 8.1: the sentence "In particular, the KDFs and DH groups..." might
  want to clarify that this statement is true only when these primitives are
  used as specified. The concern is that HKDF is only indifferentiable under
  some restrictions on salt length (for reasons noted in Section 8.3).
  
- Section 8.4: it is not quite true that using a PSK longer than Nh bytes 
  has no effect on security, since in practice there is no guarantee that
  each byte of PSK contributes 8 bits of entropy. Making this distinction
  seems worthwhile, lest people decide to use only Nh bytes with lots of
  structure (e.g., ASCII text).
  
  (I am aware that the next paragraph discusses low-entropy passwords, 
  but I am talking about an intermediate regime. For example, consider 
  the case of a diceware passphrase: each word contributes only 10--15
  bits of entropy, but is ~6 bytes long. Using 12 words is certainly
  better than using 6, even though 6 words already has length greater
  than Nh for SHA-256.)
  
- Section 8.4: would it make sense to point to PBKDF / bcrypt / scrypt here?
  Or is the idea to discourage use of low-entropy passwords entirely?  (But
  one might worry that folks will use low-entropy passwords anyway...)

---

From: "Christopher Wood" <[email-masked]>
Date: Wed, 03 Jun 2020 13:42:35 -0700

Hi Riad,

Thanks for the feedback! Please see inline below for followup comments.

>   least to note that the two are identical? 

Good point. We should probably use I2OSP, as that's what other CFRG documents use:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/104

>   secret key was generated by the holder of the private key "skS"'.

That's a fine clarification:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/105

>   but might it make sense to note that Unmarshal can fail?

Yes, definitely, that's a great observation:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/106

>   to ensure that invocations from different DHKEM instantiations are orthogonal.

That seems reasonable to me. We went through a couple iterations on domain separation, and it would be a shame to have this lingering:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/108

>   unknown key share attacks.

I concur. I'll fold it in an issue addressing editorial changes:

   https://github.com/cfrg/draft-irtf-cfrg-hpke/issues/107

>   includes checking that the public key is not the identity point.

Agreed. I'll fold these into #107.

>   this is essentially just checking that their long-term secret is nonzero.

We could certainly clarify what's required here. This check was meant to be before senders and receivers invoke the DH() function. It could be easier to just check the output. I'll address this in #107. 

>   some restrictions on salt length (for reasons noted in Section 8.3).

Agreed. I'll fold this into #107 as well.
   
>   better than using 6, even though 6 words already has length greater

Hmm, fair point. We can clarify (in #107).

>   one might worry that folks will use low-entropy passwords anyway...)

I don't see why not! I'll add the reference as part of #107.

Thanks again for the thoughtful feedback and careful review. I may ask you to review these changes before they land. :-)

Best,
Chris