Skip to content
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

[WIP] MSC3262 aPAKE authentication #3262

Draft
wants to merge 22 commits into
base: old_master
Choose a base branch
from

Conversation

mvgorcum
Copy link

@mvgorcum mvgorcum commented Jul 2, 2021

Rendered

Element Android Matrix room #secure-login:matrix.org

This MSC is more or less competing with MSC2957 and MSC3265.

Comments and thoughts very welcome.
I am specifically on the fence about advocating for SRP or OPAQUE.

Signed-off-by: Mathijs van Gorcum mvgorcum@gmail.com

@Half-Shot Half-Shot self-requested a review July 2, 2021 13:01
@mvgorcum mvgorcum changed the title [WIP] MSC aPAKE authentication [WIP] MSC3262 aPAKE authentication Jul 2, 2021
@uhoreg uhoreg marked this pull request as draft July 2, 2021 15:25
@uhoreg uhoreg added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Jul 2, 2021
proposals/3262-apake_authentication.md Outdated Show resolved Hide resolved
proposals/3262-apake_authentication.md Outdated Show resolved Hide resolved
proposals/3262-apake_authentication.md Outdated Show resolved Hide resolved
proposals/3262-apake_authentication.md Show resolved Hide resolved
proposals/3262-apake_authentication.md Show resolved Hide resolved
the new password, and as such it doesn't protect against a mitm.*


## Security considerations
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a malicious server starts reporting that it no longer supports SRP or that the client does not have SRP configured? It will take some really good UX design by clients to ensure that the client doesn't fall back to a regular login and leak the password to the server.

Copy link
Author

@mvgorcum mvgorcum Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a big one, for quite a while we won't be able to assume the server has no access to the plaintext password. I would imagine that the long-term solution is to remove the password authentication from the matrix spec, and declare all apps still supporting the old password login as incompatible.

I added some thoughts about this. Do you have any additions?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is fine. I don't think there is way to enforce it yet. Maybe add a note that clients can consider indicating when SRP is/isn't used. Maybe that indication would even take a while to be enabled by default but it provides a nice path away from sending the password to the server.

  1. Opportunistic, the client uses SRP if advertised.
  2. Recommended. The client warns if SRP isn't going to be used. (Or shows a "lock" if it is)
  3. Enforced. The client refuses to send the password to the server. (At least without some very explicit disclaimer)

Of course we shouldn't mandate client behaviour, however I think it is often helpful to explain what it could look like.

proposals/3262-apake_authentication.md Show resolved Hide resolved
mvgorcum and others added 3 commits July 14, 2021 07:36
Co-authored-by: Matthew Hodgson <matthew@arasphere.net>
Co-authored-by: Matthew Hodgson <matthew@arasphere.net>
Co-authored-by: Matthew Hodgson <matthew@arasphere.net>

```
{
"type": "m.login.srp6a.verify",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises an interesting question about how the Matrix user-interactive auth is supposed to work.

For a stage that requires multiple rounds of communication, should the type be the same for every request? ie, here it would be m.login.srp6a for both init and verify, and the details would go in the auth object.

Or should each "round" of communication be its own UIA stage? In that case, it seems like the flows returned by GET /_matrix/client/r0/login should include both stages, ie ["m.login.srp6a.init", "m.login.srp6a.verify"].

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flow only has one GET request at the very start, so returning just m.login.srp6a seems like it's sufficient to me, supporting just init or just verify makes little sense so I would argue that's implied.

If we compare against the webrtc spec (which also has multiple stages) I think using m.login.srp6a.init and m.login.srp6a.verify in POST to /login makes sense. (webRTC in matrix use m.call.invite, m.call.candidates, m.call.answer and m.call.hangup). We defined an auth_id in this MSC because we do need to keep track of the flows, you can't verify before having done the init.

Then again: srp can map fairly well on the UIA, but that's a seperate MSC and I kept that out of scope here because there's no reason either this msc or the UIA msc needs to depend one another.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, I think I was getting confused between the UIA vs normal login. I thought at some point they were going to all use the UIA? Or maybe that was just a proposal that I saw.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too, I may have missed that the proposal got merged?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course this MSC actually needs to support UIA, because authentication doesn't only happen at /login so this is added now. It's more or less a duplicate of /login except all SRP json stuff in put inside an auth object.

@cvwright
Copy link

For my $0.02, I think this proposal represents the best immediate path forward for a more secure login mechanism in Matrix. SCRAM has some unfortunate weaknesses, and while @uhoreg's MSC2957 looks great, it has not yet undergone the same level of scrutiny that SRP has. My proposal in MSC3265 is a short-term hack, and only really improves the protection of the SSSS password; MSC3265 was intended to be more informative than prescriptive.

@cvwright
Copy link

At @uhoreg's suggestion, I made a quick survey of available SRP libraries that Matrix implementations might use to implement this MSC.

  • Python srp
  • Python srptools
  • JavaScript mozilla/node-srp
  • Go 1Password/srp
    • The README says that this code is not compatible with RFC 2945 or RFC 5054 due to differences in hashing and padding. Not sure what the practical impact would be for an implementation that's not aiming to implement those RFC's exactly.
  • Swift Bouke/SRP
    • Note: The author says that the Python srp package is not compatible with this code, as it doesn't calculate k correctly. But he says that the Python srptools package is compatible.
  • C/C++ OpenSSL but it's deprecated
  • Rust srp
    • Note: This implementation also allows for plugging in different password hashing functions (eg PBKDF2 or bcrypt) to make brute force attacks more difficult. This is a very good idea, and should probably be codified in the MSC itself.

The groups are referenced by name as:

```
["2048","3072","4096","6144","8192","1536MODP","2048MODP","3072MODP","4096MODP","6144MODP","8192MODP"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these groups, the password hashes and the hash functions be put under the m.* prefix. This way it is obvious to clients how to add more, and which ones are "official" as part of the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants