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

unsigned should be stripped of disallowed keys when received over S2S #1524

Open
dkasak opened this issue May 16, 2023 · 4 comments
Open

unsigned should be stripped of disallowed keys when received over S2S #1524

dkasak opened this issue May 16, 2023 · 4 comments
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@dkasak
Copy link
Member

dkasak commented May 16, 2023

We should spell out the fact that the unsigned field of PDUs received over S2S should be stripped of everything except an explicitly allowed set of keys, to prevent issues as described in matrix-org/synapse#11080.

Synapse already has mitigations for this, as does Dendrite.

@dkasak dkasak added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label May 16, 2023
@turt2live turt2live changed the title unsigned should be stripped of non-whitelisted keys when received over S2S unsigned should be stripped of disallowed keys when received over S2S May 16, 2023
@turt2live
Copy link
Member

imo we shouldn't need to strip anything at the protocol level, but should be clear that "untrusted event bodies" goes as far as unsigned too.

@dkasak
Copy link
Member Author

dkasak commented May 16, 2023

@turt2live Did you read the replaces_state example in matrix-org/synapse#11080? What do you propose Synapse should do instead if not strip that field when it receives it over federation?

@turt2live
Copy link
Member

Synapse can strip it, but the protocol doesn't have to :)

@dkasak
Copy link
Member Author

dkasak commented May 17, 2023

I guess so, but given all major servers now do it like that, I'm unsure of what benefit the added laxness buys us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

2 participants