-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addressing client enumeration section with protocol changes #156
Conversation
Hi Kevin, I can't say I did a thorough review but here are a few comments from a cursory reading. Nothing major.
|
See my most recent comment on #22 (comment) -- I believe we should not make the changes I proposed to the protocol after all. |
1cc6b77
to
262373c
Compare
Nevermind about the previous comment for not making the changes -- after some clarification in #22 where we discussed the Addressed @hugokraw's comments:
Preventing Client EnumerationClient enumeration refers to attacks where the attacker tries to learn
Preventing the first type of attack requires the server to act with Implementations must employ care to avoid side-channel leakage (e.g., Preventing the second type of attack requires the server to supply a In the event of a server compromise that results in a re-registration of |
262373c
to
4a56e37
Compare
The essential difference between the two attacks is that to address the first one the protocol needs not change. The defense against this attack is achieved via server-side implementations that do not impact the client side. The second attack does require a change in the form of the masking of the envelope. However, it is important to note (and this addresses some of @kevinlewi concerns), that once we include the masking technique, the server is free to apply or not apply the defenses we describe. For example, an application where user enumeration is not an issue can ignore the deterministic derivation of oprf_key from the user's identifier and could generate a fresh independent oprf_key with each registration. You may want to comment on this as the need for deterministic derivation and rotation of oprf_seed may be seen as added complexity for those that don't need such defense. They would still be doing the envelope masking since it is now part of the protocol and not just an optional element. The important thing is that the deterministic derivation with oprf_seed is only needed to defend against attack 2 but has no other security implication. |
I assume you are asking about the need for a confidential channel during registration. This is because the (secret) masking_key needs to be sent from client to server (before, we only needed an authenticated channel). In most cases, if you have an authenticated channel (e.g. TLS) then you have a confidential one too (in particular, you can always upgrade authenticated to confidential by sending a public key over the authenticated channel) |
One more comment on the user enumeration defenses: |
@hugokraw makes sense. Yes, as you guessed, I think we are leaning towards reducing the number of security decisions that applications would need to make. Are you saying that you would like to have text added to the current doc that specifically clarifies the flexibility of the server in choosing or not choosing to use the "FakeCredentialResponse" mechanism? I believe that this is already inherent/implied, since a server can simply return "FAIL" rather than bother with running FakeCredentialResponse, and so my vote is to not add this text since it is unnecessary. But, happy to add something if you feel like we should add the clarification. |
It may be worth having language saying that other than envelope masking, which is part of the basic protocol and a must-to-have for interoperability, the rest of FakeCredentialResponse (such as the use of oprf_seed) becomes a pure server-side implementation decision. |
@chris-wood Could the IETF/CFRG impose on the server to implement FakeCredentialResponse for standard-compliance even though it is not needed for interoperability? In principle, such compliance could be tested by simulating a user enumeration attacker. But I doubt it is something IETF would mandate. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-wood Could the IETF/CFRG impose on the server to implement FakeCredentialResponse for standard-compliance even though it is not needed for interoperability? In principle, such compliance could be tested by simulating a user enumeration attacker. But I doubt it is something IETF would mandate. What do you think?
Well, we can certainly say that servers MUST implement the protocol this way to achieve the desired security properties. (This is akin, in my view, to saying that servers MUST choose ephemeral TLS key shares for connections, but some may choose to reuse them.) Whether or not implementations follow the requirement is up to them. :-)
Rebased, added DeriveKeyPair reference, and added the following optional text to the end of the Client Enumeration section:
|
4a56e37
to
ccf45ee
Compare
ccf45ee
to
1678296
Compare
draft-irtf-cfrg-opaque.md
Outdated
- SerializedElement: A serialized OPRF group element, a byte array of fixed | ||
length. | ||
- SerializedScalar: A serialized OPRF scalar, a byte array of fixed length. | ||
- Nok: The size of an OPRF private key | ||
|
||
- Deterministic Key Pair Generation Function: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the OPRF API, right? Why not move it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. Can you clarify what you mean by "move it up"? It is already under the OPRF section -- do you mean to move it up in front of Blind(x)
, Evaluate(k, M)
, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! If it's an OPRF API, it should be in the same list as Blind() etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I am saying is, it is already in the same list as Blind() etc... unless I am misinterpreting what you are saying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in a separate list, like the KDF and other functions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH I see what you mean -- I thought you were referring to the Nok
parameter as needing to move up. But now I realize that you are referring to DeriveKeyPair
being needed to move up. Got it! :) The comment line numbers this points to was a bit ambiguous :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yep :) Sorry, I should have been more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be resolved in the latest diff.
draft-irtf-cfrg-opaque.md
Outdated
## Setup Phase {#setup-phase} | ||
|
||
In a setup phase, C chooses its password, and S chooses its own pair of private-public | ||
AKE keys (server_private_key, server_public_key) for use with the AKE, along with a Nh-byte oprf_seed. S can use | ||
the same pair of keys with multiple clients. These steps can happen offline, i.e., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it required that the same seed be used across all users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not have to be one seed for all. For example, there could be a seed for all userid's that start with letter a, one for those that start with letter b, etc. But a userid should always use the same oprf seed. A change of seed can only happen at re-registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-wood: How should we allow for the flexibility in this API, in terms of producing test vectors, etc. ? Does it suffice to just add a note here saying that multiple oprf_seeds can be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think noting that multiple seeds may be used is sufficient, but the test vectors should just use one seed for all users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a side note that multiple seeds can be used
1678296
to
37d1d42
Compare
Addressed above comments. Also changed the use of "CredentialResponsePad" literal string to use encryption_nonce instead. |
37d1d42
to
bc5f1c1
Compare
Changed names:
|
bc5f1c1
to
fe37df6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo some final nits!
draft-irtf-cfrg-opaque.md
Outdated
@@ -638,21 +647,22 @@ Output: | |||
Steps: | |||
1. y = Finalize(password, blind, response.data) | |||
2. envelope_nonce = random(32) | |||
3. prk = Extract(envelope_nonce, Harden(y, params)) | |||
3. prk = Extract(salt=0, Harden(y, params)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. prk = Extract(salt=0, Harden(y, params)) | |
3. prk = Extract("", Harden(y, params)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, also changed the salt=0 instance to "" in the "OPAQUE-3DH Key Schedule" section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is better to set the salt to 0. It does not have to be described as salt=0 but just 0 (instead of ""). In principle, any fixed salt value can do, but I prefer to see 0 being used as the default "fixed value". In particular, 0 is used in TLS 1.3 key derivation when a random value for salt is not available and here we want to stay as close as possible to TLS 1.3 key derivation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, "" is the notation used in TLS for zero-length strings, which is what we want here, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of 0 in the following TLS 1.3 figure. Is it the value 0 or a 0-length string?
0
|
v
PSK -> HKDF-Extract = Early Secret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I see the answer: "0" indicates a string of Hash.length bytes set to zero.
We should specify the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a zero-length string (or NULL, or the equivalent of providing no salt at all), but I was referring to "" used elsewhere where context strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I see the answer: "0" indicates a string of Hash.length bytes set to zero.
I'm not sure this is best, since the KDF might not be HKDF (and therefore have an implied hash function underneath). I think nil/""/0/whatever are equivalent, and "" seems to be most clear (to me) and consistent with 8446.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. If we are treating the KDF as a general Extract-then-Expand primitive then we cannot say "a string of Hash.length bytes set to zero" but we also cannot say the empty string as it may not make sense in some other KDF. If you want to be generic, then you should refer to this salt value as a "constant" (as opposed to a random one), maybe call it const_salt and set it to "a string of Hash.length bytes set to zero" in the case of HKDF.
fe37df6
to
aba1fca
Compare
oprf_seed
parameter which must be generated during server setup and persisted across all registration/login requests (similar toserver_private_key
)cred_identifier
parameter -- this could be the same asclient_identity
, but does not need to beoprf_key
fromoprf_seed
andcred_identifier
-- this results in alteringRegistrationResponse
andCredentialResponse
oprf_seed
andserver_private_key
need to be kept around tooFinalizeRequest
, adds a new elementenvelope_key
which must be stored as part of aRegistrationUpload
objectenvelope_key
to work properlyencryption_nonce
parameter, and steps for encrypting the envelope usingenvelope_key
, inCreateCredentialResponse
FakeCredentialResponse
functionNok
for representing the size of an OPRF key. Removes unused calls toSerializeScalar
andDeserializeScalar
from the VOPRF draftCloses #22.