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

MSC2757: Sign Events #2757

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

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented Sep 1, 2020

Rendered

Signed-Off-By: Sorunome [email protected]

@Sorunome Sorunome changed the title MSC2756: Sign Messages MSC2757: Sign Messages Sep 1, 2020
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Sep 1, 2020
proposals/2757-sign-events.md Show resolved Hide resolved
proposals/2757-sign-events.md Show resolved Hide resolved
proposals/2757-sign-events.md Outdated Show resolved Hide resolved
The private key can be stored base64-encoded in SSSS under the key `m.message_signing`
(`m.cross_signing.message_signin`? Calling these keys "cross-signing" was prolly not too descriptive...).

### Signing messages
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worthwhile defining signing of state events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that be needed? This MSC outlines signing events in general, and that should also include state events already?

Copy link
Contributor

Choose a reason for hiding this comment

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

The title strongly implies this just signs messages. Also, you may want to add the state_key to the part that gets signed, similar to how the event type is handled.

Copy link
Contributor

@Half-Shot Half-Shot Sep 1, 2020

Choose a reason for hiding this comment

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

It doesn't, because you do not include the state_key in the stripped content. You'd need to define that. (argh message races :|)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So like event_type + state_key + canonical_json, where if state_key is a blank string we have "normal" PDUs?

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 one aspect is if that you banned person A state_key=@alice:example.com, and the server changed that to say state_key=@bob:example.com, it would look to others like you had banned bob.

If the server was clever enough to hide that fact from you somehow, it could look, at least for a bit, like you had done something malicious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would still be signed as state_key=@alice:example.com, so the signature would not match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't get soru wrong, event type and state key are included in the signature as per this MSC, there is just no separater, so it is just event_type + state_key + canonical_json

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. Yes, I don't think you can really attack it. I defer to a spec team grey-beard on whether they want separators because I think it's mostly a style thing at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

even with separators you could always construct event types / state keys to mimic those.

Depends how you design the separators, you could of course sign:

{
  "type": "something",
  "state_key": "blub",
  "content": {...}
}

Is that an attack vector at all?

I don't think so.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Some thoughts when going through the MSC.

Thus, clients will have to think about themselves how to resolve this issue, e.g. by using a securly
encrypted store provided by their platform.

Additionally, you lose deniability if you sign all your messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs expanding upon, as the statement alone doesn't really explain why this is a good/bad thing or how it relates to security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure, the avarage user will pinpoint you to your avatar anyways, no matter if the thing is cryptographically signed or not


### Signing messages

To sign a message you strip the `signatures` and `unsigned` dicts off of the `content` (if present),
Copy link
Contributor

@Half-Shot Half-Shot Sep 1, 2020

Choose a reason for hiding this comment

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

content.unsigned is a new dict for storing things you do not wish to sign? Could you define it's purpose formally in another paragraph. I'm actually not too sure why you wouldn't want to sign part of your content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its purpose mostly exists to stay in-line with everything else that is signable in the matrix spec so far

Copy link
Contributor

Choose a reason for hiding this comment

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

Righto, so in the C-S api representation of events, the unsigned section is for the local server to drop in bits of related information that hasn't been signed by the sending server (usually because it's an age value, or prev_state, which is mutable).

In this case we don't expect any third parties to drop in bits of information into content.unsigned because servers have a section for that. Since the client is sending immutable information, I'd expect they will want to sign everything so the section feels superfluous to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be easiest to include it already in case we have usecases for it in the future. Will note in the MSC that it doesn't really have any usecase yet. Does that seem OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think until someone comes up with a valid use case for it, it's probably not worth speccing it (given these MSCs take ages to go through anyway, there should be ample time for someone to shout).

I'm just thinking that because content is guaranteed to be immutable, there is little argument for including it. Sorry :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is little argument for including it.

Greater code simplicity as signing code clients have will already strip signatures and unsigned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a valid usecase: A proxy (like pantalaimon, only pan doesn't make sense as pan does e2ee, but a different kind of proxy) could add additional information into the unsigned block.

proposals/2757-sign-events.md Show resolved Hide resolved
The private key can be stored base64-encoded in SSSS under the key `m.message_signing`
(`m.cross_signing.message_signin`? Calling these keys "cross-signing" was prolly not too descriptive...).

### Signing messages
Copy link
Contributor

@Half-Shot Half-Shot Sep 1, 2020

Choose a reason for hiding this comment

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

It doesn't, because you do not include the state_key in the stripped content. You'd need to define that. (argh message races :|)

@Sorunome Sorunome changed the title MSC2757: Sign Messages MSC2757: Sign Events Sep 1, 2020

## Proposal

The general idea is to sign the `content` of each event you send out with your own ed25519 key and a
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered how redactions will be handled?

If the new content signature is covered by the existing hash/signature then it will not be able to be redacted when the ”content” is removed. If it isn’t covered by the existing hash/signature then it is also forgeable by the homeserver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, redactions have actually not be considered yet. However, why would they have to be considered specially? The homeserver can just clear out the content alltogether for redactions, like it currently does.

A server will alwasy be able to forge an event being redacted or simply not deliver an event properly. Soru is not sure what a preventable attack vector would be here.


Would yield the following string needing to be signed: `m.room.message{"body":"foxies!","msgtype":"m.text"}`

Prepending the event type and state key is done to rule out attack vectors where the server could modify
Copy link
Contributor

Choose a reason for hiding this comment

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

The server cannot do this without modifying the hashes/signatures of the event and/or the event ID (room v3+later).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the server could still modify type / state key directly before persisting it into its storage, so directly before it generates the event ID.

Furthermore, this also has to be safe for v1 rooms

Copy link
Contributor

Choose a reason for hiding this comment

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

But those keys are covered by the hashes/signature of the event, so if you change those fields, you invalidate the origin hashes/signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can change them already right when the user posts the event to the room, before you send it over to other servers. So right when calling /send/

@Sanqui
Copy link

Sanqui commented Nov 13, 2020

I honestly think this should be implemented as soon as possible. My Matrix client already has a verified key for my device, so signing messages is practically a freebie! If I announce through secure side channels that I sign my messages with a certain key, others can be certain that my messages in public rooms have not been tampered with.

@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
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.

7 participants