Change display names/avatar URLs to None if they contain null bytes before storing in DB (#11230)

* change display names/avatar URLS to None if they contain null bytes

* add changelog

* add POC test, requested changes

* add a saner test and remove old one

* update test to verify that display name has been changed to None

* make test less fragile

Author: H-Shay

changelog.d/11230.bugfix

@@ -0,0 +1,2 @@
+Fix a long-standing bug wherein display names or avatar URLs containing null bytes cause an internal server error
+when stored in the DB.

synapse/storage/databases/main/events.py

@@ -1641,8 +1641,8 @@ def _store_event_reference_hashes_txn(self, txn, events):
     def _store_room_members_txn(self, txn, events, backfilled):
         """Store a room member in the database."""
 
-        def str_or_none(val: Any) -> Optional[str]:
-            return val if isinstance(val, str) else None
+        def non_null_str_or_none(val: Any) -> Optional[str]:
+            return val if isinstance(val, str) and "\u0000" not in val else None
 
         self.db_pool.simple_insert_many_txn(
             txn,
@@ -1654,8 +1654,10 @@ def str_or_none(val: Any) -> Optional[str]:
                     "sender": event.user_id,
                     "room_id": event.room_id,
                     "membership": event.membership,
-                    "display_name": str_or_none(event.content.get("displayname")),
-                    "avatar_url": str_or_none(event.content.get("avatar_url")),
+                    "display_name": non_null_str_or_none(
+                        event.content.get("displayname")
+                    ),
+                    "avatar_url": non_null_str_or_none(event.content.get("avatar_url")),
                 }
                 for event in events
             ],

tests/storage/test_roommember.py

@@ -161,6 +161,54 @@ def test_get_joined_users_from_context(self):
         )
         self.assertEqual(users.keys(), {self.u_alice, self.u_bob})
 
+    def test__null_byte_in_display_name_properly_handled(self):
+        room = self.helper.create_room_as(self.u_alice, tok=self.t_alice)
+
+        res = self.get_success(
+            self.store.db_pool.simple_select_list(
+                "room_memberships",
+                {"user_id": "@alice:test"},
+                ["display_name", "event_id"],
+            )
+        )
+        # Check that we only got one result back
+        self.assertEqual(len(res), 1)
+
+        # Check that alice's display name is "alice"
+        self.assertEqual(res[0]["display_name"], "alice")
+
+        # Grab the event_id to use later
+        event_id = res[0]["event_id"]
+
+        # Create a profile with the offending null byte in the display name
+        new_profile = {"displayname": "ali\u0000ce"}
+
+        # Ensure that the change goes smoothly and does not fail due to the null byte
+        self.helper.change_membership(
+            room,
+            self.u_alice,
+            self.u_alice,
+            "join",
+            extra_data=new_profile,
+            tok=self.t_alice,
+        )
+
+        res2 = self.get_success(
+            self.store.db_pool.simple_select_list(
+                "room_memberships",
+                {"user_id": "@alice:test"},
+                ["display_name", "event_id"],
+            )
+        )
+        # Check that we only have two results
+        self.assertEqual(len(res2), 2)
+
+        # Filter out the previous event using the event_id we grabbed above
+        row = [row for row in res2 if row["event_id"] != event_id]
+
+        # Check that alice's display name is now None
+        self.assertEqual(row[0]["display_name"], None)
+
 
 class CurrentStateMembershipUpdateTestCase(unittest.HomeserverTestCase):
     def prepare(self, reactor, clock, homeserver):