Rework UI Auth session validation

changelog.d/7455.bugfix

@@ -0,0 +1 @@
+Ensure that a user inteactive authentication session is tied to a single request.

synapse/handlers/auth.py

@@ -252,6 +252,7 @@ async def check_auth(
         clientdict: Dict[str, Any],
         clientip: str,
         description: str,
+        validate_clientdict: bool = True,
     ) -> Tuple[dict, dict, str]:
         """
         Takes a dictionary sent by the client in the login / registration
@@ -277,6 +278,10 @@ async def check_auth(
             description: A human readable string to be displayed to the user that
                          describes the operation happening on their account.
 
+            validate_clientdict: Whether to validate that the operation happening
+                                 on the account has not changed. If this is false,
+                                 the client dict is persisted instead of validated.
+
         Returns:
             A tuple of (creds, params, session_id).
 
@@ -317,30 +322,51 @@ async def check_auth(
             except StoreError:
                 raise SynapseError(400, "Unknown session ID: %s" % (sid,))
 
+            # If the client provides parameters, update what is persisted,
+            # otherwise use whatever was last provided.
+            #
+            # This was designed to allow the client to omit the parameters
+            # and just supply the session in subsequent calls so it split
+            # auth between devices by just sharing the session, (eg. so you
+            # could continue registration from your phone having clicked the
+            # email auth link on there). It's probably too open to abuse
+            # because it lets unauthenticated clients store arbitrary objects
+            # on a homeserver.
+            #
+            # Revisit: Assuming the REST APIs do sensible validation, the data
+            # isn't arbitrary.
+            #
+            # Note that the registration endpoint explicitly removes the
+            # "initial_device_display_name" parameter if it is provided
+            # without a "password" parameter. See the changes to
+            # synapse.rest.client.v2_alpha.register.RegisterRestServlet.on_POST
+            # in commit 544722bad23fc31056b9240189c3cbbbf0ffd3f9.
             if not clientdict:
-                # This was designed to allow the client to omit the parameters
-                # and just supply the session in subsequent calls so it split
-                # auth between devices by just sharing the session, (eg. so you
-                # could continue registration from your phone having clicked the
-                # email auth link on there). It's probably too open to abuse
-                # because it lets unauthenticated clients store arbitrary objects
-                # on a homeserver.
-                # Revisit: Assuming the REST APIs do sensible validation, the data
-                # isn't arbitrary.
                 clientdict = session.clientdict
 
             # Ensure that the queried operation does not vary between stages of
             # the UI authentication session. This is done by generating a stable
-            # comparator based on the URI, method, and body (minus the auth dict)
-            # and storing it during the initial query. Subsequent queries ensure
-            # that this comparator has not changed.
-            comparator = (uri, method, clientdict)
-            if (session.uri, session.method, session.clientdict) != comparator:
+            # comparator based on the URI, method, and client dict (minus the
+            # auth dict) and storing it during the initial query. Subsequent
+            # queries ensure that this comparator has not changed.
+            if validate_clientdict:
+                session_comparator = (session.uri, session.method, session.clientdict)
+                comparator = (uri, method, clientdict)
+            else:
+                session_comparator = (session.uri, session.method)  # type: ignore
+                comparator = (uri, method)  # type: ignore
+
+            if session_comparator != comparator:
                 raise SynapseError(
                     403,
                     "Requested operation has changed during the UI authentication session.",
                 )
 
+            # For backwards compatibility the registration endpoint persists
+            # changes to the client dict instead of validating them.
+            if not validate_clientdict:
+                await self.store.set_ui_auth_clientdict(sid, clientdict)
+
         if not authdict:
             raise InteractiveAuthIncompleteError(
                 self._auth_dict_for_flows(flows, session.session_id)

synapse/rest/client/v2_alpha/register.py

@@ -516,6 +516,7 @@ async def on_POST(self, request):
             body,
             self.hs.get_ip_from_request(request),
             "register a new account",
+            validate_clientdict=False,
         )
 
         # Check that we're not trying to register a denied 3pid.

synapse/storage/data_stores/main/ui_auth.py

@@ -172,6 +172,27 @@ async def get_completed_ui_auth_stages(
 
         return results
 
+    async def set_ui_auth_clientdict(
+        self, session_id: str, clientdict: JsonDict
+    ) -> None:
+        """
+        Store an updated clientdict for a given session ID.
+
+        Args:
+            session_id: The ID of this session as returned from check_auth
+            clientdict:
+                The dictionary from the client root level, not the 'auth' key.
+        """
+        # The clientdict gets stored as JSON.
+        clientdict_json = json.dumps(clientdict)
+
+        self.db.simple_update_one(
+            table="ui_auth_sessions",
+            keyvalues={"session_id": session_id},
+            updatevalues={"clientdict": clientdict_json},
+            desc="set_ui_auth_client_dict",
+        )
+
     async def set_ui_auth_session_data(self, session_id: str, key: str, value: Any):
         """
         Store a key-value pair into the sessions data associated with this