Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Ratelimit invites by room and target user #9258

Merged
merged 9 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/9258.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add ratelimits to invites in rooms and to specific users.
10 changes: 10 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,8 @@ log_config: "CONFDIR/SERVERNAME.log.config"
# "remote" for when users are trying to join rooms not on the server (which
# can be more expensive)
# - one for ratelimiting how often a user or IP can attempt to validate a 3PID.
# - two for ratelimiting how often invites can be sent in a room or to a
# specific user.
#
# The defaults are as shown below.
#
Expand Down Expand Up @@ -862,6 +864,14 @@ log_config: "CONFDIR/SERVERNAME.log.config"
#rc_3pid_validation:
# per_second: 0.003
# burst_count: 5
#
#rc_invites:
# per_room:
# per_second: 0.3
# burst_count: 10
# per_user:
# per_second: 0.003
# burst_count: 5
Comment on lines +872 to +874
Copy link
Member

Choose a reason for hiding this comment

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

if this is a ratelimit per recipient, does that mean that a spammer can DoS a given recipient so that they can't receive any invites from genuine users any more? what happens to genuine invites - are they just lost somehow?

Copy link
Member

Choose a reason for hiding this comment

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

(could this sort of info be added to the comments in the config file, please?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, true. This does mean In some ways this is somewhat by design, the idea is that you don't want to have users inundated by invites from a lot of different fake accounts. The invites will see the 429 though, so at least its a visible thing. I guess we could ratelimit by (sending_server, target_user) for inbound remote invites so that a spammer would have to have accounts on different servers?

Copy link
Member

Choose a reason for hiding this comment

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

maybe. Honestly, I'm kinda questioning how useful this limit is as it stands.


# Ratelimiting settings for incoming federation
#
Expand Down
19 changes: 19 additions & 0 deletions synapse/config/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,15 @@ def read_config(self, config, **kwargs):
defaults={"per_second": 0.003, "burst_count": 5},
)

self.rc_invites_per_room = RateLimitConfig(
config.get("rc_invites", {}).get("per_room", {}),
defaults={"per_second": 0.3, "burst_count": 10},
)
self.rc_invites_per_user = RateLimitConfig(
config.get("rc_invites", {}).get("per_user", {}),
defaults={"per_second": 0.003, "burst_count": 5},
)

def generate_config_section(self, **kwargs):
return """\
## Ratelimiting ##
Expand Down Expand Up @@ -137,6 +146,8 @@ def generate_config_section(self, **kwargs):
# "remote" for when users are trying to join rooms not on the server (which
# can be more expensive)
# - one for ratelimiting how often a user or IP can attempt to validate a 3PID.
# - two for ratelimiting how often invites can be sent in a room or to a
# specific user.
#
# The defaults are as shown below.
#
Expand Down Expand Up @@ -174,6 +185,14 @@ def generate_config_section(self, **kwargs):
#rc_3pid_validation:
# per_second: 0.003
# burst_count: 5
#
#rc_invites:
# per_room:
# per_second: 0.3
# burst_count: 10
# per_user:
# per_second: 0.003
# burst_count: 5

# Ratelimiting settings for incoming federation
#
Expand Down
2 changes: 1 addition & 1 deletion synapse/federation/federation_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ async def _do_send_invite(
"User's homeserver does not support this room version",
Codes.UNSUPPORTED_ROOM_VERSION,
)
elif e.code == 403:
elif e.code in (403, 429):
raise e.to_synapse_error()
Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for added. This won't pass through retry_after_ms but I presume the client will have some default amount of time they'll wait for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, hmm, why on earth don't we proxy through all the fields??

I don't think clients actually care about retry_after_ms...

else:
raise
Expand Down
4 changes: 4 additions & 0 deletions synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1617,6 +1617,10 @@ async def on_invite_request(
if event.state_key == self._server_notices_mxid:
raise SynapseError(HTTPStatus.FORBIDDEN, "Cannot invite this user")

# We retrieve the room member handler here as to not cause a cyclic dependency
member_handler = self.hs.get_room_member_handler()
member_handler.ratelimit_invite(event.room_id, event.state_key)

# keep a record of the room version, if we don't yet know it.
# (this may get overwritten if we later get a different room version in a
# join dance).
Expand Down
7 changes: 7 additions & 0 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ def __init__(self, hs: "HomeServer"):

self.third_party_event_rules = hs.get_third_party_event_rules()

self._invite_burst_count = (
hs.config.ratelimiting.rc_invites_per_room.burst_count
)

async def upgrade_room(
self, requester: Requester, old_room_id: str, new_version: RoomVersion
) -> str:
Expand Down Expand Up @@ -662,6 +666,9 @@ async def create_room(
invite_3pid_list = []
invite_list = []

if len(invite_list) + len(invite_3pid_list) > self._invite_burst_count:
raise SynapseError(400, "Cannot invite so many users at once")

await self.event_creation_handler.assert_accepted_privacy_policy(requester)

power_level_content_override = config.get("power_level_content_override")
Expand Down
25 changes: 23 additions & 2 deletions synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ def __init__(self, hs: "HomeServer"):
burst_count=hs.config.ratelimiting.rc_joins_remote.burst_count,
)

self._invites_per_room_limiter = Ratelimiter(
clock=self.clock,
rate_hz=hs.config.ratelimiting.rc_invites_per_room.per_second,
burst_count=hs.config.ratelimiting.rc_invites_per_room.burst_count,
)
self._invites_per_user_limiter = Ratelimiter(
clock=self.clock,
rate_hz=hs.config.ratelimiting.rc_invites_per_user.per_second,
burst_count=hs.config.ratelimiting.rc_invites_per_user.burst_count,
)

# This is only used to get at ratelimit function, and
# maybe_kick_guest_users. It's fine there are multiple of these as
# it doesn't store state.
Expand Down Expand Up @@ -144,6 +155,12 @@ async def _user_left_room(self, target: UserID, room_id: str) -> None:
"""
raise NotImplementedError()

def ratelimit_invite(self, room_id: str, invitee_user_id: str):
"""Ratelimit invites by room and by target user.
"""
self._invites_per_room_limiter.ratelimit(room_id)
self._invites_per_user_limiter.ratelimit(invitee_user_id)

async def _local_membership_update(
self,
requester: Requester,
Expand Down Expand Up @@ -387,8 +404,12 @@ async def update_membership_locked(
raise SynapseError(403, "This room has been blocked on this server")

if effective_membership_state == Membership.INVITE:
target_id = target.to_string()
if ratelimit:
self.ratelimit_invite(room_id, target_id)

# block any attempts to invite the server notices mxid
if target.to_string() == self._server_notices_mxid:
if target_id == self._server_notices_mxid:
raise SynapseError(HTTPStatus.FORBIDDEN, "Cannot invite this user")

block_invite = False
Expand All @@ -412,7 +433,7 @@ async def update_membership_locked(
block_invite = True

if not await self.spam_checker.user_may_invite(
requester.user.to_string(), target.to_string(), room_id
requester.user.to_string(), target_id, room_id
):
logger.info("Blocking invite due to spam checker")
block_invite = True
Expand Down
93 changes: 92 additions & 1 deletion tests/handlers/test_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from unittest import TestCase

from synapse.api.constants import EventTypes
from synapse.api.errors import AuthError, Codes, SynapseError
from synapse.api.errors import AuthError, Codes, LimitExceededError, SynapseError
from synapse.api.room_versions import RoomVersions
from synapse.events import EventBase
from synapse.federation.federation_base import event_from_pdu_json
Expand Down Expand Up @@ -191,6 +191,97 @@ def test_rejected_state_event_state(self):

self.assertEqual(sg, sg2)

@unittest.override_config(
{"rc_invites": {"per_room": {"per_second": 0.5, "burst_count": 3}}}
)
def test_invite_by_room_ratelimit(self):
"""Tests that invites from federation in a room are actually rate-limited.
"""
other_server = "otherserver"
other_user = "@otheruser:" + other_server

# create the room
user_id = self.register_user("kermit", "test")
tok = self.login("kermit", "test")
room_id = self.helper.create_room_as(room_creator=user_id, tok=tok)
room_version = self.get_success(self.store.get_room_version(room_id))

def create_invite_for(local_user):
return event_from_pdu_json(
{
"type": EventTypes.Member,
"content": {"membership": "invite"},
"room_id": room_id,
"sender": other_user,
"state_key": local_user,
"depth": 32,
"prev_events": [],
"auth_events": [],
"origin_server_ts": self.clock.time_msec(),
},
room_version,
)

for i in range(3):
self.get_success(
self.handler.on_invite_request(
other_server,
create_invite_for("@user-%d:test" % (i,)),
room_version,
)
)

self.get_failure(
self.handler.on_invite_request(
other_server, create_invite_for("@user-4:test"), room_version,
),
exc=LimitExceededError,
)

@unittest.override_config(
{"rc_invites": {"per_user": {"per_second": 0.5, "burst_count": 3}}}
)
def test_invite_by_user_ratelimit(self):
"""Tests that invites from federation to a particular user are
actually rate-limited.
"""
other_server = "otherserver"
other_user = "@otheruser:" + other_server

# create the room
user_id = self.register_user("kermit", "test")
tok = self.login("kermit", "test")

def create_invite():
room_id = self.helper.create_room_as(room_creator=user_id, tok=tok)
room_version = self.get_success(self.store.get_room_version(room_id))
return event_from_pdu_json(
{
"type": EventTypes.Member,
"content": {"membership": "invite"},
"room_id": room_id,
"sender": other_user,
"state_key": "@user:test",
"depth": 32,
"prev_events": [],
"auth_events": [],
"origin_server_ts": self.clock.time_msec(),
},
room_version,
)

for i in range(3):
event = create_invite()
self.get_success(
self.handler.on_invite_request(other_server, event, event.room_version,)
)

event = create_invite()
self.get_failure(
self.handler.on_invite_request(other_server, event, event.room_version,),
exc=LimitExceededError,
)

def _build_and_send_join_event(self, other_server, other_user, room_id):
join_event = self.get_success(
self.handler.on_make_join_request(other_server, room_id, other_user)
Expand Down
35 changes: 35 additions & 0 deletions tests/rest/client/v1/test_rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,41 @@ def test_rooms_members_other_custom_keys(self):
self.assertEquals(json.loads(content), channel.json_body)


class RoomInviteRatelimitTestCase(RoomBase):
user_id = "@sid1:red"

servlets = [
admin.register_servlets,
profile.register_servlets,
room.register_servlets,
]

@unittest.override_config(
{"rc_invites": {"per_room": {"per_second": 0.5, "burst_count": 3}}}
)
def test_invites_by_rooms_ratelimit(self):
"""Tests that invites in a room are actually rate-limited."""
room_id = self.helper.create_room_as(self.user_id)

for i in range(3):
self.helper.invite(room_id, self.user_id, "@user-%s:red" % (i,))

self.helper.invite(room_id, self.user_id, "@user-4:red", expect_code=429)

@unittest.override_config(
{"rc_invites": {"per_user": {"per_second": 0.5, "burst_count": 3}}}
)
def test_invites_by_users_ratelimit(self):
"""Tests that invites to a specific user are actually rate-limited."""

for i in range(3):
room_id = self.helper.create_room_as(self.user_id)
self.helper.invite(room_id, self.user_id, "@other-users:red")

room_id = self.helper.create_room_as(self.user_id)
self.helper.invite(room_id, self.user_id, "@other-users:red", expect_code=429)


class RoomJoinRatelimitTestCase(RoomBase):
user_id = "@sid1:red"

Expand Down