Fix inconsistencies in event validation (#13088)

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