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.

Sign up for GitHub

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

Jump to bottom

Don't ratelimit server-sent delayed events #18018

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Don't ratelimit server-sent delayed events #18018

wants to merge 2 commits into from

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Dec 9, 2024 •
edited by hughns
Loading

Fixes #18021

When the server sends a delayed event upon timeout / on-demand sending, don't apply ratelimiting to the event create+send attempt, because it is a send that the server expects to be able to send.

What should (and does) get rate-limited are client requests to manage delayed events.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

@AndrewFerr
Don't ratelimit server-sent delayed events
a9244c8 
When the server sends a delayed event upon timeout / on-demand sending,
don't apply ratelimiting to the event create+send attempt, because it is
a send that the server expects to be able to send.

What should (and does) get rate-limited are client requests to manage
delayed events.
@AndrewFerr AndrewFerr requested a review from a team as a code owner December 9, 2024 16:58
@hughns hughns mentioned this pull request Dec 10, 2024
Open
hughns
hughns reviewed Dec 10, 2024
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

This change goes against the security considerations of MSC4140. Specifically: https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/expiring-events-keep-alive/proposals/4140-delayed-events-futures.md#rate-limiting-at-the-point-of-sending

To make sure that this doesn't get forgotten I've made a tracking issue and added context there: #18021 (comment)

To be clear: I don't think it is a good idea to be removing this rate limit at this point without a planned action of how the risk would be mitigated in future.

@thisisjoesGH
Copy link

What should (and does) get rate-limited are client requests to manage delayed events.

The client request ratelimit is trivially overridden by a malicious homeserver without any code modifications since it is exposed in the Synapse config. Synapse should not be relying on other homeservers to play nice. If the proposed fix is merged it seems like this entire feature will just be a massive new abuse vector.

@AndrewFerr
Copy link
Member Author

The client request ratelimit is trivially overridden by a malicious homeserver without any code modifications since it is exposed in the Synapse config.

If you're referring to non-ratelimited delayed events arriving via federation, I believe that would be protected against by federation-level ratelimiting. Otherwise, there'd already be the possibility of a malicious homeserver disabling all ratelimiting & flooding remote rooms with events.

@thisisjoesGH
Copy link

If you're referring to non-ratelimited delayed events arriving via federation, I believe that would be protected against by federation-level ratelimiting. Otherwise, there'd already be the possibility of a malicious homeserver disabling all ratelimiting & flooding remote rooms with events.

I misunderstood this change as affecting the processing of incoming events rather than outgoing. In that case, is this a potential federation DoS issue? It appears a user could currently schedule an unlimited number of delayed events. Is "a limit on the maximum number of outstanding delayed events" going to be implemented in Synapse as suggested by the MSC?

@AndrewFerr
Copy link
Member Author

Is "a limit on the maximum number of outstanding delayed events" going to be implemented in Synapse as suggested by the MSC?

Yes, this is something I'm also working on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughns hughns hughns left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MSC4140 delayed event rate limit failures are not handled
3 participants
@AndrewFerr @thisisjoesGH @hughns