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

MSC3903: X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secure channel between two Matrix clients #3903

Closed
wants to merge 16 commits into from

Conversation

hughns
Copy link
Member

@hughns hughns commented Oct 4, 2022

@hughns hughns changed the title X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secur… X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secure channel between two Matrix clients Oct 4, 2022
@hughns hughns changed the title X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secure channel between two Matrix clients MSC3903: X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secure channel between two Matrix clients Oct 4, 2022
@hughns hughns marked this pull request as ready for review October 4, 2022 17:26
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Oct 4, 2022
This proposal introduces yet another key that Matrix client implementations need awareness of. It's also not clear to me
where exactly this would fit in the spec documents.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

it sounds like this is duplicating Olm sessions over to-device? Early on in this proposal there should be something to address that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, acknowledged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, no. It's actually that you could achieve this with to-device messaging instead.

Copy link
Member

Choose a reason for hiding this comment

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

MSC3886 vs to-device messaging is one thing, and I appreciate you creating a comparison of the two. But I think we still need a comparison between this and Olm. I think a natural question is why can't we just do Olm over MSC3886?

Copy link
Member

Choose a reason for hiding this comment

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

this^

Copy link
Member Author

@hughns hughns Oct 18, 2023

Choose a reason for hiding this comment

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

I concur that the feasibility of Olm over MSC3886 should be assessed.

proposals/3903-x25519-ecdhe.md Outdated Show resolved Hide resolved
proposals/3903-x25519-ecdhe.md Outdated Show resolved Hide resolved
proposals/3903-x25519-ecdhe.md Outdated Show resolved Hide resolved
Copy link

@notramo notramo left a comment

Choose a reason for hiding this comment

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

This is horrible UX.


This means that for AES-GCM implementations that require the authentication tag to be explicitly processed (e.g. CryptoKit on iOS) that it can be sourced from the last 16 bytes of the base64-decoded `ciphertext`.

**7.** The user should confirm that the established channel is secure by means of a checksum derived from the shared key.
Copy link

Choose a reason for hiding this comment

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

This is CRAPPY, HORRIBLE UX! WHY? Why another visual comparison, another “I don't get what should I do now.” moment for users? Matrix already has too much visual comparison in its UX.

You already have a secure channel by means of QR code. Utilize it well, so users don't have to compare checksums.

Copy link
Member

Choose a reason for hiding this comment

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

The QR code is only secure in the sense that you can be sure that its contents were generated by the device that you're trying to communicate with. But it is not secret, as it is sometimes possible that someone else can scan the QR code as well. So unless you have another step (in this case, a visual comparison) to verify that the other public key was transmitted correctly, you cannot be sure that the channel is secure.

proposals/3903-x25519-ecdhe.md Outdated Show resolved Hide resolved
proposals/3903-x25519-ecdhe.md Outdated Show resolved Hide resolved
@turt2live turt2live requested a review from a team February 28, 2023 02:34
@turt2live turt2live self-requested a review February 28, 2023 02:34
@turt2live
Copy link
Member

Like in #3886 (comment) , the author believes this is ready for FCP. The MSC appears to have conflicts with toDevice messaging though, so to get a slightly different review tone I'm putting this in technical review needed.

This doesn't mean the MSC isn't ready for FCP, just that the review given should be more useful to handle.

@matrix-org/spec-core-team please review this for technical design.

@hughns
Copy link
Member Author

hughns commented Mar 15, 2023

There has been more engagement in the topic of to-device messaging in #3886 so I have consolidated the notes and discussion onto that proposal so that they can be worked through more easily.

displayed on both devices for the user to visually verify.

The checksum should be derived in a similar manner to step **4** above, however
only 40 bit should be derived this time. The salt and info are the same as before.
Copy link
Member

Choose a reason for hiding this comment

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

The info should be different, otherwise the checksum will leak bits of the encryption key. You can prepend the info with checksum|.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. As it stands, this looks like a gaping hole.

**1b.** The initiator derives the public key from `privateA` as
`publicA = scalarMult(privateA, 9)`

**1c.** The initiator shares it's key with the recipient via a trusted medium
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**1c.** The initiator shares it's key with the recipient via a trusted medium
**1c.** The initiator shares its key with the recipient via a trusted medium

This proposal introduces yet another key that Matrix client implementations need awareness of. It's also not clear to me
where exactly this would fit in the spec documents.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

MSC3886 vs to-device messaging is one thing, and I appreciate you creating a comparison of the two. But I think we still need a comparison between this and Olm. I think a natural question is why can't we just do Olm over MSC3886?

## Security considerations

Algorithm selection and implementation are crucial.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this protects against replay attacks. That might be OK if the thing built on top of it can detect if a message is replayed (e.g. if messages have a definite order, so a replayed message would be flagged as being out of place), but I think it should be mentioned.

@turt2live turt2live removed their request for review June 6, 2023 19:48
This proposal introduces yet another key that Matrix client implementations need awareness of. It's also not clear to me
where exactly this would fit in the spec documents.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

this^

As Diffie-Hellman key agreement is a non-authenticated key-agreement protocol,
this proposal makes use of a checksum for the user to authenticate the key agreement.

**1a.** The initiator generates a ephemeral Curve25519 private key `privateA`.
Copy link
Member

Choose a reason for hiding this comment

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

nit: please use markdown's built-in numbering/bullet system

Comment on lines +3 to +11
In [MSC3906](https://github.com/matrix-org/matrix-spec-proposals/pull/3906) a
proposal is made to allow a user to login on a new device using an existing
device by means of scanning a QR code.

[MSC3886](https://github.com/matrix-org/matrix-spec-proposals/pull/3886) already
proposes a simple unsecured rendezvous protocol.

In this proposal we build a secure layer on top of MSC3886 to allow for trusted
out-of-bands communication between two Matrix clients.
Copy link
Member

Choose a reason for hiding this comment

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

The reference to MSC3906 should be clarified by stating what kind of relationship it has with this MSC.

Suggested change
In [MSC3906](https://github.com/matrix-org/matrix-spec-proposals/pull/3906) a
proposal is made to allow a user to login on a new device using an existing
device by means of scanning a QR code.
[MSC3886](https://github.com/matrix-org/matrix-spec-proposals/pull/3886) already
proposes a simple unsecured rendezvous protocol.
In this proposal we build a secure layer on top of MSC3886 to allow for trusted
out-of-bands communication between two Matrix clients.
In [MSC3906](https://github.com/matrix-org/matrix-spec-proposals/pull/3906) a
proposal is made to allow a user to login on a new device using an existing
device by means of scanning a QR code, followed by exchanging some messages over
a secure channel.
[MSC3886](https://github.com/matrix-org/matrix-spec-proposals/pull/3886) already
proposes a simple unsecured rendezvous protocol.
In this proposal we build a secure layer on top of MSC3886 to allow for trusted
out-of-bands communication between two Matrix clients, which satisfies the
requirements of MSC3906.

Comment on lines +20 to +21
The proposal is to use X25519 to agree a shared secret that is then used to
perform AES.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The proposal is to use X25519 to agree a shared secret that is then used to
perform AES.
The proposal is to use X25519 to agree on a shared secret that is then used to
perform AES.

As Diffie-Hellman key agreement is a non-authenticated key-agreement protocol,
this proposal makes use of a checksum for the user to authenticate the key agreement.

**1a.** The initiator generates a ephemeral Curve25519 private key `privateA`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**1a.** The initiator generates a ephemeral Curve25519 private key `privateA`.
**1a.** The initiator generates an ephemeral Curve25519 private key `privateA`.

of the curve).

**2.** The recipient similarly generates a private key `privateB`, derives the
public key `publicB` and shares is using the same structure of payload:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public key `publicB` and shares is using the same structure of payload:
public key `publicB` and shares it using the same structure of payload:

@@ -0,0 +1,157 @@
# MSC3903: X25519 Elliptic-curve Diffie-Hellman ephemeral for establishing secure channel between two Matrix clients
Copy link
Member

Choose a reason for hiding this comment

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

This entire MSC is essentially re-specifying ECIES (some more resources: [1], [2]).

MIT-licensed ECIES implementations in Rust, Python, TypeScript and Golang available here.

secure by means of a checksum that is shown on both devices.

If the checksum shown is not the same on both defives then it means that the
devices have not directly exchanged keys with one another and are subject to a man-in-the-middle.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
devices have not directly exchanged keys with one another and are subject to a man-in-the-middle.
devices have not directly exchanged keys with one another and are subject to a
man-in-the-middle attack.

displayed on both devices for the user to visually verify.

The checksum should be derived in a similar manner to step **4** above, however
only 40 bit should be derived this time. The salt and info are the same as before.
Copy link
Member

Choose a reason for hiding this comment

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

Yep. As it stands, this looks like a gaping hole.

@hughns
Copy link
Member Author

hughns commented Apr 9, 2024

It is proposed that MSC4108 supersedes this MSC.

@hughns
Copy link
Member Author

hughns commented Apr 30, 2024

Closing this PR as #4108 is now ready for review.

@hughns hughns closed this Apr 30, 2024
@turt2live turt2live added the obsolete A proposal which has been overtaken by other proposals label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API hacktoberfest-accepted kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
Status: Done for now
Development

Successfully merging this pull request may close these issues.

6 participants