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

[Jean-Philippe Aumasson <[e-mail]>]

Thanks Kevin for taking the time to respond to my comments, I reviewed the
PR and the changes look good to me.



On Mon 11 Sep 2023 at 23:55 Kevin Lewi <[e-mail]> wrote:

> 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.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>