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

Encryption for message relationships #660

Open
uhoreg opened this issue Jul 7, 2020 · 13 comments
Open

Encryption for message relationships #660

uhoreg opened this issue Jul 7, 2020 · 13 comments
Labels
A-Client-Server Issues affecting the CS API A-E2EE Issues about end-to-end encryption feature Suggestion for a significant extension which needs considerable consideration

Comments

@uhoreg
Copy link
Member

uhoreg commented Jul 7, 2020

Event relationships are currently sent unencrypted so that they may be aggregated server-side. However, they may contain information that we do not want to be plain-text (e.g. we don't want the server to know if a user reacted with a 👍 or 👎 ). We should figure out a way to encrypt them. matrix-org/matrix-spec-proposals#1849 includes some thoughts on how to do encryption, and sorunome and deepbluev7 have also given some thoughts at matrix-org/matrix-spec-proposals#1849 (comment)

@turt2live turt2live added A-Client-Server Issues affecting the CS API A-E2EE Issues about end-to-end encryption feature Suggestion for a significant extension which needs considerable consideration labels Jul 7, 2020
@Sorunome
Copy link
Contributor

Sorunome commented Jan 2, 2021

There was yesterday? two days ago? recently! a lengthy discussion about this in the spec room with multiple people. While multiple ideas were pitched, the things everyone seemed to agree on were:

  1. Have a copy of m.relates_to in the cyphertext, so that after decrypting a client has the full metadata on the relationship
  2. Do not have the cleartext rel_type in the event. Some were in favour of leaving it out at all, not allowing the server to aggregate it, while others proposed way to encrypt the type (and event Id) to allow the server to aggregate arbitrary strings.

@Sorunome
Copy link
Contributor

Sorunome commented Jan 2, 2021

Idea developed together with nico on how to maybe do encryption of event_id (and maybe rel_type, soru might prefer it to not be aggregatable at all): Have the attribute in plaintext be sha_512(event_id + enc_key + value), where enc_key is the (base64'd?) aes decryption key for the message this one refers to, event_id is the event_id of the message it refers to, and value is the value that it would have in plaintext.

So then the cyphertext of a reaction would read

{
  "type": "m.reaction",
  "content": {
    "m.relates_to": {
      "event_id": "$orig_event",
      "rel_type": "m.annotation",
      "key": "🦊",
    }
  }
}

And the event would then read

{
  "type": "m.room.encryption",
  "body": {
      // normal encrypted stuff
      "m.relates_to": {
        "event_id": sha512("$orig_event" + base64_aes_key_of_orig_event + "$orig_event"),
        "rel_type": sha512("$orig_event" + base64_aes_key_of_orig_event + "m.annotation"), // soru would prefer to leave this one out
    }
  }
}

That way the server can still aggregate random strings, clients can calculate the random strings as they can decrypt messages, and clients have the proper / real relation in the megolm cyphertext.

hereby it is crucial to only include event_id (and rel_type), and NOT key, or anything else, as the server can otherwise know which relation type it is, ruining the whole hashing. This might also be important for MSCs like matrix-org/matrix-spec-proposals#2730

@bkil
Copy link

bkil commented Sep 23, 2021

Could we perhaps consider using a HMAC construction for enhanced security?

@Mikaela

This comment has been minimized.

@MazeChaZer
Copy link

MazeChaZer commented Oct 5, 2021

@Mikaela no, that problem is covered in #368, and also in the MSC matrix-org/matrix-spec-proposals#2781.

@dkasak
Copy link
Member

dkasak commented Oct 22, 2021

@Sorunome I think the approach you outlined breaks event aggregations, does it not? Since the server no longer knows which event an event with a relationship refers to, only that it contains a relationship.

@Sorunome
Copy link
Contributor

Sorunome commented Oct 23, 2021

that is incorrect; the server can still aggregate relations, however by a (for the server) random opaque string, rather than the event id. The client can calculate said opaque string from the event to be able to query the server then.

If that is unwanted it'd ofc be trivial to not encrypt the event id and just stick to the rel_type and key and whatever else may come in the future

@dkasak
Copy link
Member

dkasak commented Oct 25, 2021

that is incorrect; the server can still aggregate relations

I'm talking about bundled relations specifically. These should be returned along with messages from a bunch of APIs, specifically: the initial sync, gappy incremental sync, /messages and /context.

Since the server no longer knows which relations pertain to which event IDs, I don't see how these could be returned from e.g. /context along with the returned message events. Of course, we could also change those APIs so that the requester also sends the obfuscated identifiers when making the call... But at that point, it's pretty obvious to the server which obfuscated identifiers map to which event IDs, so we buy nothing in the end.

So I'm not sure hiding event IDs will be feasible.

If that is unwanted it'd ofc be trivial to not encrypt the event id and just stick to the rel_type and key and whatever else may come in the future

Yes, the general idea of hiding stuff like key inside the encrypted event is sound. I hope rel_type would be hideable as well, though there might be some interaction with the upcoming feature thread (for instance if it required special handling for the thread rel_type).

@strct
Copy link

strct commented Jan 28, 2022

Why does the server need to aggregate the messages at all? Isn't it enough that the client does it for encrypted rooms? That would encrypt a lot more metadata and would be desirable from my point of view.

@richvdh richvdh transferred this issue from matrix-org/matrix-spec-proposals Mar 1, 2022
@JanZerebecki
Copy link

There is a trade off for requiring clients to download full history and compute an index vs storing an encrypted index that gets updated by anything appending state to the room and letting other clients only download the slices of the index it queries, both provide better privacy than a server readable index. (However for its full effect that requires something like Sealed Sender and Anonymous Credentials, see #549 .)

@FSG-Cat
Copy link
Contributor

FSG-Cat commented Mar 14, 2023

Just wanted to leave a comment documenting that Encryption for Reactions specifically is no longer blocked on aggregation grounds as they are no longer being aggregated as of matrix-org/matrix-spec-proposals@749198f

Synapse implements this change to the MSC in 1.79.0rc1 and later as of writing.

@richvdh
Copy link
Member

richvdh commented Mar 14, 2023

Just wanted to leave a comment documenting that the blocker of that we will break aggregations or that aggregations are important for encrypting of reactions specifically is no longer an issue as of matrix-org/matrix-spec-proposals@749198f

I'm afraid I'm struggling to parse this sentence. Please can you rephrase.

@Neustradamus

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Client-Server Issues affecting the CS API A-E2EE Issues about end-to-end encryption feature Suggestion for a significant extension which needs considerable consideration
Projects
None yet
Development

No branches or pull requests

13 participants