Check the room_id of events when fetching room state/auth #6524
Conversation
When we request the state/auth_events to populate a backwards extremity (on backfill or in the case of missing events in a transaction push), we should check that the returned events are in the right room rather than blindly using them in the room state or auth chain. Given that _get_events_from_store_or_dest takes a room_id, it seems clear that it should be sanity-checking the room_id of the requested events, so let's do it there.
| # this can happen if a remote server claims that the state or | ||
| # auth_events at an event in room A are actually events in room B | ||
|
|
||
| bad_events = list( |
There was a problem hiding this comment.
| bad_events = list( | |
| bad_events = ( |
There was a problem hiding this comment.
no, I'm deliberately building a list here rather than using a generator that refers to fetched_events: otherwise we're going to get errors about modifying fetched_events while iterating.
There was a problem hiding this comment.
Wouldn't it be cleaner to have used a list comprehension then rather than calling list on a generator expression?
There was a problem hiding this comment.
you'll have to fight @hawkowl over that; I'm kinda ambivalent on the topic.
The reason to prefer this syntax (aiui) is for symmetry with constructing a tuple, where you have to invoke tuple explicitly to get a tuple rather than a generator.
In the case of creating a list: I'd imagine (I haven't checked) that the bytecode generated for list(x for x in foo) is the same as that for [x for x in foo] ?
There was a problem hiding this comment.
It is not - there is no way to guarantee that list refers to the list builtin, so it will always compile to a call to list.
There was a problem hiding this comment.
interesting. thanks for the info.
When we request the state/auth_events to populate a backwards extremity (on backfill or in the case of missing events in a transaction push), we should check that the returned events are in the right room rather than blindly using them in the room state or auth chain. Given that _get_events_from_store_or_dest takes a room_id, it seems clear that it should be sanity-checking the room_id of the requested events, so let's do it there.
Synapse 1.7.1 (2019-12-18) ========================== This release includes several security fixes as well as a fix to a bug exposed by the security fixes. Administrators are encouraged to upgrade as soon as possible. Security updates ---------------- - Fix a bug which could cause room events to be incorrectly authorized using events from a different room. ([\#6501](#6501), [\#6503](#6503), [\#6521](#6521), [\#6524](#6524), [\#6530](#6530), [\#6531](#6531)) - Fix a bug causing responses to the `/context` client endpoint to not use the pruned version of the event. ([\#6553](#6553)) - Fix a cause of state resets in room versions 2 onwards. ([\#6556](#6556), [\#6560](#6560)) Bugfixes -------- - Fix a bug which could cause the federation server to incorrectly return errors when handling certain obscure event graphs. ([\#6526](#6526), [\#6527](#6527))
* commit '25f124432': Check the room_id of events when fetching room state/auth (#6524)
* commit '35bbe4ca7': Check the room_id of events when fetching room state/auth (#6524)
This is a similar vein to #6472, though a different bit of the code.
When we request the state/auth_events to populate a backwards extremity (on backfill or in the case of missing events in a transaction push), we should check that the returned events are in the right room rather than blindly using them in the room state or auth chain.
Given that _get_events_from_store_or_dest takes a room_id, it seems clear that it should be sanity-checking the room_id of the requested events, so let's do it there.