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

Check auth on received events' auth_events #11001

Merged
merged 6 commits into from
Oct 18, 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/11001.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug which meant that events received over federation were sometimes incorrectly accepted into the room state.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm scared of this breaking more MSC2716 stuff (related #10764)

Copy link
Member Author

Choose a reason for hiding this comment

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

@MadLittleMods I hope it won't :/

Can you explain your concern in more detail? I don't think this is directly related to #10764, which as I read it is about whether remote servers can provide the prev_events for a given event, rather than the auth_events.

The protocol in general, and Synapse in particular, already assume quite heavily that you must have copies of the auth_events for any event you end up serving over federation. This PR fixes some edge-cases where we weren't properly checking it, but doesn't change the fundamental philosophy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a bit squeamish about making sure the remote homeservers can still accept our imported historical events. It might be fine, just sounds kinda related to something that could make it harder. Our auth_events on historical events are real (some outliers) so this probably works. We only use fake prev_events.

The other issue was just related in terms of other breaking changes.

1 change: 1 addition & 0 deletions changelog.d/11009.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug which meant that events received over federation were sometimes incorrectly accepted into the room state.
33 changes: 15 additions & 18 deletions synapse/event_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
# limitations under the License.

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 signedjson.key import decode_verify_key_bytes
Expand Down Expand Up @@ -113,7 +113,7 @@ def validate_event_for_room_version(


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:
"""Check that an event complies with the auth rules

Expand All @@ -137,8 +137,6 @@ def check_auth_rules_for_event(
Raises:
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
# stop people from using powers they've been granted in other rooms for
# example.
Expand All @@ -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.
# Still, it's easier to do it anyway.
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:
raise AuthError(
403,
Expand Down Expand Up @@ -186,16 +184,18 @@ def check_auth_rules_for_event(
logger.debug("Allowing! %s", event)
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.
creation_event = auth_events.get((EventTypes.Create, ""), None)
creation_event = auth_dict.get((EventTypes.Create, ""), None)
if not creation_event:
raise AuthError(403, "No create event in auth events")

# additional check for m.federate
creating_domain = get_domain_from_id(event.room_id)
originating_domain = get_domain_from_id(event.sender)
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.")

# 4. If type is m.room.aliases
Expand All @@ -217,44 +217,41 @@ def check_auth_rules_for_event(
logger.debug("Allowing! %s", event)
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
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)
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
# a user is allowed to issue invites. Fixes
# https://github.com/vector-im/vector-web/issues/1208 hopefully
if event.type == EventTypes.ThirdPartyInvite:
user_level = get_user_power_level(event.user_id, auth_events)
invite_level = get_named_level(auth_events, "invite", 0)
user_level = get_user_power_level(event.user_id, auth_dict)
invite_level = get_named_level(auth_dict, "invite", 0)

if user_level < invite_level:
raise AuthError(403, "You don't have permission to invite users")
else:
logger.debug("Allowing! %s", event)
return

_can_send_event(event, auth_events)
_can_send_event(event, auth_dict)

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:
check_redaction(room_version_obj, event, auth_events)
check_redaction(room_version_obj, event, auth_dict)

if (
event.type == EventTypes.MSC2716_INSERTION
or event.type == EventTypes.MSC2716_BATCH
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)

Expand Down
3 changes: 1 addition & 2 deletions synapse/handlers/event_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ async def check_auth_rules_from_context(
"""Check an event passes the auth rules at its own auth events"""
auth_event_ids = event.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)
check_auth_rules_for_event(room_version_obj, event, auth_events_by_id.values())

def compute_auth_events(
self,
Expand Down
10 changes: 4 additions & 6 deletions synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1167,13 +1167,11 @@ async def _persist_auth_tree(
logger.info("Failed to find auth event %r", e_id)

for e in itertools.chain(auth_events, state, [event]):
auth_for_e = {
(event_map[e_id].type, event_map[e_id].state_key): event_map[e_id]
for e_id in e.auth_event_ids()
if e_id in event_map
}
auth_for_e = [
event_map[e_id] for e_id in e.auth_event_ids() if e_id in event_map
]
if create_event:
auth_for_e[(EventTypes.Create, "")] = create_event
auth_for_e.append(create_event)

try:
validate_event_for_room_version(room_version, e)
Expand Down
115 changes: 105 additions & 10 deletions synapse/handlers/federation_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -1197,7 +1197,7 @@ async def _auth_and_persist_fetched_events_inner(

def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]:
with nested_logging_context(suffix=event.event_id):
auth = {}
auth = []
for auth_event_id in event.auth_event_ids():
ae = persisted_events.get(auth_event_id)
if not ae:
Expand All @@ -1210,7 +1210,7 @@ def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]:
# exist, which means it is premature to reject `event`. Instead we
# just ignore it for now.
return None
auth[(ae.type, ae.state_key)] = ae
auth.append(ae)

context = EventContext.for_outlier()
try:
Expand Down Expand Up @@ -1250,6 +1250,10 @@ async def _check_event_auth(

Returns:
The updated context object.

Raises:
AuthError if we were unable to find copies of the event's auth events.
(Most other failures just cause us to set `context.rejected`.)
"""
# This method should only be used for non-outliers
assert not event.internal_metadata.outlier
Expand All @@ -1266,7 +1270,26 @@ async def _check_event_auth(
context.rejected = RejectedReason.AUTH_ERROR
return context

# calculate what the auth events *should* be, to use as a basis for auth.
# next, check that we have all of the event's auth events.
#
# Note that this can raise AuthError, which we want to propagate to the
# caller rather than swallow with `context.rejected` (since we cannot be
# certain that there is a permanent problem with the event).
claimed_auth_events = await self._load_or_fetch_auth_events_for_event(
origin, event
)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we only doing this for non-outliers? We want to have the full auth chain for outlier events too?

Copy link
Member Author

Choose a reason for hiding this comment

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

we already do it for outlier events. They go through a completely different code path (_auth_and_persist_fetched_events which does the same sort of thing as this in _auth_and_persist_fetched_events_inner).


# ... and check that the event passes auth at those auth events.
try:
check_auth_rules_for_event(room_version_obj, event, claimed_auth_events)
except AuthError as e:
logger.warning(
"While checking auth of %r against auth_events: %s", event, e
)
context.rejected = RejectedReason.AUTH_ERROR
return context

# now check auth against what we think the auth events *should* be.
prev_state_ids = await context.get_prev_state_ids()
auth_events_ids = self._event_auth_handler.compute_auth_events(
event, prev_state_ids, for_verification=True
Expand Down Expand Up @@ -1299,7 +1322,9 @@ async def _check_event_auth(
auth_events_for_auth = calculated_auth_event_map

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:
logger.warning("Failed auth resolution for %r because %s", event, e)
context.rejected = RejectedReason.AUTH_ERROR
Expand Down Expand Up @@ -1397,11 +1422,9 @@ async def _check_for_soft_fail(
current_state_ids_list = [
e for k, e in current_state_ids.items() if k in auth_types
]

auth_events_map = await self._store.get_events(current_state_ids_list)
current_auth_events = {
(e.type, e.state_key): e for e in auth_events_map.values()
}
current_auth_events = await self._store.get_events_as_list(
current_state_ids_list
)

try:
check_auth_rules_for_event(room_version_obj, event, current_auth_events)
Expand Down Expand Up @@ -1466,6 +1489,9 @@ async def _update_auth_events_and_context_for_auth(
# if we have missing events, we need to fetch those events from somewhere.
#
# we start by checking if they are in the store, and then try calling /event_auth/.
#
# TODO: this code is now redundant, since it should be impossible for us to
# get here without already having the auth events.
Comment on lines +1493 to +1494
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to clean this up in another PR.

if missing_auth:
have_events = await self._store.have_seen_events(
event.room_id, missing_auth
Expand Down Expand Up @@ -1569,7 +1595,7 @@ async def _update_auth_events_and_context_for_auth(
logger.info(
"After state res: updating auth_events with new state %s",
{
(d.type, d.state_key): d.event_id
d
for d in new_state.values()
if auth_events.get((d.type, d.state_key)) != d
},
Expand All @@ -1583,6 +1609,75 @@ async def _update_auth_events_and_context_for_auth(

return context, auth_events

async def _load_or_fetch_auth_events_for_event(
self, destination: str, event: EventBase
) -> Collection[EventBase]:
"""Fetch this event's auth_events, from database or remote

Loads any of the auth_events that we already have from the database/cache. If
there are any that are missing, calls /event_auth to get the complete auth
chain for the event (and then attempts to load the auth_events again).

If any of the auth_events cannot be found, raises an AuthError. This can happen
for a number of reasons; eg: the events don't exist, or we were unable to talk
to `destination`, or we couldn't validate the signature on the event (which
in turn has multiple potential causes).

Args:
destination: where to send the /event_auth request. Typically the server
that sent us `event` in the first place.
event: the event whose auth_events we want

Returns:
all of the events in `event.auth_events`, after deduplication

Raises:
AuthError if we were unable to fetch the auth_events for any reason.
"""
event_auth_event_ids = set(event.auth_event_ids())
event_auth_events = await self._store.get_events(
event_auth_event_ids, allow_rejected=True
)
missing_auth_event_ids = event_auth_event_ids.difference(
event_auth_events.keys()
)
if not missing_auth_event_ids:
return event_auth_events.values()

logger.info(
"Event %s refers to unknown auth events %s: fetching auth chain",
event,
missing_auth_event_ids,
)
try:
await self._get_remote_auth_chain_for_event(
Copy link
Member

Choose a reason for hiding this comment

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

Getting the entire auth chain again is fairly heavyweight. I sort of wonder if we should only fetch the missing auth events, falling back to /event_auth if we're still missing auth events? 🤷 Probably not something for this PR though

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, probably, though as you say, I think it's a change for another day.

destination, event.room_id, event.event_id
)
Comment on lines +1652 to +1655
Copy link
Member Author

Choose a reason for hiding this comment

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

the use of _get_remote_auth_chain_for_event is copied from _update_auth_events_and_context_for_auth, which is called a little later in the flow and does the same thing under the same circumstances.

except Exception as e:
logger.warning("Failed to get auth chain for %s: %s", event, e)
# in this case, it's very likely we still won't have all the auth
# events - but we pick that up below.

# try to fetch the auth events we missed list time.
extra_auth_events = await self._store.get_events(
missing_auth_event_ids, allow_rejected=True
)
missing_auth_event_ids.difference_update(extra_auth_events.keys())
event_auth_events.update(extra_auth_events)
if not missing_auth_event_ids:
return event_auth_events.values()

# we still don't have all the auth events.
logger.warning(
"Missing auth events for %s: %s",
event,
shortstr(missing_auth_event_ids),
)
# the fact we can't find the auth event doesn't mean it doesn't
# exist, which means it is premature to store `event` as rejected.
# instead we raise an AuthError, which will make the caller ignore it.
raise AuthError(code=HTTPStatus.FORBIDDEN, msg="Auth events could not be found")

async def _get_remote_auth_chain_for_event(
self, destination: str, room_id: str, event_id: str
) -> None:
Expand Down
4 changes: 2 additions & 2 deletions synapse/state/v1.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ def _resolve_auth_events(
event_auth.check_auth_rules_for_event(
RoomVersions.V1,
event,
auth_events,
auth_events.values(),
)
prev_event = event
except AuthError:
Expand All @@ -350,7 +350,7 @@ def _resolve_normal_events(
event_auth.check_auth_rules_for_event(
RoomVersions.V1,
event,
auth_events,
auth_events.values(),
)
return event
except AuthError:
Expand Down
2 changes: 1 addition & 1 deletion synapse/state/v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ async def _iterative_auth_checks(
event_auth.check_auth_rules_for_event(
room_version,
event,
auth_events,
auth_events.values(),
)

resolved_state[(event.type, event.state_key)] = event_id
Expand Down
Loading