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

MSC3013: Encrypted Push #3013

Open
wants to merge 12 commits into
base: old_master
Choose a base branch
from

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented Feb 17, 2021

proposals/3013-encrypted-push.md Outdated Show resolved Hide resolved
proposals/3013-encrypted-push.md Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review push labels Feb 17, 2021
@flumeware
Copy link

Hi

1. Generate an ephemeral curve25519 key, and perform an ECDH with the ephemeral key and the backup's
   public key to generate a shared secret. The public half of the ephemeral key, encoded using unpadded
   base64, becomes the `ephemeral` property of the new payload.

Should backup's public key be pusher's public key?

Thanks

@kegsay
Copy link
Member

kegsay commented Apr 3, 2021

MSC 3079: Low Bandwidth CS API may be an alternative to this as it makes it viable to rely on /sync for push notifications directly as it is made bandwidth efficient.

@Cadair
Copy link
Contributor

Cadair commented Apr 6, 2021

I think this and #3079 are likely to be complementary. If you are already using your platforms push provider running a permanent background process is still going to be more expensive than using the push provider, no matter how well optimised it is. #3979 with the extra work to add sync filters just for events which trigger push is a good solution to if you don't have (or don't want) and untrusted push provider.

@kegsay
Copy link
Member

kegsay commented Apr 7, 2021

Indeed, they can be complementary but at the cost of an increased API surface. If you don't trust your push provider, that has knock on concerns though. Malicious push providers can do more subtle attacks than just sniff the message (e.g delay the delivery) so the point the provider is untrusted then really you shouldn't be using it at all.

proposals/3013-encrypted-push.md Outdated Show resolved Hide resolved
proposals/3013-encrypted-push.md Show resolved Hide resolved
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@anoadragon453 anoadragon453 removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Dec 7, 2021
@mscbot mscbot added the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Dec 28, 2021
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

I personally think its fine to make the server do some encryption, so this makes sense to me.

data, encoded using unpadded base64, becomes the `ciphertext` property of the new payload.
4. Pass the raw encrypted data (prior to base64 encoding) through HMAC-SHA-256 using the MAC key
generated above. The first 8 bytes of the resulting MAC are base64-encoded, and become the `mac`
property of the new payload.
Copy link
Member

Choose a reason for hiding this comment

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

It's worth mentioning that the server should immediately drop the ephemeral secret key and shared secret after each use (or at least regularly regenerate them). If either secret is leaked then the push content will be decryptable.


## Proposal

A new pusher data field, `algorithm`, is introduced for pushers of the kind `http`. It is an enum,
Copy link
Member

Choose a reason for hiding this comment

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

please could we have some links to the existing spec, so we can see what is changing?

### `m.curve25519-aes-sha2` algorithm

The `m.curve25519-aes-sha2` algorithm indicates that the push payloads are to be sent encrypted.
For this, two new pusher data fields are added: `public_key` and `counts_only_type`. Both fields are
Copy link
Member

Choose a reason for hiding this comment

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

Note that data is really meant to be passed straight through to the push gateway. The push gw spec says:

data: A dictionary of additional pusher-specific data. For ‘http’ pushers, this is the data dictionary passed in at pusher creation minus the url key.

(https://spec.matrix.org/v1.1/push-gateway-api/#post_matrixpushv1notify). (It means property, not key.)

In other words, the contents of data are outside the spec: it's there for client apps and their associated push gateway implementations to use as they choose. (For example, Sygnal has some notes about what it expects in https://github.com/matrix-org/sygnal/blob/main/docs/applications.md).

Ideally, url would be in a different place, but I can't help but feel that we're making the problem worse by adding another three fields that are intended for interpretation by the homeserver rather than the push gateway. Shouldn't we spec a new top-level field?

Comment on lines +85 to +89
3. Stringify the JSON object, and encrypt it using AES-CBC-256 with PKCS#7 padding. This encrypted
data, encoded using unpadded base64, becomes the `ciphertext` property of the new payload.
4. Pass the raw encrypted data (prior to base64 encoding) through HMAC-SHA-256 using the MAC key
generated above. The first 8 bytes of the resulting MAC are base64-encoded, and become the `mac`
property of the new payload.
Copy link
Member

Choose a reason for hiding this comment

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

could we use AES-GCM or something modern rather than AES-CBC, for improved performance and to save having to make a separate MAC?

(olm/megolm avoided GCM because GCM wasn't terribly widely supported at the time - I don't think that's true any more?)

Copy link
Contributor

Choose a reason for hiding this comment

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

This code was intentionally designed to be similar to the current pk encryption in libolm, just without the bugs in it. But switching to a different cryptographic primitive solves that too, I guess. But it would need some of the crypto experts approval then.

Copy link
Member

Choose a reason for hiding this comment

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

The only other (standard) use for libolm's pkencryption is in key backups, and we're moving away from that to symmetric key backup. So being consistent with other things is probably less of a concern.

AES-GCM should be fine.

@richvdh
Copy link
Member

richvdh commented Feb 16, 2022

Just to pick up on an earlier comment by @dbkr

This does add significant complexity (especially because it means Homeservers now have to do elliptic curve crypto too)

note that homeservers always had to do EC crypto, for signing events/requests. Admittedly that was all ed25519 signing rather than x25519 DH, but the point remains that any HS implementation is already going to be linked against a curve25519 library.

## Proposal

A new pusher data field, `algorithm`, is introduced for pushers of the kind `http`. It is an enum,
represented as string. The currently allowed values are `m.plain` and `m.curve25519-aes-sha2`. If the
Copy link
Member

Choose a reason for hiding this comment

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

namespacing and "only these values" are at odds: need to pick either namespacing (and thus it's not an enum) or to restrict to a set of values (like memberships).

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess what was meant here is "currently supported values". It follows the same naming rules as the other encryption algorithms.

@turt2live
Copy link
Member

@mscbot concern Enums vs namespaces
@mscbot concern Algorithm might need further review
@mscbot concern Push data is overloaded currently and should be avoided (use top level keys instead)

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Feb 17, 2022
@turt2live
Copy link
Member

@Sorunome are you able to take a look at updating this to unblock FCP?

@deepbluev7
Copy link
Contributor

@turt2live I am planning to implement the changes proposed in ~1.5 weeks and try them out, then update the MSC. But I am still a bit busy next week (and was for the last 8).

### iOS

Sadly iOS is pretty limiting concerning push notifications. While modifying the content of a push frame,
and thus e2ee push notifications, is possible on iOS, these modified push frames *have* to result in
Copy link
Contributor

Choose a reason for hiding this comment

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

An iOS app can bypass the mandatory display of a user notification for each push by using com.apple.developer.usernotifications.filtering entitlement.

The app develop must request it to Apple before using it. Element-iOS uses it now.

@turt2live
Copy link
Member

@deepbluev7 From the chat in the SCT office room last week it sounds like you're aiming to take this on? It looks like it's stuck in needing an implementation and a text update.

Given it's been over a year since the FCP call was stalled due to concerns, I'm cancelling FCP for now and we can re-start it with modern process/considerations. It sounds like it'll be relatively easy to get back into shape?

@mscbot fcp cancel

Once this is ready for re-review, let us know in the SCT Office please.

@mscbot mscbot added proposal-in-review and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jun 5, 2023
@@ -0,0 +1,209 @@
# MSC3013: Encrypted Push
Copy link
Member

@BillCarsonFr BillCarsonFr Aug 31, 2023

Choose a reason for hiding this comment

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

Can these details (seen in other comments) be written somewhere in the MSC?
For non-e2ee room it would allow to put content of the notification and then avoid additional http call.
But for e2e room it might not help as anyhow the client might need to sync (additional http call) the to_devices to get the megolm key.
On the benefit side it could allow to encrypt room_id/event_id metada, but one could arg that we could just remove them from the payload given that most client will sync on push anyhow.
Some potential problems: Not all push do a notification; clients have to process the push rules locally to know if it should notify.

@p1gp1g
Copy link

p1gp1g commented Jul 12, 2024

Posting here, I commented on a resolved comment..

A (robust IMO) alternative would be to add a webpush pusher kind directly.

The advantage of using web push directly:

  • This is E2EE
  • It supports VAPID
  • This is a standard:
    • Libraries are already available
    • Has been reviewed and acknowledged by experts
    • Webpush to APNS/FCM/HMS gateways already exist
    • The ecosystem should tend to use this instead of other custom protocols
  • This can provide push notifications without gateway to
    • Web app and desktop app
    • Android apps using UnifiedPush
    • Maybe other ? We have seen apple moving a lot into web push support
  • This doesn't have a requirement on the path (making https://github.com/matrix-org/matrix-spec-proposals/pull/2970/files obsolete)

Edit:

To add a bit more context, the specifications already need to be updated:

  • hydrogen, and sygnal (and very probably element web, not searched yet) goes over the specifications because it misses informations required by webpush:
    • They use endpoint, and auth in the PusherData (hydrogen, sygnal), and the specs let understand that only url and format are allowed (specs)

PS: I'm OK to write an MSC for webpush pushkind.

@anoadragon453
Copy link
Member

I think an MSC for a web push pusher would be useful down the road. Feel free to write it up while you're thinking about it @p1gp1g.

@p1gp1g
Copy link

p1gp1g commented Aug 1, 2024

Here it is: #4174

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 proposal-in-review push unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Scheduled - v1.8
Development

Successfully merging this pull request may close these issues.