diff options
| -rw-r--r-- | changelog.d/13088.bugfix | 1 | ||||
| -rw-r--r-- | synapse/event_auth.py | 23 | ||||
| -rw-r--r-- | tests/handlers/test_federation.py | 14 | ||||
| -rw-r--r-- | tests/handlers/test_federation_event.py | 1 | ||||
| -rw-r--r-- | tests/test_event_auth.py | 86 |
5 files changed, 118 insertions, 7 deletions
diff --git a/changelog.d/13088.bugfix b/changelog.d/13088.bugfix new file mode 100644 index 0000000000..7c69801afe --- /dev/null +++ b/changelog.d/13088.bugfix @@ -0,0 +1 @@ +Fix some inconsistencies in the event authentication code. diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 440b1ae418..0fc2c4b27e 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -150,7 +150,7 @@ async def check_state_independent_auth_rules( # 1.5 Otherwise, allow return - # Check the auth events. + # 2. Reject if event has auth_events that: ... auth_events = await store.get_events( event.auth_event_ids(), redact_behaviour=EventRedactBehaviour.as_is, @@ -158,6 +158,7 @@ async def check_state_independent_auth_rules( ) room_id = event.room_id auth_dict: MutableStateMap[str] = {} + expected_auth_types = auth_types_for_event(event.room_version, event) for auth_event_id in event.auth_event_ids(): auth_event = auth_events.get(auth_event_id) @@ -179,6 +180,24 @@ async def check_state_independent_auth_rules( % (event.event_id, room_id, auth_event_id, auth_event.room_id), ) + k = (auth_event.type, auth_event.state_key) + + # 2.1 ... have duplicate entries for a given type and state_key pair + if k in auth_dict: + raise AuthError( + 403, + f"Event {event.event_id} has duplicate auth_events for {k}: {auth_dict[k]} and {auth_event_id}", + ) + + # 2.2 ... have entries whose type and state_key don’t match those specified by + # the auth events selection algorithm described in the server + # specification. + if k not in expected_auth_types: + raise AuthError( + 403, + f"Event {event.event_id} has unexpected auth_event for {k}: {auth_event_id}", + ) + # We also need to check that the auth event itself is not rejected. if auth_event.rejected_reason: raise AuthError( @@ -187,7 +206,7 @@ async def check_state_independent_auth_rules( % (event.event_id, auth_event.event_id), ) - auth_dict[(auth_event.type, auth_event.state_key)] = auth_event_id + auth_dict[k] = auth_event_id # 3. If event does not have a m.room.create in its auth_events, reject. creation_event = auth_dict.get((EventTypes.Create, ""), None) diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 9afba7b0e8..9b9c11fab7 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -225,9 +225,10 @@ class FederationTestCase(unittest.FederatingHomeserverTestCase): # we need a user on the remote server to be a member, so that we can send # extremity-causing events. + remote_server_user_id = f"@user:{self.OTHER_SERVER_NAME}" self.get_success( event_injection.inject_member_event( - self.hs, room_id, f"@user:{self.OTHER_SERVER_NAME}", "join" + self.hs, room_id, remote_server_user_id, "join" ) ) @@ -247,6 +248,12 @@ class FederationTestCase(unittest.FederatingHomeserverTestCase): # create more than is 5 which corresponds to the number of backward # extremities we slice off in `_maybe_backfill_inner` federation_event_handler = self.hs.get_federation_event_handler() + auth_events = [ + ev + for ev in current_state + if (ev.type, ev.state_key) + in {("m.room.create", ""), ("m.room.member", remote_server_user_id)} + ] for _ in range(0, 8): event = make_event_from_dict( self.add_hashes_and_signatures( @@ -258,15 +265,14 @@ class FederationTestCase(unittest.FederatingHomeserverTestCase): "body": "message connected to fake event", }, "room_id": room_id, - "sender": f"@user:{self.OTHER_SERVER_NAME}", + "sender": remote_server_user_id, "prev_events": [ ev1.event_id, # We're creating an backward extremity each time thanks # to this fake event generate_fake_event_id(), ], - # lazy: *everything* is an auth event - "auth_events": [ev.event_id for ev in current_state], + "auth_events": [ev.event_id for ev in auth_events], "depth": ev1.depth + 1, }, room_version, diff --git a/tests/handlers/test_federation_event.py b/tests/handlers/test_federation_event.py index 1a36c25c41..4b1a8f04db 100644 --- a/tests/handlers/test_federation_event.py +++ b/tests/handlers/test_federation_event.py @@ -98,7 +98,6 @@ class FederationEventHandlerTests(unittest.FederatingHomeserverTestCase): auth_event_ids = [ initial_state_map[("m.room.create", "")], initial_state_map[("m.room.power_levels", "")], - initial_state_map[("m.room.join_rules", "")], member_event.event_id, ] diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index ed7a3cbcee..371cd201af 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -150,6 +150,92 @@ class EventAuthTestCase(unittest.TestCase): event_auth.check_state_independent_auth_rules(event_store, bad_event) ) + def test_duplicate_auth_events(self): + """Events with duplicate auth_events should be rejected + + https://spec.matrix.org/v1.3/rooms/v9/#authorization-rules + 2. Reject if event has auth_events that: + 1. have duplicate entries for a given type and state_key pair + """ + creator = "@creator:example.com" + + create_event = _create_event(RoomVersions.V9, creator) + join_event1 = _join_event(RoomVersions.V9, creator) + pl_event = _power_levels_event( + RoomVersions.V9, + creator, + {"state_default": 30, "users": {"creator": 100}}, + ) + + # create a second join event, so that we can make a duplicate + join_event2 = _join_event(RoomVersions.V9, creator) + + event_store = _StubEventSourceStore() + event_store.add_events([create_event, join_event1, join_event2, pl_event]) + + good_event = _random_state_event( + RoomVersions.V9, creator, [create_event, join_event2, pl_event] + ) + bad_event = _random_state_event( + RoomVersions.V9, creator, [create_event, join_event1, join_event2, pl_event] + ) + # a variation: two instances of the *same* event + bad_event2 = _random_state_event( + RoomVersions.V9, creator, [create_event, join_event2, join_event2, pl_event] + ) + + get_awaitable_result( + event_auth.check_state_independent_auth_rules(event_store, good_event) + ) + with self.assertRaises(AuthError): + get_awaitable_result( + event_auth.check_state_independent_auth_rules(event_store, bad_event) + ) + with self.assertRaises(AuthError): + get_awaitable_result( + event_auth.check_state_independent_auth_rules(event_store, bad_event2) + ) + + def test_unexpected_auth_events(self): + """Events with excess auth_events should be rejected + + https://spec.matrix.org/v1.3/rooms/v9/#authorization-rules + 2. Reject if event has auth_events that: + 2. have entries whose type and state_key don’t match those specified by the + auth events selection algorithm described in the server specification. + """ + creator = "@creator:example.com" + + create_event = _create_event(RoomVersions.V9, creator) + join_event = _join_event(RoomVersions.V9, creator) + pl_event = _power_levels_event( + RoomVersions.V9, + creator, + {"state_default": 30, "users": {"creator": 100}}, + ) + join_rules_event = _join_rules_event(RoomVersions.V9, creator, "public") + + event_store = _StubEventSourceStore() + event_store.add_events([create_event, join_event, pl_event, join_rules_event]) + + good_event = _random_state_event( + RoomVersions.V9, creator, [create_event, join_event, pl_event] + ) + # join rules should *not* be included in the auth events. + bad_event = _random_state_event( + RoomVersions.V9, + creator, + [create_event, join_event, pl_event, join_rules_event], + ) + + get_awaitable_result( + event_auth.check_state_independent_auth_rules(event_store, good_event) + ) + with self.assertRaises(AuthError): + get_awaitable_result( + event_auth.check_state_independent_auth_rules(event_store, bad_event) + ) + def test_random_users_cannot_send_state_before_first_pl(self): """ Check that, before the first PL lands, the creator is the only user |