Server registration finish without username
jonahbeckford opened this issue · 11 comments
This issue is in regards to:
The ServerRegistration has the password file but not the username (credential_identifier). If you expose two server REST methods /ServerRegistrationStart and /ServerRegistrationFinish, your /ServerRegistrationFinish would have to authenticate the username some other way.
I may be missing something in the API, but simple_login.rs seems to assume that you will have a single connection for all the registration messages. A typical REST implementation would violate that assumption. If so, a documentation comment will help users not to forget this part of the authentication.
Thanks. Nice library!
If I understand correctly, I think there is a misunderstanding here. ServerRegistration::finish doesn't require any state data, it also doesn't require credential_identifier.
I assume, the problem was that you were using the credential_identifier as an identifier in your database to save the resulting ServerRegistration. As far as I am concerned this has nothing to do with the OPAQUE protocol and is an implementation detail for the user to figure out.
Did I understand this correctly? Please feel free to elaborate on any point I might have misunderstood there!
If I understand correctly, I think there is a misunderstanding here.
ServerRegistration::finishdoesn't require any state data, it also doesn't requirecredential_identifier.
Agreed.
I assume, the problem was that you were using the
credential_identifieras an identifier in your database to save the resultingServerRegistration. As far as I am concerned this has nothing to do with the OPAQUE protocol and is an implementation detail for the user to figure out.
I haven't saved it as a database identifier yet; just doing a sanity check of how it is meant to be used.
Together the following lib.rs documentation and the parameter name credential_identifier create an impression that the OPAQUE protocol will authenticate the credential_identifier:
If it really is an implementation detail for the user to figure out, can I suggest changing adjusting the above Rust doc so a user doesn't accidentally do the wrong thing?
Perhaps the following can work? I can send a PR:
//! ## Registration
//! The registration protocol between the client and server consists of four
//! steps along with three messages: [RegistrationRequest],
//! [RegistrationResponse], and [RegistrationUpload]. A successful execution of
//! the registration protocol results in the server producing a password file
//! corresponding to a server-side identifier for the client, along with the
//! password provided by the client. This password file is typically stored in a
//! key-value database, where the keys consist of these server-side identifiers
//! for each client, and the values consist of their corresponding password
//! files, to be retrieved upon future login attempts made by the client.
//! --- added text below ---
//! It is your responsibility to authenticate the identifier given in [RegistrationRequest]
//! is the same identifier used as the server-side identifier in a key-value database
//! in the final [RegistrationUpload] step. The OPAQUE protocol will not help
//! authenticate the identifier.Together the following lib.rs documentation and the parameter name
credential_identifiercreate an impression that the OPAQUE protocol will authenticate thecredential_identifier:
The protocol does authenticate the credential_identifier, I'm not sure if the word "authenticate" is right, but the login will fail if not the same credential_identifier was used during registration and login.
If it really is an implementation detail for the user to figure out, can I suggest changing adjusting the above Rust doc so a user doesn't accidentally do the wrong thing?
What I meant is an implementation detail is how you store the ServerRegistration, how the credential_identifier is used in ServerRegistration::start and ServerLogin::start has nothing to do with that.
In any case, clearly the documentation needs to be improved.
More context:
- My language about authentication only makes sense with stateless REST channels, where a) each message has to specify enough parameters so the server can rehydrate the old state and persist the new state and b) each message from the client has to be authenticated by the server. Sorry for my lack of precision.
- If the Registration messages from opaque-ke library travel on stateful Websocket TLS channels, no extra protections are needed.
- Eventually I'll be releasing some low-level OCaml bindings to this library, assuming I end up using the library.
Even though almost all the pieces are present in opaque-ke to use it over a stateless REST channel, I'll be using a stateful channel for the protocol. It may be good to recommend users to do the same as well.
Thanks!
I suspect there is still some misunderstanding.
No extra protection is needed in a stateless connection, e.g. REST. Everything should work as is.
Rehydrating the old state isn't necessary because the server requires no state between ServerRegistration::start and ServerRegistration::finish.
State rehydration would be necessary for ServerLogin::finish though, but no "protection" is needed, all checks are done by the library as intended, the user has to take no extra measures. To rehydrate, all that would be necessary is to store the state after ServerLogin::start and get the same state back during ServerLogin::finish.
I would argue that opaque-ke works quiet well in a stateless connection and I have used it before in such.
Indeed, the only information I used to send along the messages as defined in OPAQUE, is the username, which I used as the credential_identifier, which is required for ServerRegistration::start, for ServerRegistration::finish to store the resulting ServerRegistration, to retrieve the ServerRegistration during ServerLogin::start and the state during ServerLogin::finish.
What am I missing?
Indeed, the only information I used to send along the messages as defined in OPAQUE, is the username, which I used as the credential_identifier, which is required for ServerRegistration::start, for ServerRegistration::finish to store the resulting ServerRegistration, to retrieve the ServerRegistration during ServerLogin::start and the state during ServerLogin::finish.
What is stopping a malicious user from the following 2.0.0-pre.2 sequence?
- The client does ClientRegistration::start.
- During
POST /api/ServerRegistrationStart/:usernamethe ServerRegistration::start correctly usescredential_identifier = "CharlieBot"since Charlie asserted their username was CharlieBot in the URL. The REST method returns the RegistrationResponse back to the client. - The client does ClientRegistration::finish.
- During
POST /api/ServerRegistrationFinish/:usernamethe ServerRegistration::finish creates a valid ServerRegistration. The REST method stores the ServerRegistration in a (username,ServerRegistration) login database. But Charlie is malicious, and has set:username = "JoeBiden". Uh-oh!
Now the opaque-ke adopter has mitigations available to them:
- They can disallow updates in their login database (ie. no SQL UPDATE allowed) ==> no changing passwords
- They can precheck whether the username already existed in the first POST; if so, switch to the Login subprotocol ==> enumeration of usernames
- They can check whether the username exists in the second POST ==> race condition between first and second POST means Charlie can takeover an account during initial registration (perhaps useless). A bit harder for me to explain is the race condition could be exploited during a password reset (useful) as well.
- They can accept the valid ServerRegistration and simply overwrite the username record ==> account takeover
Those not-so-good mitigations are only necessary because the stateless REST client has to pass in the username in the second POST.
None of these mitigations are necessary if a stateful channel is used. And none of these mitigations are necessary if the username could be extracted on the server during ServerRegistration::finish using only the RegistrationUpload document.
Am I missing something?
Hi @jonahbeckford, thanks for the clear explanation. It is indeed the case that the server must check that that credential_identifier is the same as the server-side identifier stored in the database. I think something along the lines of the text you proposed in the documentation looks good (will add comments on exact verbiage in a PR if you submit one).
It isn't possible to securely extract the credential_identifier that was used in ServerRegistration::start from the RegistrationUpload struct. So, keeping state in between the two registration rounds is an alternative option. One way to do this would be to select a random identifier (nonce) that is generated after running the server registration start, and send this to the client, while also storing the credential_identifier <-> nonce mapping server-side. That way, the client can return this nonce in the second step, so that the server can recover the corresponding credential_identifier used in the first step. This would solve the attack you mentioned above, correct?
Anyway, I am supportive of amending the documentation to make this more clear. Thanks for highlighting the gap!
Yes, a server-side nonce mapping would solve the problem. I'll send a small doc PR. Thanks!
I believe there are some issues with this scenario you pointed out above @jonahbeckford:
-
They can disallow updates in their login database (ie. no SQL UPDATE allowed) ==> no changing passwords
POST /api/ServerRegistrationFinish/:usernameshould never be used to update a password. Updating a password requires a user to log in first, which requires some sort of authentication and for example a corresponding session, a problem OPAQUE can help with, seeClientLoginFinishResult::session_key, but is out of scope for. -
They can precheck whether the username already existed in the first POST; if so, switch to the Login subprotocol ==> enumeration of usernames
Not exactly sure what "switch to the Login subprotocol" means, but to fight username enumeration, if that is desired, you want the attacker to sustain the highest cost possible. E.g. don't fail until the very last step, which I believe is what you are saying.
-
They can check whether the username exists in the second POST ==> race condition between first and second POST means Charlie can takeover an account during initial registration (perhaps useless). A bit harder for me to explain is the race condition could be exploited during a password reset (useful) as well.
This comes back to the first point. This REST endpoint should absolutely never
UPDATEor overwrite any registration, as that does require some sort of authentication first. Even in the case of a password reset, after all, you don't want just anybody to be able to reset any password, they need some sort of authentication first, which registration (usually) doesn't. -
They can accept the valid ServerRegistration and simply overwrite the username record ==> account takeover
Same as the first point.
@kevinlewi I think we should be careful about adding overly use-case specific recommendations or suggestions. Clearly there is a gap in the documentation though.
I suggest we should specifically address the stateless approach, as this is actually quiet common:
- Regarding to @jonahbeckford problem, a reason I can come up with, to check if the
credential_identifieris the same between the first and second registration step, is if it's used as the username and to reserve that username between steps. In that case, you want to check if thecredential_identifieris the same. - Another reason I can come up with to retain state between the first and second registration step, is for sanity checks. Clearly somebody using different
credential_identifierbetween these steps is malicious (unless there is a bug), which at the very least should be flagged somehow. You might also want to refuse a request like that because saving such a record is useless, as authentication with this data will forever be impossible, and wouldn't only clutter your database, but also reserve an unused username. - I believe we should definitely point out, that
ServerRegistration::finishshould never be used to overwrite or update any data, without a previous authentication of some sort. That is the attack that @jonahbeckford is pointing out above.
P.S.: I don't want to overstep, I'm just a lowly contributor dropping my two cents here. Ultimately @kevinlewi is responsible here and actually knows what he is talking about.
Right, I think when reading @jonahbeckford's description, I did not interpret this as an attack that allows a malicious user to overwrite data -- indeed that would mean that the server is blindly allowing to overwrite records without authentication, which would probably make for an insecure application. I instead interpreted it as your second point: somebody supplying a different credential_identifier between the steps would create a useless record.
The text that @jonahbeckford proposed to amend the documentation with seems not too specific to any application, and I think can be framed in a way that addresses potential issues with the stateless approach. I'd be happy to deliberate over the phrasing in the comments of a PR.
And P.S.: This feedback is super valuable and I wouldn't worry about overstepping. Thank you for continuing to provide your contributions and helpful suggestions!