Re: [Crypto-panel] Request for review: OPAQUE

[Kevin Lewi <[e-mail]>]

Thanks JP for the review! I went through with the edits you suggested,
and they are incorporated in this PR:
https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/422

Here are my responses, inline:

On Sun, Sep 10, 2023 at 1:27 AM Jean-Philippe Aumasson <
[e-mail]> wrote:

> Please find my comments below. I could not spend as much time as I
> probably should have for a comprehensive, detailed review (approx. 2
> hours), but I hope this will be helpful nonetheless. I apologize in advance
> if some of these points have already been discussed and resolved.
>
> # 2.1
>
> "SerializeElement(element): Map input element to a fixed-length byte array
> buf."
>
> Is it necessary and IETF-standard to name the output value variable?
> This function's output is later assigned to other variables such as
> "blinded_message = SerializeElement(blinded_element)" (5.2.1)
>

Thanks, good catch, removed the "buf" from the sentence.


>
>
> # 2.2
>
> Not really expecting this to be revised, but mentioning it FTR:
>
> The Expand()/Extract() API is specific to HKDF, and not a standard
> crypto primitive API. This precludes the (direct) use of other KDFs than
> HKDF. It might have been more "inclusive" to use standard KDF or XOF
> APIs.
>

>

>
> "a collision-resistant Message Authentication Code (MAC)"
> This is not a cryptographically standard security notion, as a MAC's
> security goal is unforgeability. If some collision resistance is
> additionally required, it should be specified that we're talking (I
> suppose) of collisions with a same secret key.
> Also, these combined requirements will imply in practice the use of a
> PRF, so it might be simpler to require a PRF and restrict its output
> size to greater than some security bound.
>

Thanks for bringing this up as well. Actually, after some comments from
Hugo Krawczyk, we are changing this to require: "random key robust" instead
of "collision-resistant" MAC. Additionally, we are adding the following
definition for a random key robust MAC, under the Security Considerations
section:

"The random-key robustness property is only needed for the MAC function: the
  property being that, given two random keys k1 and k2, it is infeasible to
find a
  message m such that MAC(k1, m) = MAC(k2, m)."

Changes are on github here (search for "random key robust"):
https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/419


> # 3
>
> nit: the "authenticated key exchange" is mentioned, and then 3.1 refers
> to it as the "AKE", but it may not be obvious to all readers that it's
> the "authenticated key exchange". Suggested change: "authenticated key
> exchange (AKE)" in 3.
>

👍


>
> # 3.1
>
> "chooses a seed (oprf_seed) of Nh bytes for the OPRF":
> Is this consistent with earlier statement "all random nonces and seeds
> used in these dependencies and the rest of the OPAQUE protocol are of
> length Nn and Nseed bytes, respectively, where Nn = Nseed = 32."?
>
> Maybe mention earlier that Nn = Nseed = Nh = 32?
>
>
For SHA512 outputs, Nh = 64, whereas for SHA256, Nh = 32. So, Nh would be
an instance of the exception "*Unless said otherwise*, all random nonces
and seeds..."


>
> # 3.2
>
> For the avoidance of doubt, it may be added that "export_key" is a
> symmetric key, not a public key or a private key.
>

Added this text: "The client output of this stage is a single value
`export_key` that the client may use for application-specific purposes,
e.g., *as a symmetric key* used to encrypt..."


> # 4
>
> This section was not clear to me:
> it starts by introducing the concept of Envelope structured, then says
> "The following types of application credential information are
> considered", from which a reader might understand "are part of the
> Envelope" or "may be part of the Envelope".
> Right after that, the text says "These credential values are used in the
> CleartextCredentials", when in fact it's only 3 out of the 5 values.
>

I see 👍. To help make things clearer, I changed the text to say, "*A
subset of* these credential values are used in ..."


>
> # 4.1.1
>
> "nonce: A unique nonce"
> It may be specified that it is unique with respect to the set of users
> of a given application (and set of parameters thereof)
>
> Wouldn't it be safer to "bind" values derived from the nonce to the
> server's identity/publickey? In that case, the nonce should be unique
> only per server, rather than per application parameters.
> But maybe that's on purpose, to allow the use of the same nonce with
> multiple servers?
>

Hmm. This nonce is meant to be sampled randomly upon each envelope creation
(based on the line `envelope_nonce = random(Nn)`). So actually, we do not
want this nonce to ever be reused. Perhaps it would help to clarify that
this nonce is "randomly-sampled" instead of just saying "unique nonce"? I
amended the text to read: "nonce: A randomly-sampled nonce ..." in hopes of
clearing up this confusion.


>
>
> # 4.1.3
>
> "  If !ct_equal(envelope.auth_tag, expected_tag)
>     raise EnvelopeRecoveryError"
>
> We may add that, in such a case, all the previously computed
> values/key/credentials must be discarded/deleted.
>

Added the text: "In the case of `EnvelopeRecoveryError` being raised, all
previously-computed intermediary values in this function MUST be deleted."


>
> # 5.2.1
>
> "password, an opaque byte string"
>
> The definition of an opaque string in this context seems necessary, as
> it may confuse readers (e.g., as related to opaque types).
>
>
Indeed what we mean here by "opaque byte string" is: a string of bytes of
unspecified format. Perhaps @Christopher Wood <[e-mail]> can
chime in on this for suggestions on how we can clarify, as I was assuming
it is the unambiguous terminology for these sorts of things :)


> # 6.3.2.2
>
> "It is RECOMMENDED that a fake client record is created once (e.g. as
> the first user record of the application) and stored alongside
> legitimate client records.  This allows servers to locate the record in
> a time comparable to that of a legitimate client record."
>
> This part wasn't clear to me first (regarding the purpose "to locate").
> It may be clarified that storing such fake record (or, even better, many
> such records and pick a random one) saves the cost/time of computing
> one, thus avoiding potential timing side channels.
>

I clarified by rephrasing the words "to locate" to "to retrieve".


>
> # 7
>
> "is 128-bits" ->  "is 128 bits" (or "is 128-bit").
>
> It may be added that the recommended configurations target 128-bit
> security.
>

Thanks, also added after the configurations, the sentence: "The above
recommended configurations target 128-bit security."


> # 10
>
> Totally trivial and obvious, but shouldn't the security considerations
> mention somewhere the risk of weak/short passwords?
> Here of maybe in section 5 (Offline Registation)
>


Added a sentence at the end of the "## Password Salt and Storage
Implications" section:

"In general, passwords should be selected with sufficient entropy to avoid
being susceptible to recovery through dictionary attacks, both online and
offline."


>
>
> On Fri, Sep 8, 2023 at 1:22 AM Kevin Lewi <[e-mail]> wrote:
>
>> Hi Scott,
>>
>> Replies inline:
>>
>>
>>>
>>>
>>>    - The draft mentions that it supports multiple AKE and OPRF
>>>    algorithms, and that the server may want to select between them (based on
>>>    hardware support).  However, the client initiates the transaction, and the
>>>    generation of the KE1 message would appear to require knowledge of the OPRF
>>>    used; how does the client know this?  Is it communicated out-of-band (say,
>>>    in the DNS record)?  Is there expected to be a preliminary (outside the
>>>    protocol) phase where the client asks the server?  Or, does the client take
>>>    a guess and if the server doesn’t support that, it informs the client of
>>>    the one it does expect (and if so, from the KE1 message, how does the
>>>    server know which one the client guessed)?  I couldn’t find any discussion
>>>    of such issue in the draft, and I don’t believe it would serve the
>>>    implementors right by having them try to figure this out for themselves.
>>>       - This question comes up because one scenario I envision for
>>>       Opaque is “the user has a PC (or tablet or phone) which might not be the PC
>>>       he registered with, and wants to log into the server using his password.
>>>       The PC has no stored information about the server” – how does this work?
>>>
>>>
>> IMO this is outside of the scope of the protocol and the draft. In the
>> Protocol Overview section, under Setup, we say "Prior to both stages, the
>> client and server agree on a configuration that fully specifies the
>> cryptographic algorithm dependencies necessary to run the protocol; see
>> {{configurations}} for details." In the past we discussed this and decided
>> to leave it up to the implementers, primarily because there are many ways
>> this negotiation could happen in practice, and if we pick one format, it
>> may be suitable for some but not for others.
>>
>>
>>>
>>>    - The draft includes support for Ristretto, which is itself
>>>    currently a CFRG draft. Is it expected that Ristretto will become an RFC
>>>    before this does?
>>>
>>>
>> Yes. We also have a dependency on voprf, which itself has a dependency on
>> Ristretto anyways.
>>
>>
>>>
>>>    - At one point, the draft uses “RFCXXXX” as a protocol identifier in
>>>    the preamble; is that expected to be replaced by the assigned RFC number?
>>>    If so, I would suggest you add instructions to the RFC editors to that
>>>    effect.
>>>
>>>
>> Done! Put up a PR for this change:
>> https://github.com/cfrg/draft-irtf-cfrg-opaque/pull/420
>>
>>
>>>
>>>    -
>>>    - Question about 4.1.2: if the DeriveDiffieHellmanKeyPair has a
>>>    nonzero (but low) failure rate, why doesn’t the Store functionality retry
>>>    it internally, rather than asking for the server application to perform
>>>    that logic?  Of course, this would need to include some sanity limit (to
>>>    prevent a bogus input from causing an infinite loop); IMHO, this is
>>>    something that the application shouldn’t be tasked with.
>>>
>>>
>> The failure scenario for DeriveDiffieHellmanKeyPair comes from voprf's
>> "DeriveKeyPair" function:
>> https://www.ietf.org/archive/id/draft-irtf-cfrg-voprf-21.html#section-3.2.1
>> which already does the retry logic that you are describing before raising
>> the error. Presumably it would be unnecessary for us to introduce yet
>> another layer of a retry loop in DeriveDiffieHellmanKeyPair, no?
>>
>> Thanks for the comments!
>>
>> On Tue, Sep 5, 2023 at 10:59 AM Scott Fluhrer (sfluhrer) <
>> [e-mail]> wrote:
>>
>>> Here is my review of the latest Opaque draft:
>>>
>>>
>>>
>>> It looks pretty good; I do have a few questions and nits:
>>>
>>>
>>>
>>>    - The draft mentions that it supports multiple AKE and OPRF
>>>    algorithms, and that the server may want to select between them (based on
>>>    hardware support).  However, the client initiates the transaction, and the
>>>    generation of the KE1 message would appear to require knowledge of the OPRF
>>>    used; how does the client know this?  Is it communicated out-of-band (say,
>>>    in the DNS record)?  Is there expected to be a preliminary (outside the
>>>    protocol) phase where the client asks the server?  Or, does the client take
>>>    a guess and if the server doesn’t support that, it informs the client of
>>>    the one it does expect (and if so, from the KE1 message, how does the
>>>    server know which one the client guessed)?  I couldn’t find any discussion
>>>    of such issue in the draft, and I don’t believe it would serve the
>>>    implementors right by having them try to figure this out for themselves.
>>>       - This question comes up because one scenario I envision for
>>>       Opaque is “the user has a PC (or tablet or phone) which might not be the PC
>>>       he registered with, and wants to log into the server using his password.
>>>       The PC has no stored information about the server” – how does this work?
>>>    - The draft includes support for Ristretto, which is itself
>>>    currently a CFRG draft. Is it expected that Ristretto will become an RFC
>>>    before this does?
>>>    - At one point, the draft uses “RFCXXXX” as a protocol identifier in
>>>    the preamble; is that expected to be replaced by the assigned RFC number?
>>>    If so, I would suggest you add instructions to the RFC editors to that
>>>    effect.
>>>    - Question about 4.1.2: if the DeriveDiffieHellmanKeyPair has a
>>>    nonzero (but low) failure rate, why doesn’t the Store functionality retry
>>>    it internally, rather than asking for the server application to perform
>>>    that logic?  Of course, this would need to include some sanity limit (to
>>>    prevent a bogus input from causing an infinite loop); IMHO, this is
>>>    something that the application shouldn’t be tasked with.
>>>
>>>
>>>
>>>
>>>
>>>