cfrg / draft-irtf-cfrg-opaque

The OPAQUE Asymmetric PAKE Protocol
https://cfrg.github.io/draft-irtf-cfrg-opaque/draft-irtf-cfrg-opaque.html
Other
100 stars 20 forks source link

Various edits suggested by JP Aumasson #422

Closed kevinlewi closed 1 year ago

kevinlewi commented 1 year ago

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

kevinlewi commented 1 year ago

@chris-wood I think this is good to go, feel free to merge it in!