Check *all* auth events for room id and rejection (#11009)

This fixes a bug where we would accept an event whose `auth_events` include
rejected events, if the rejected event was shadowed by another `auth_event`
with same `(type, state_key)`.

The approach is to pass a list of auth events into
`check_auth_rules_for_event` instead of a dict, which of course means updating
the call sites.

This is an extension of #10956.
This commit is contained in:
Richard van der Hoff 2021-10-18 19:28:30 +02:00 committed by GitHub
parent 73743b8ad1
commit a5d2ea3d08
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 122 additions and 85 deletions

1
changelog.d/11009.bugfix Normal file
View file

@ -0,0 +1 @@
Fix a long-standing bug which meant that events received over federation were sometimes incorrectly accepted into the room state.

View file

@ -14,7 +14,7 @@
# limitations under the License. # limitations under the License.
import logging import logging
from typing import Any, Dict, List, Optional, Set, Tuple, Union from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union
from canonicaljson import encode_canonical_json from canonicaljson import encode_canonical_json
from signedjson.key import decode_verify_key_bytes from signedjson.key import decode_verify_key_bytes
@ -113,7 +113,7 @@ def validate_event_for_room_version(
def check_auth_rules_for_event( def check_auth_rules_for_event(
room_version_obj: RoomVersion, event: EventBase, auth_events: StateMap[EventBase] room_version_obj: RoomVersion, event: EventBase, auth_events: Iterable[EventBase]
) -> None: ) -> None:
"""Check that an event complies with the auth rules """Check that an event complies with the auth rules
@ -137,8 +137,6 @@ def check_auth_rules_for_event(
Raises: Raises:
AuthError if the checks fail AuthError if the checks fail
""" """
assert isinstance(auth_events, dict)
# We need to ensure that the auth events are actually for the same room, to # We need to ensure that the auth events are actually for the same room, to
# stop people from using powers they've been granted in other rooms for # stop people from using powers they've been granted in other rooms for
# example. # example.
@ -147,7 +145,7 @@ def check_auth_rules_for_event(
# the state res algorithm isn't silly enough to give us events from different rooms. # the state res algorithm isn't silly enough to give us events from different rooms.
# Still, it's easier to do it anyway. # Still, it's easier to do it anyway.
room_id = event.room_id room_id = event.room_id
for auth_event in auth_events.values(): for auth_event in auth_events:
if auth_event.room_id != room_id: if auth_event.room_id != room_id:
raise AuthError( raise AuthError(
403, 403,
@ -186,8 +184,10 @@ def check_auth_rules_for_event(
logger.debug("Allowing! %s", event) logger.debug("Allowing! %s", event)
return return
auth_dict = {(e.type, e.state_key): e for e in auth_events}
# 3. If event does not have a m.room.create in its auth_events, reject. # 3. If event does not have a m.room.create in its auth_events, reject.
creation_event = auth_events.get((EventTypes.Create, ""), None) creation_event = auth_dict.get((EventTypes.Create, ""), None)
if not creation_event: if not creation_event:
raise AuthError(403, "No create event in auth events") raise AuthError(403, "No create event in auth events")
@ -195,7 +195,7 @@ def check_auth_rules_for_event(
creating_domain = get_domain_from_id(event.room_id) creating_domain = get_domain_from_id(event.room_id)
originating_domain = get_domain_from_id(event.sender) originating_domain = get_domain_from_id(event.sender)
if creating_domain != originating_domain: if creating_domain != originating_domain:
if not _can_federate(event, auth_events): if not _can_federate(event, auth_dict):
raise AuthError(403, "This room has been marked as unfederatable.") raise AuthError(403, "This room has been marked as unfederatable.")
# 4. If type is m.room.aliases # 4. If type is m.room.aliases
@ -217,23 +217,20 @@ def check_auth_rules_for_event(
logger.debug("Allowing! %s", event) logger.debug("Allowing! %s", event)
return return
if logger.isEnabledFor(logging.DEBUG):
logger.debug("Auth events: %s", [a.event_id for a in auth_events.values()])
# 5. If type is m.room.membership # 5. If type is m.room.membership
if event.type == EventTypes.Member: if event.type == EventTypes.Member:
_is_membership_change_allowed(room_version_obj, event, auth_events) _is_membership_change_allowed(room_version_obj, event, auth_dict)
logger.debug("Allowing! %s", event) logger.debug("Allowing! %s", event)
return return
_check_event_sender_in_room(event, auth_events) _check_event_sender_in_room(event, auth_dict)
# Special case to allow m.room.third_party_invite events wherever # Special case to allow m.room.third_party_invite events wherever
# a user is allowed to issue invites. Fixes # a user is allowed to issue invites. Fixes
# https://github.com/vector-im/vector-web/issues/1208 hopefully # https://github.com/vector-im/vector-web/issues/1208 hopefully
if event.type == EventTypes.ThirdPartyInvite: if event.type == EventTypes.ThirdPartyInvite:
user_level = get_user_power_level(event.user_id, auth_events) user_level = get_user_power_level(event.user_id, auth_dict)
invite_level = get_named_level(auth_events, "invite", 0) invite_level = get_named_level(auth_dict, "invite", 0)
if user_level < invite_level: if user_level < invite_level:
raise AuthError(403, "You don't have permission to invite users") raise AuthError(403, "You don't have permission to invite users")
@ -241,20 +238,20 @@ def check_auth_rules_for_event(
logger.debug("Allowing! %s", event) logger.debug("Allowing! %s", event)
return return
_can_send_event(event, auth_events) _can_send_event(event, auth_dict)
if event.type == EventTypes.PowerLevels: if event.type == EventTypes.PowerLevels:
_check_power_levels(room_version_obj, event, auth_events) _check_power_levels(room_version_obj, event, auth_dict)
if event.type == EventTypes.Redaction: if event.type == EventTypes.Redaction:
check_redaction(room_version_obj, event, auth_events) check_redaction(room_version_obj, event, auth_dict)
if ( if (
event.type == EventTypes.MSC2716_INSERTION event.type == EventTypes.MSC2716_INSERTION
or event.type == EventTypes.MSC2716_BATCH or event.type == EventTypes.MSC2716_BATCH
or event.type == EventTypes.MSC2716_MARKER or event.type == EventTypes.MSC2716_MARKER
): ):
check_historical(room_version_obj, event, auth_events) check_historical(room_version_obj, event, auth_dict)
logger.debug("Allowing! %s", event) logger.debug("Allowing! %s", event)

View file

@ -55,8 +55,7 @@ class EventAuthHandler:
"""Check an event passes the auth rules at its own auth events""" """Check an event passes the auth rules at its own auth events"""
auth_event_ids = event.auth_event_ids() auth_event_ids = event.auth_event_ids()
auth_events_by_id = await self._store.get_events(auth_event_ids) auth_events_by_id = await self._store.get_events(auth_event_ids)
auth_events = {(e.type, e.state_key): e for e in auth_events_by_id.values()} check_auth_rules_for_event(room_version_obj, event, auth_events_by_id.values())
check_auth_rules_for_event(room_version_obj, event, auth_events)
def compute_auth_events( def compute_auth_events(
self, self,

View file

@ -1167,13 +1167,11 @@ class FederationHandler:
logger.info("Failed to find auth event %r", e_id) logger.info("Failed to find auth event %r", e_id)
for e in itertools.chain(auth_events, state, [event]): for e in itertools.chain(auth_events, state, [event]):
auth_for_e = { auth_for_e = [
(event_map[e_id].type, event_map[e_id].state_key): event_map[e_id] event_map[e_id] for e_id in e.auth_event_ids() if e_id in event_map
for e_id in e.auth_event_ids() ]
if e_id in event_map
}
if create_event: if create_event:
auth_for_e[(EventTypes.Create, "")] = create_event auth_for_e.append(create_event)
try: try:
validate_event_for_room_version(room_version, e) validate_event_for_room_version(room_version, e)

View file

@ -1203,7 +1203,7 @@ class FederationEventHandler:
def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]:
with nested_logging_context(suffix=event.event_id): with nested_logging_context(suffix=event.event_id):
auth = {} auth = []
for auth_event_id in event.auth_event_ids(): for auth_event_id in event.auth_event_ids():
ae = persisted_events.get(auth_event_id) ae = persisted_events.get(auth_event_id)
if not ae: if not ae:
@ -1216,7 +1216,7 @@ class FederationEventHandler:
# exist, which means it is premature to reject `event`. Instead we # exist, which means it is premature to reject `event`. Instead we
# just ignore it for now. # just ignore it for now.
return None return None
auth[(ae.type, ae.state_key)] = ae auth.append(ae)
context = EventContext.for_outlier() context = EventContext.for_outlier()
try: try:
@ -1305,7 +1305,9 @@ class FederationEventHandler:
auth_events_for_auth = calculated_auth_event_map auth_events_for_auth = calculated_auth_event_map
try: try:
check_auth_rules_for_event(room_version_obj, event, auth_events_for_auth) check_auth_rules_for_event(
room_version_obj, event, auth_events_for_auth.values()
)
except AuthError as e: except AuthError as e:
logger.warning("Failed auth resolution for %r because %s", event, e) logger.warning("Failed auth resolution for %r because %s", event, e)
context.rejected = RejectedReason.AUTH_ERROR context.rejected = RejectedReason.AUTH_ERROR
@ -1403,11 +1405,9 @@ class FederationEventHandler:
current_state_ids_list = [ current_state_ids_list = [
e for k, e in current_state_ids.items() if k in auth_types e for k, e in current_state_ids.items() if k in auth_types
] ]
current_auth_events = await self._store.get_events_as_list(
auth_events_map = await self._store.get_events(current_state_ids_list) current_state_ids_list
current_auth_events = { )
(e.type, e.state_key): e for e in auth_events_map.values()
}
try: try:
check_auth_rules_for_event(room_version_obj, event, current_auth_events) check_auth_rules_for_event(room_version_obj, event, current_auth_events)

View file

@ -332,7 +332,7 @@ def _resolve_auth_events(
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V1, RoomVersions.V1,
event, event,
auth_events, auth_events.values(),
) )
prev_event = event prev_event = event
except AuthError: except AuthError:
@ -350,7 +350,7 @@ def _resolve_normal_events(
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V1, RoomVersions.V1,
event, event,
auth_events, auth_events.values(),
) )
return event return event
except AuthError: except AuthError:

View file

@ -549,7 +549,7 @@ async def _iterative_auth_checks(
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
room_version, room_version,
event, event,
auth_events, auth_events.values(),
) )
resolved_state[(event.type, event.state_key)] = event_id resolved_state[(event.type, event.state_key)] = event_id

View file

@ -24,6 +24,47 @@ from synapse.types import JsonDict, get_domain_from_id
class EventAuthTestCase(unittest.TestCase): class EventAuthTestCase(unittest.TestCase):
def test_rejected_auth_events(self):
"""
Events that refer to rejected events in their auth events are rejected
"""
creator = "@creator:example.com"
auth_events = [
_create_event(creator),
_join_event(creator),
]
# creator should be able to send state
event_auth.check_auth_rules_for_event(
RoomVersions.V9,
_random_state_event(creator),
auth_events,
)
# ... but a rejected join_rules event should cause it to be rejected
rejected_join_rules = _join_rules_event(creator, "public")
rejected_join_rules.rejected_reason = "stinky"
auth_events.append(rejected_join_rules)
self.assertRaises(
AuthError,
event_auth.check_auth_rules_for_event,
RoomVersions.V9,
_random_state_event(creator),
auth_events,
)
# ... even if there is *also* a good join rules
auth_events.append(_join_rules_event(creator, "public"))
self.assertRaises(
AuthError,
event_auth.check_auth_rules_for_event,
RoomVersions.V9,
_random_state_event(creator),
auth_events,
)
def test_random_users_cannot_send_state_before_first_pl(self): def test_random_users_cannot_send_state_before_first_pl(self):
""" """
Check that, before the first PL lands, the creator is the only user Check that, before the first PL lands, the creator is the only user
@ -31,11 +72,11 @@ class EventAuthTestCase(unittest.TestCase):
""" """
creator = "@creator:example.com" creator = "@creator:example.com"
joiner = "@joiner:example.com" joiner = "@joiner:example.com"
auth_events = { auth_events = [
("m.room.create", ""): _create_event(creator), _create_event(creator),
("m.room.member", creator): _join_event(creator), _join_event(creator),
("m.room.member", joiner): _join_event(joiner), _join_event(joiner),
} ]
# creator should be able to send state # creator should be able to send state
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
@ -62,15 +103,15 @@ class EventAuthTestCase(unittest.TestCase):
pleb = "@joiner:example.com" pleb = "@joiner:example.com"
king = "@joiner2:example.com" king = "@joiner2:example.com"
auth_events = { auth_events = [
("m.room.create", ""): _create_event(creator), _create_event(creator),
("m.room.member", creator): _join_event(creator), _join_event(creator),
("m.room.power_levels", ""): _power_levels_event( _power_levels_event(
creator, {"state_default": "30", "users": {pleb: "29", king: "30"}} creator, {"state_default": "30", "users": {pleb: "29", king: "30"}}
), ),
("m.room.member", pleb): _join_event(pleb), _join_event(pleb),
("m.room.member", king): _join_event(king), _join_event(king),
} ]
# pleb should not be able to send state # pleb should not be able to send state
self.assertRaises( self.assertRaises(
@ -92,10 +133,10 @@ class EventAuthTestCase(unittest.TestCase):
"""Alias events have special behavior up through room version 6.""" """Alias events have special behavior up through room version 6."""
creator = "@creator:example.com" creator = "@creator:example.com"
other = "@other:example.com" other = "@other:example.com"
auth_events = { auth_events = [
("m.room.create", ""): _create_event(creator), _create_event(creator),
("m.room.member", creator): _join_event(creator), _join_event(creator),
} ]
# creator should be able to send aliases # creator should be able to send aliases
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
@ -131,10 +172,10 @@ class EventAuthTestCase(unittest.TestCase):
"""After MSC2432, alias events have no special behavior.""" """After MSC2432, alias events have no special behavior."""
creator = "@creator:example.com" creator = "@creator:example.com"
other = "@other:example.com" other = "@other:example.com"
auth_events = { auth_events = [
("m.room.create", ""): _create_event(creator), _create_event(creator),
("m.room.member", creator): _join_event(creator), _join_event(creator),
} ]
# creator should be able to send aliases # creator should be able to send aliases
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
@ -170,14 +211,14 @@ class EventAuthTestCase(unittest.TestCase):
creator = "@creator:example.com" creator = "@creator:example.com"
pleb = "@joiner:example.com" pleb = "@joiner:example.com"
auth_events = { auth_events = [
("m.room.create", ""): _create_event(creator), _create_event(creator),
("m.room.member", creator): _join_event(creator), _join_event(creator),
("m.room.power_levels", ""): _power_levels_event( _power_levels_event(
creator, {"state_default": "30", "users": {pleb: "30"}} creator, {"state_default": "30", "users": {pleb: "30"}}
), ),
("m.room.member", pleb): _join_event(pleb), _join_event(pleb),
} ]
# pleb should be able to modify the notifications power level. # pleb should be able to modify the notifications power level.
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
@ -211,7 +252,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user cannot be force-joined to a room. # A user cannot be force-joined to a room.
@ -219,7 +260,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_member_event(pleb, "join", sender=creator), _member_event(pleb, "join", sender=creator),
auth_events, auth_events.values(),
) )
# Banned should be rejected. # Banned should be rejected.
@ -228,7 +269,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user who left can re-join. # A user who left can re-join.
@ -236,7 +277,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user can send a join if they're in the room. # A user can send a join if they're in the room.
@ -244,7 +285,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user can accept an invite. # A user can accept an invite.
@ -254,7 +295,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
def test_join_rules_invite(self): def test_join_rules_invite(self):
@ -275,7 +316,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user cannot be force-joined to a room. # A user cannot be force-joined to a room.
@ -283,7 +324,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_member_event(pleb, "join", sender=creator), _member_event(pleb, "join", sender=creator),
auth_events, auth_events.values(),
) )
# Banned should be rejected. # Banned should be rejected.
@ -292,7 +333,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user who left cannot re-join. # A user who left cannot re-join.
@ -301,7 +342,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user can send a join if they're in the room. # A user can send a join if they're in the room.
@ -309,7 +350,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user can accept an invite. # A user can accept an invite.
@ -319,7 +360,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
def test_join_rules_msc3083_restricted(self): def test_join_rules_msc3083_restricted(self):
@ -347,7 +388,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V6, RoomVersions.V6,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A properly formatted join event should work. # A properly formatted join event should work.
@ -360,7 +401,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
authorised_join_event, authorised_join_event,
auth_events, auth_events.values(),
) )
# A join issued by a specific user works (i.e. the power level checks # A join issued by a specific user works (i.e. the power level checks
@ -380,7 +421,7 @@ class EventAuthTestCase(unittest.TestCase):
EventContentFields.AUTHORISING_USER: "@inviter:foo.test" EventContentFields.AUTHORISING_USER: "@inviter:foo.test"
}, },
), ),
pl_auth_events, pl_auth_events.values(),
) )
# A join which is missing an authorised server is rejected. # A join which is missing an authorised server is rejected.
@ -388,7 +429,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# An join authorised by a user who is not in the room is rejected. # An join authorised by a user who is not in the room is rejected.
@ -405,7 +446,7 @@ class EventAuthTestCase(unittest.TestCase):
EventContentFields.AUTHORISING_USER: "@other:example.com" EventContentFields.AUTHORISING_USER: "@other:example.com"
}, },
), ),
auth_events, auth_events.values(),
) )
# A user cannot be force-joined to a room. (This uses an event which # A user cannot be force-joined to a room. (This uses an event which
@ -421,7 +462,7 @@ class EventAuthTestCase(unittest.TestCase):
EventContentFields.AUTHORISING_USER: "@inviter:foo.test" EventContentFields.AUTHORISING_USER: "@inviter:foo.test"
}, },
), ),
auth_events, auth_events.values(),
) )
# Banned should be rejected. # Banned should be rejected.
@ -430,7 +471,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
authorised_join_event, authorised_join_event,
auth_events, auth_events.values(),
) )
# A user who left can re-join. # A user who left can re-join.
@ -438,7 +479,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
authorised_join_event, authorised_join_event,
auth_events, auth_events.values(),
) )
# A user can send a join if they're in the room. (This doesn't need to # A user can send a join if they're in the room. (This doesn't need to
@ -447,7 +488,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
# A user can accept an invite. (This doesn't need to be authorised since # A user can accept an invite. (This doesn't need to be authorised since
@ -458,7 +499,7 @@ class EventAuthTestCase(unittest.TestCase):
event_auth.check_auth_rules_for_event( event_auth.check_auth_rules_for_event(
RoomVersions.V8, RoomVersions.V8,
_join_event(pleb), _join_event(pleb),
auth_events, auth_events.values(),
) )
@ -473,6 +514,7 @@ def _create_event(user_id: str) -> EventBase:
"room_id": TEST_ROOM_ID, "room_id": TEST_ROOM_ID,
"event_id": _get_event_id(), "event_id": _get_event_id(),
"type": "m.room.create", "type": "m.room.create",
"state_key": "",
"sender": user_id, "sender": user_id,
"content": {"creator": user_id}, "content": {"creator": user_id},
} }