Improve validation for `send_{join,leave,knock}` (#10225)
The idea here is to stop people sending things that aren't joins/leaves/knocks through these endpoints: previously you could send anything you liked through them. I wasn't able to find any security holes from doing so, but it doesn't sound like a good thing.
This commit is contained in:
Richard van der Hoff
GitHub
parent
bd4919fb72
commit
6e8fb42be7
|
|
@ -0,0 +1 @@
|
|||
Improve validation on federation `send_{join,leave,knock}` endpoints.
|
||||
|
|
@ -34,7 +34,7 @@ from twisted.internet import defer
|
|||
from twisted.internet.abstract import isIPAddress
|
||||
from twisted.python import failure
|
||||
|
||||
from synapse.api.constants import EduTypes, EventTypes
|
||||
from synapse.api.constants import EduTypes, EventTypes, Membership
|
||||
from synapse.api.errors import (
|
||||
AuthError,
|
||||
Codes,
|
||||
|
|
@ -46,6 +46,7 @@ from synapse.api.errors import (
|
|||
)
|
||||
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
|
||||
from synapse.events import EventBase
|
||||
from synapse.events.snapshot import EventContext
|
||||
from synapse.federation.federation_base import FederationBase, event_from_pdu_json
|
||||
from synapse.federation.persistence import TransactionActions
|
||||
from synapse.federation.units import Edu, Transaction
|
||||
|
|
@ -537,26 +538,21 @@ class FederationServer(FederationBase):
|
|||
return {"event": ret_pdu.get_pdu_json(time_now)}
|
||||
|
||||
async def on_send_join_request(
|
||||
self, origin: str, content: JsonDict
|
||||
self, origin: str, content: JsonDict, room_id: str
|
||||
) -> Dict[str, Any]:
|
||||
logger.debug("on_send_join_request: content: %s", content)
|
||||
context = await self._on_send_membership_event(
|
||||
origin, content, Membership.JOIN, room_id
|
||||
)
|
||||
|
||||
assert_params_in_dict(content, ["room_id"])
|
||||
room_version = await self.store.get_room_version(content["room_id"])
|
||||
pdu = event_from_pdu_json(content, room_version)
|
||||
prev_state_ids = await context.get_prev_state_ids()
|
||||
state_ids = list(prev_state_ids.values())
|
||||
auth_chain = await self.store.get_auth_chain(room_id, state_ids)
|
||||
state = await self.store.get_events(state_ids)
|
||||
|
||||
origin_host, _ = parse_server_name(origin)
|
||||
await self.check_server_matches_acl(origin_host, pdu.room_id)
|
||||
|
||||
logger.debug("on_send_join_request: pdu sigs: %s", pdu.signatures)
|
||||
|
||||
pdu = await self._check_sigs_and_hash(room_version, pdu)
|
||||
|
||||
res_pdus = await self.handler.on_send_join_request(origin, pdu)
|
||||
time_now = self._clock.time_msec()
|
||||
return {
|
||||
"state": [p.get_pdu_json(time_now) for p in res_pdus["state"]],
|
||||
"auth_chain": [p.get_pdu_json(time_now) for p in res_pdus["auth_chain"]],
|
||||
"state": [p.get_pdu_json(time_now) for p in state.values()],
|
||||
"auth_chain": [p.get_pdu_json(time_now) for p in auth_chain],
|
||||
}
|
||||
|
||||
async def on_make_leave_request(
|
||||
|
|
@ -571,21 +567,11 @@ class FederationServer(FederationBase):
|
|||
time_now = self._clock.time_msec()
|
||||
return {"event": pdu.get_pdu_json(time_now), "room_version": room_version}
|
||||
|
||||
async def on_send_leave_request(self, origin: str, content: JsonDict) -> dict:
|
||||
async def on_send_leave_request(
|
||||
self, origin: str, content: JsonDict, room_id: str
|
||||
) -> dict:
|
||||
logger.debug("on_send_leave_request: content: %s", content)
|
||||
|
||||
assert_params_in_dict(content, ["room_id"])
|
||||
room_version = await self.store.get_room_version(content["room_id"])
|
||||
pdu = event_from_pdu_json(content, room_version)
|
||||
|
||||
origin_host, _ = parse_server_name(origin)
|
||||
await self.check_server_matches_acl(origin_host, pdu.room_id)
|
||||
|
||||
logger.debug("on_send_leave_request: pdu sigs: %s", pdu.signatures)
|
||||
|
||||
pdu = await self._check_sigs_and_hash(room_version, pdu)
|
||||
|
||||
await self.handler.on_send_leave_request(origin, pdu)
|
||||
await self._on_send_membership_event(origin, content, Membership.LEAVE, room_id)
|
||||
return {}
|
||||
|
||||
async def on_make_knock_request(
|
||||
|
|
@ -651,29 +637,9 @@ class FederationServer(FederationBase):
|
|||
Returns:
|
||||
The stripped room state.
|
||||
"""
|
||||
logger.debug("on_send_knock_request: content: %s", content)
|
||||
|
||||
room_version = await self.store.get_room_version(room_id)
|
||||
|
||||
# Check that this room supports knocking as defined by its room version
|
||||
if not room_version.msc2403_knocking:
|
||||
raise SynapseError(
|
||||
403,
|
||||
"This room version does not support knocking",
|
||||
errcode=Codes.FORBIDDEN,
|
||||
)
|
||||
|
||||
pdu = event_from_pdu_json(content, room_version)
|
||||
|
||||
origin_host, _ = parse_server_name(origin)
|
||||
await self.check_server_matches_acl(origin_host, pdu.room_id)
|
||||
|
||||
logger.debug("on_send_knock_request: pdu sigs: %s", pdu.signatures)
|
||||
|
||||
pdu = await self._check_sigs_and_hash(room_version, pdu)
|
||||
|
||||
# Handle the event, and retrieve the EventContext
|
||||
event_context = await self.handler.on_send_knock_request(origin, pdu)
|
||||
event_context = await self._on_send_membership_event(
|
||||
origin, content, Membership.KNOCK, room_id
|
||||
)
|
||||
|
||||
# Retrieve stripped state events from the room and send them back to the remote
|
||||
# server. This will allow the remote server's clients to display information
|
||||
|
|
@ -685,6 +651,63 @@ class FederationServer(FederationBase):
|
|||
)
|
||||
return {"knock_state_events": stripped_room_state}
|
||||
|
||||
async def _on_send_membership_event(
|
||||
self, origin: str, content: JsonDict, membership_type: str, room_id: str
|
||||
) -> EventContext:
|
||||
"""Handle an on_send_{join,leave,knock} request
|
||||
|
||||
Does some preliminary validation before passing the request on to the
|
||||
federation handler.
|
||||
|
||||
Args:
|
||||
origin: The (authenticated) requesting server
|
||||
content: The body of the send_* request - a complete membership event
|
||||
membership_type: The expected membership type (join or leave, depending
|
||||
on the endpoint)
|
||||
room_id: The room_id from the request, to be validated against the room_id
|
||||
in the event
|
||||
|
||||
Returns:
|
||||
The context of the event after inserting it into the room graph.
|
||||
|
||||
Raises:
|
||||
SynapseError if there is a problem with the request, including things like
|
||||
the room_id not matching or the event not being authorized.
|
||||
"""
|
||||
assert_params_in_dict(content, ["room_id"])
|
||||
if content["room_id"] != room_id:
|
||||
raise SynapseError(
|
||||
400,
|
||||
"Room ID in body does not match that in request path",
|
||||
Codes.BAD_JSON,
|
||||
)
|
||||
|
||||
room_version = await self.store.get_room_version(room_id)
|
||||
|
||||
if membership_type == Membership.KNOCK and not room_version.msc2403_knocking:
|
||||
raise SynapseError(
|
||||
403,
|
||||
"This room version does not support knocking",
|
||||
errcode=Codes.FORBIDDEN,
|
||||
)
|
||||
|
||||
event = event_from_pdu_json(content, room_version)
|
||||
|
||||
if event.type != EventTypes.Member or not event.is_state():
|
||||
raise SynapseError(400, "Not an m.room.member event", Codes.BAD_JSON)
|
||||
|
||||
if event.content.get("membership") != membership_type:
|
||||
raise SynapseError(400, "Not a %s event" % membership_type, Codes.BAD_JSON)
|
||||
|
||||
origin_host, _ = parse_server_name(origin)
|
||||
await self.check_server_matches_acl(origin_host, event.room_id)
|
||||
|
||||
logger.debug("_on_send_membership_event: pdu sigs: %s", event.signatures)
|
||||
|
||||
event = await self._check_sigs_and_hash(room_version, event)
|
||||
|
||||
return await self.handler.on_send_membership_event(origin, event)
|
||||
|
||||
async def on_event_auth(
|
||||
self, origin: str, room_id: str, event_id: str
|
||||
) -> Tuple[int, Dict[str, Any]]:
|
||||
|
|
|
|||
|
|
@ -553,7 +553,7 @@ class FederationV1SendLeaveServlet(BaseFederationServerServlet):
|
|||
PATH = "/send_leave/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
|
||||
|
||||
async def on_PUT(self, origin, content, query, room_id, event_id):
|
||||
content = await self.handler.on_send_leave_request(origin, content)
|
||||
content = await self.handler.on_send_leave_request(origin, content, room_id)
|
||||
return 200, (200, content)
|
||||
|
||||
|
||||
|
|
@ -563,7 +563,7 @@ class FederationV2SendLeaveServlet(BaseFederationServerServlet):
|
|||
PREFIX = FEDERATION_V2_PREFIX
|
||||
|
||||
async def on_PUT(self, origin, content, query, room_id, event_id):
|
||||
content = await self.handler.on_send_leave_request(origin, content)
|
||||
content = await self.handler.on_send_leave_request(origin, content, room_id)
|
||||
return 200, content
|
||||
|
||||
|
||||
|
|
@ -602,9 +602,9 @@ class FederationV1SendJoinServlet(BaseFederationServerServlet):
|
|||
PATH = "/send_join/(?P<room_id>[^/]*)/(?P<event_id>[^/]*)"
|
||||
|
||||
async def on_PUT(self, origin, content, query, room_id, event_id):
|
||||
# TODO(paul): assert that room_id/event_id parsed from path actually
|
||||
# TODO(paul): assert that event_id parsed from path actually
|
||||
# match those given in content
|
||||
content = await self.handler.on_send_join_request(origin, content)
|
||||
content = await self.handler.on_send_join_request(origin, content, room_id)
|
||||
return 200, (200, content)
|
||||
|
||||
|
||||
|
|
@ -614,9 +614,9 @@ class FederationV2SendJoinServlet(BaseFederationServerServlet):
|
|||
PREFIX = FEDERATION_V2_PREFIX
|
||||
|
||||
async def on_PUT(self, origin, content, query, room_id, event_id):
|
||||
# TODO(paul): assert that room_id/event_id parsed from path actually
|
||||
# TODO(paul): assert that event_id parsed from path actually
|
||||
# match those given in content
|
||||
content = await self.handler.on_send_join_request(origin, content)
|
||||
content = await self.handler.on_send_join_request(origin, content, room_id)
|
||||
return 200, content
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1711,80 +1711,6 @@ class FederationHandler(BaseHandler):
|
|||
|
||||
return event
|
||||
|
||||
async def on_send_join_request(self, origin: str, pdu: EventBase) -> JsonDict:
|
||||
"""We have received a join event for a room. Fully process it and
|
||||
respond with the current state and auth chains.
|
||||
"""
|
||||
event = pdu
|
||||
|
||||
logger.debug(
|
||||
"on_send_join_request from %s: Got event: %s, signatures: %s",
|
||||
origin,
|
||||
event.event_id,
|
||||
event.signatures,
|
||||
)
|
||||
|
||||
if get_domain_from_id(event.sender) != origin:
|
||||
logger.info(
|
||||
"Got /send_join request for user %r from different origin %s",
|
||||
event.sender,
|
||||
origin,
|
||||
)
|
||||
raise SynapseError(403, "User not from origin", Codes.FORBIDDEN)
|
||||
|
||||
event.internal_metadata.outlier = False
|
||||
# Send this event on behalf of the origin server.
|
||||
#
|
||||
# The reasons we have the destination server rather than the origin
|
||||
# server send it are slightly mysterious: the origin server should have
|
||||
# all the necessary state once it gets the response to the send_join,
|
||||
# so it could send the event itself if it wanted to. It may be that
|
||||
# doing it this way reduces failure modes, or avoids certain attacks
|
||||
# where a new server selectively tells a subset of the federation that
|
||||
# it has joined.
|
||||
#
|
||||
# The fact is that, as of the current writing, Synapse doesn't send out
|
||||
# the join event over federation after joining, and changing it now
|
||||
# would introduce the danger of backwards-compatibility problems.
|
||||
event.internal_metadata.send_on_behalf_of = origin
|
||||
|
||||
# Calculate the event context.
|
||||
context = await self.state_handler.compute_event_context(event)
|
||||
|
||||
# Get the state before the new event.
|
||||
prev_state_ids = await context.get_prev_state_ids()
|
||||
|
||||
# Check if the user is already in the room or invited to the room.
|
||||
user_id = event.state_key
|
||||
prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None)
|
||||
prev_member_event = None
|
||||
if prev_member_event_id:
|
||||
prev_member_event = await self.store.get_event(prev_member_event_id)
|
||||
|
||||
# Check if the member should be allowed access via membership in a space.
|
||||
await self._event_auth_handler.check_restricted_join_rules(
|
||||
prev_state_ids,
|
||||
event.room_version,
|
||||
user_id,
|
||||
prev_member_event,
|
||||
)
|
||||
|
||||
# Persist the event.
|
||||
await self._auth_and_persist_event(origin, event, context)
|
||||
|
||||
logger.debug(
|
||||
"on_send_join_request: After _auth_and_persist_event: %s, sigs: %s",
|
||||
event.event_id,
|
||||
event.signatures,
|
||||
)
|
||||
|
||||
state_ids = list(prev_state_ids.values())
|
||||
auth_chain = await self.store.get_auth_chain(event.room_id, state_ids)
|
||||
|
||||
state = await self.store.get_events(list(prev_state_ids.values()))
|
||||
|
||||
return {"state": list(state.values()), "auth_chain": auth_chain}
|
||||
|
||||
async def on_invite_request(
|
||||
self, origin: str, event: EventBase, room_version: RoomVersion
|
||||
) -> EventBase:
|
||||
|
|
@ -1960,44 +1886,6 @@ class FederationHandler(BaseHandler):
|
|||
|
||||
return event
|
||||
|
||||
async def on_send_leave_request(self, origin: str, pdu: EventBase) -> None:
|
||||
"""We have received a leave event for a room. Fully process it."""
|
||||
event = pdu
|
||||
|
||||
logger.debug(
|
||||
"on_send_leave_request: Got event: %s, signatures: %s",
|
||||
event.event_id,
|
||||
event.signatures,
|
||||
)
|
||||
|
||||
if get_domain_from_id(event.sender) != origin:
|
||||
logger.info(
|
||||
"Got /send_leave request for user %r from different origin %s",
|
||||
event.sender,
|
||||
origin,
|
||||
)
|
||||
raise SynapseError(403, "User not from origin", Codes.FORBIDDEN)
|
||||
|
||||
event.internal_metadata.outlier = False
|
||||
|
||||
# Send this event on behalf of the other server.
|
||||
#
|
||||
# The remote server isn't a full participant in the room at this point, so
|
||||
# may not have an up-to-date list of the other homeservers participating in
|
||||
# the room, so we send it on their behalf.
|
||||
event.internal_metadata.send_on_behalf_of = origin
|
||||
|
||||
context = await self.state_handler.compute_event_context(event)
|
||||
await self._auth_and_persist_event(origin, event, context)
|
||||
|
||||
logger.debug(
|
||||
"on_send_leave_request: After _auth_and_persist_event: %s, sigs: %s",
|
||||
event.event_id,
|
||||
event.signatures,
|
||||
)
|
||||
|
||||
return None
|
||||
|
||||
@log_function
|
||||
async def on_make_knock_request(
|
||||
self, origin: str, room_id: str, user_id: str
|
||||
|
|
@ -2061,34 +1949,38 @@ class FederationHandler(BaseHandler):
|
|||
return event
|
||||
|
||||
@log_function
|
||||
async def on_send_knock_request(
|
||||
async def on_send_membership_event(
|
||||
self, origin: str, event: EventBase
|
||||
) -> EventContext:
|
||||
"""
|
||||
We have received a knock event for a room. Verify that event and send it into the room
|
||||
on the knocking homeserver's behalf.
|
||||
We have received a join/leave/knock event for a room.
|
||||
|
||||
Verify that event and send it into the room on the remote homeserver's behalf.
|
||||
|
||||
Args:
|
||||
origin: The remote homeserver of the knocking user.
|
||||
event: The knocking member event that has been signed by the remote homeserver.
|
||||
origin: The homeserver of the remote (joining/invited/knocking) user.
|
||||
event: The member event that has been signed by the remote homeserver.
|
||||
|
||||
Returns:
|
||||
The context of the event after inserting it into the room graph.
|
||||
"""
|
||||
logger.debug(
|
||||
"on_send_knock_request: Got event: %s, signatures: %s",
|
||||
"on_send_membership_event: Got event: %s, signatures: %s",
|
||||
event.event_id,
|
||||
event.signatures,
|
||||
)
|
||||
|
||||
if get_domain_from_id(event.sender) != origin:
|
||||
logger.info(
|
||||
"Got /send_knock request for user %r from different origin %s",
|
||||
"Got send_membership request for user %r from different origin %s",
|
||||
event.sender,
|
||||
origin,
|
||||
)
|
||||
raise SynapseError(403, "User not from origin", Codes.FORBIDDEN)
|
||||
|
||||
if event.sender != event.state_key:
|
||||
raise SynapseError(400, "state_key and sender must match", Codes.BAD_JSON)
|
||||
|
||||
event.internal_metadata.outlier = False
|
||||
|
||||
# Send this event on behalf of the other server.
|
||||
|
|
@ -2100,19 +1992,52 @@ class FederationHandler(BaseHandler):
|
|||
|
||||
context = await self.state_handler.compute_event_context(event)
|
||||
|
||||
event_allowed = await self.third_party_event_rules.check_event_allowed(
|
||||
event, context
|
||||
)
|
||||
if not event_allowed:
|
||||
logger.info("Sending of knock %s forbidden by third-party rules", event)
|
||||
raise SynapseError(
|
||||
403, "This event is not allowed in this context", Codes.FORBIDDEN
|
||||
# for joins, we need to check the restrictions of restricted rooms
|
||||
if event.membership == Membership.JOIN:
|
||||
await self._check_join_restrictions(context, event)
|
||||
|
||||
# for knock events, we run the third-party event rules. It's not entirely clear
|
||||
# why we don't do this for other sorts of membership events.
|
||||
if event.membership == Membership.KNOCK:
|
||||
event_allowed = await self.third_party_event_rules.check_event_allowed(
|
||||
event, context
|
||||
)
|
||||
if not event_allowed:
|
||||
logger.info("Sending of knock %s forbidden by third-party rules", event)
|
||||
raise SynapseError(
|
||||
403, "This event is not allowed in this context", Codes.FORBIDDEN
|
||||
)
|
||||
|
||||
await self._auth_and_persist_event(origin, event, context)
|
||||
|
||||
return context
|
||||
|
||||
async def _check_join_restrictions(
|
||||
self, context: EventContext, event: EventBase
|
||||
) -> None:
|
||||
"""Check that restrictions in restricted join rules are matched
|
||||
|
||||
Called when we receive a join event via send_join.
|
||||
|
||||
Raises an auth error if the restrictions are not matched.
|
||||
"""
|
||||
prev_state_ids = await context.get_prev_state_ids()
|
||||
|
||||
# Check if the user is already in the room or invited to the room.
|
||||
user_id = event.state_key
|
||||
prev_member_event_id = prev_state_ids.get((EventTypes.Member, user_id), None)
|
||||
prev_member_event = None
|
||||
if prev_member_event_id:
|
||||
prev_member_event = await self.store.get_event(prev_member_event_id)
|
||||
|
||||
# Check if the member should be allowed access via membership in a space.
|
||||
await self._event_auth_handler.check_restricted_join_rules(
|
||||
prev_state_ids,
|
||||
event.room_version,
|
||||
user_id,
|
||||
prev_member_event,
|
||||
)
|
||||
|
||||
async def get_state_for_pdu(self, room_id: str, event_id: str) -> List[EventBase]:
|
||||
"""Returns the state at the event. i.e. not including said event."""
|
||||
|
||||
|
|
|
|||
|
|
@ -251,7 +251,7 @@ class FederationTestCase(unittest.HomeserverTestCase):
|
|||
join_event.signatures[other_server] = {"x": "y"}
|
||||
with LoggingContext("send_join"):
|
||||
d = run_in_background(
|
||||
self.handler.on_send_join_request, other_server, join_event
|
||||
self.handler.on_send_membership_event, other_server, join_event
|
||||
)
|
||||
self.get_success(d)
|
||||
|
||||
|
|
|
|||
|
|
@ -228,7 +228,7 @@ class FederationSenderTestCase(BaseMultiWorkerStreamTestCase):
|
|||
builder.build(prev_event_ids=prev_event_ids, auth_event_ids=None)
|
||||
)
|
||||
|
||||
self.get_success(federation.on_send_join_request(remote_server, join_event))
|
||||
self.get_success(federation.on_send_membership_event(remote_server, join_event))
|
||||
self.replicate()
|
||||
|
||||
return room
|
||||
|
|
|
|||
Loading…
Reference in New Issue