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

MSC2730: Verifiable forwarded events #2730

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

Conversation

tulir
Copy link
Member

@tulir tulir commented Aug 12, 2020

Rendered

This is an alternative to #2723

Synapse implementation: matrix-org/synapse#8078
Element web implementation: matrix-org/matrix-js-sdk#1439 / matrix-org/matrix-react-sdk#5117

Signed-off-by: Tulir Asokan <tulir@maunium.net>

Signed-off-by: Tulir Asokan <tulir@maunium.net>
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Aug 12, 2020
tulir added 3 commits August 13, 2020 01:20
Signed-off-by: Tulir Asokan <tulir@maunium.net>
Signed-off-by: Tulir Asokan <tulir@maunium.net>
Signed-off-by: Tulir Asokan <tulir@maunium.net>
@tulir tulir marked this pull request as ready for review August 12, 2020 22:39
@tulir
Copy link
Member Author

tulir commented Aug 13, 2020

There's now an initial Synapse implementation: matrix-org/synapse#8078

@MurzNN
Copy link
Contributor

MurzNN commented Aug 14, 2020

Maybe allow to forward multiple events in one forwarded event? This feature will can also partly implement https://github.com/matrix-org/matrix-doc/issues/2289

Signed-off-by: Tulir Asokan <tulir@maunium.net>
Comment on lines +221 to +234
### Encrypted events
In some cases, users may want to forward encrypted messages to rooms with users
who are not in the origin room. In order to allow everyone in the recipient
room to decrypt the forwarded message, the keys must be sent with the message.
However, only keys for the message being forwarded should be sent, any other
messages in the origin room must not be decryptable with those keys.

To achieve this, the user forwarding the message includes the message-specific
symmetric AES and HMAC keys (see [Message encryption] in the Megolm spec). Each
of the keys are encoded as unpadded base64 and placed in the `aes_key`,
`hmac_key` and `aes_iv` fields in the `decryption_keys` object in the forward
request.

[Message encryption]: https://gitlab.matrix.org/matrix-org/olm/-/blob/master/docs/megolm.md#message-encryption
Copy link
Member Author

Choose a reason for hiding this comment

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

Someone who knows cryptography should make sure this actually makes sense and is secure enough (i.e. doesn't leak anything else than the one message being forwarded)

Copy link

Choose a reason for hiding this comment

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

When forwarding events to other encrypted rooms this approach does not seem to leak any additional information nor session security related secrets of the megolm context of the originating room. The receiving room members can only encrypt this single message under normal circumstances. If there are any corner cases or bugs in the clients that cause re-use of the megolm index for two events, then that specific event would be decryptable as well, but this should not happen anyway.

The actual issue to consider is that should we break the trust model of Matrix E2EE by leaking the key information to the HS? What makes this a big issue is that the actual original event contents combined with all of the event meta information is then explicitly leaked outside the original E2EE context in cryptographically verifiable manner. This is totally different case than copy-pasting secret message to another context and claiming that it originated somewhere.

Thus, I would strongly suggest that E2EE secured events should only be allowed to be forwarded in this verifiable manner to other E2EE rooms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there should be mandatory server-side limits, but I guess I could reword the client section to say "Clients are strongly recommended to discourage or disallow users from forwarding encrypted messages to unencrypted rooms".

(also, megolm keys are technically not cryptographically verifiable, only keys received via a secure olm channel can actually be verified to originate from the user - this is mentioned in the client behavior section)

Copy link

Choose a reason for hiding this comment

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

This does still build a direct functionality to the spec to leak the message related secrets to the HS in plaintext, if forwarding to unencrypted rooms is allowed. HS itself has the original event, and thus can then see not only the meta information of the event it originally was aware of, but also the contents of it.

IMO this approach would be a violation of the implicit trust model Matrix tries to provide by E2EE. This design decision should not be made lightly.

Would it be worth considering to leave the decision of event forwarding to the context of a room? Then forwarding could be allowed to other encrypted rooms only, or all rooms, or denied in full. This will at least provide some guarantees that the data hygiene and separation of privileges can be upheld to sufficient degree.

Signed-off-by: Tulir Asokan <tulir@maunium.net>
Signed-off-by: Tulir Asokan <tulir@maunium.net>
Comment on lines +169 to +172
"unsigned": {
"displayname": "tulir",
"avatar_url": "mxc://maunium.net/jdlSfvudiMSmcRrleeiYjjFO"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is missing the room_version. (Relatedly, what should happen if it is missing?)

Copy link
Contributor

Choose a reason for hiding this comment

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

To expand on this what if the room version is not supported by clients? I guess in most cases it will just work but it seems unfortunate if a client can't see a forward in a room with a supported version because the event is from a newer versioned room.

Copy link
Member Author

Choose a reason for hiding this comment

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

Clients don't need to support the room version, but servers do. I guess missing or unsupported room_version can just be marked as non-trusted/invalid

Copy link
Contributor

Choose a reason for hiding this comment

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

That isn't a great experience. If I am in a room version X why am I being sent content in a different room version? Maybe we should support some sort of fallback content? Of course now you need to worry about verifying that the fallback content matches the source content. (which is basically impossible if the room version the source is from us unsupported)

Copy link
Member Author

@tulir tulir Apr 30, 2021

Choose a reason for hiding this comment

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

The only client-facing change in room versions so far is the event ID (which is mentioned in the proposal). Even in the future there most likely won't be any huge client-facing changes, since room versions are for changing the federation protocol. Major client-facing changes like extensible events won't bump the room version.

Servers currently support all room versions, deprecating older room versions hasn't been decided yet. Even if there are unsupported versions, it only means the forward metadata won't be verified.

implemented as a new endpoint, while validating happens automatically and the
server adds the validation result to the top-level `unsigned` object.

### `PUT /_matrix/client/r0/rooms/{roomId}/event/{eventId}/forward/{targetRoomId}/{txnId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This seems like the wrong place for this. We aren't modifying {roomId} so it seems weird to be PUTing to that URL. It would make more sense to have something like PUT /_matrix/client/r0/rooms/{targetRoomId}/forward/{sourceRoomId}/{txnId}.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PUT is there mostly because of {txnId}, otherwise it would be POST.

The current path is that way because you're doing something with the event. /_matrix/client/r0/rooms/{targetRoomId}/forward/{sourceRoomId}/{eventId}/{txnId} feels less clear in that way

Copy link
Contributor

Choose a reason for hiding this comment

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

But you aren't actually doing something to the event. You are creating a new event that is based on it and references it.

* If the source event was not a forward, but contains (invalid) data in the
`m.forwarded` key, the request will be rejected with `M_NOT_FORWARDABLE` like
other unforwardable events. This limitation also can be used to intentionally
mark messages as unforwardable (e.g. `"m.forwarded": {"allow": false}`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting "invalid" data in this key seems like something that we shouldn't recommend. If we are going to recommend this I would prefer to update the forwarding field to explicitly allow this option.

a notice saying it's unverified.

When receiving forwarded encrypted events, clients should treat the message
like they treat forwarded keys, i.e. not confirmed to originate from the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear what should the assume? AKA what does this verification give us?

IIUC it tells us if our homeserver trusts that this event came from the homeserver of the sender? Is this correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

It means this message in elements:

image

Currently happens when you get keys from a key backup or key sharing, rather than directly from the origin device. The reason is that Matrix e2ee is deniable, so there's no way to ensure that the message was sent by the user unless you received the keys directly from that user. Forwarded messages are equivalent to key sharing, so clients should treat those keys the same way.

relation target event. The existence of reply metadata may still be used to
remove reply fallbacks.

## Potential issues
Copy link
Contributor

@kevincox kevincox Apr 12, 2021

Choose a reason for hiding this comment

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

A concern I have is that this requires forwarding all of the information in the original event. I can imagine cases where I may want to reference the author, but not expose the room. Other cases could not want to reveal the time of the message. Basically there is a legitimate use case for including any subset of the information from the original event.

Furthermore it is likely unclear to the forwarder exactly what information they are sharing.

This proposal seems to suggest that it is not allowed to forward modified (including redacted) events. Of course modified events can not be verified but I think it is still nice to have all of this structured data about the event even if it can't be verified to be coming from the original user's homeserver. For example I may just want to forward (message, author, time) and am perfectly fine saying "according to Kevin".

So basically how should we handle non-verifiable forwarded events? The same as this protocol but just without "valid": true? Or do we want to separate spec?

Copy link

Choose a reason for hiding this comment

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

In my opinion copying content from a message to other selectively is a totally different use-case than forwarding a full message. This MSC should be tightly restricted to forwarding full events only, and more importantly, I would explicitly deny forwarding of encrypted events to un-encrypted rooms!

When forwarding an encrypted event, you don't only copy the original message, but the actual point-in-time event sent by an original author and including all the meta information in it to another context. While this probably has a bunch of valid use-cases, I consider it a very bad practice to make it easy for 2nd parties of original communication to leak all this to the HSes when the original message content is deliberately shielded from server side spying. When a user copy-pastes a content of a originally encrypted message, the relation to the original encrypted counterpart is not explicitly leaked to the homeservers, and the relation to the original encrypted context is not explicitly crytpographically proven. This is a major difference that should be considered when forwarding the original events to other contexts.

Thus, my recommendation is that forwarding of encrypted messages to unencrypted rooms should be blocked by both the server and the client implementations explicitly. Forwarding of encrypted events to other encrypted rooms does not break the separation of trust contexts between the HS and the clients as long as the decryption keys are encrypted with the keys of the target room.

Regarding the thoughts on selectively "forwarding" some of the event information the current signing scheme does not support being picky on what to include or exclude if you want to be able to verify the signature. Building something that would allow forwarding selectively, while still providing cryptographic proofs would require a hugely more complex cryptosystem design than Matrix currently has. One possibility to that direction would be using something like Zero Knowledge proofs, but that is totally out of scope in terms of added complexity for this single use case.

Comment on lines +169 to +172
"unsigned": {
"displayname": "tulir",
"avatar_url": "mxc://maunium.net/jdlSfvudiMSmcRrleeiYjjFO"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To expand on this what if the room version is not supported by clients? I guess in most cases it will just work but it seems unfortunate if a client can't see a forward in a room with a supported version because the event is from a newer versioned room.

Comment on lines +1 to +3
# Verifiable forwarded events
This is an alternative to [MSC2723](https://github.com/matrix-org/matrix-doc/pull/2723)
that handles the issue of faking forwards.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this more I'm not sure that this represents a valuable use case. I also think the risk of forwarding more information than you wanted to is too high.

Particularly for the best forwarding experience it probably makes sense to trim the message anyways. If we want to support various ways of redacting parts of the forwarded event (which I think we should) we currently don't have a way to verify that and it will be confusing to clients to warn about "according to the sender" for some forwards but having other forwards be verified.

The most compelling use case I can think of are abuse reports but I think in most cases the moderator receiving the report can see the room anyways or otherwise check the legitimacy of the report.

Copy link
Contributor

Choose a reason for hiding this comment

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

Forwarding message means that it isn't changed, if you modify the message text (redact part of it), it is already quoting, not forwarding.

Copy link
Contributor

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

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

All in all i find this a sound proposal with some genius ideas, i'd still recommend someone versed in cryptography and privacy to look at all the data this is forwarding as well, and if it's possible to redact any of it (optionally)

The prospect of adding another endpoint to a server would make me not like this MSC on first glance (in a world where P2P is the future), but seeing as it's merely needed to have the server copy signature information (which is then self-verifying by other servers/devices), I like this solution.

@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

Additionally, the server MUST include an `unsigned` object, containing a
`room_version` field that specifies the version of the source room. The server
SHOULD also include the sender's profile metadata in the unsigned object under
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn’t this unsigned/unverified profile data at risk of falsification?

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 needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants