Validate that the session is not modified during UI-Auth (matrix-org#…

…7068)

changelog.d/7068.bugfix

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

synapse/handlers/auth.py

@@ -125,7 +125,11 @@ def __init__(self, hs):
 
  @defer.inlineCallbacks
  def validate_user_via_ui_auth(
- self, requester: Requester, request_body: Dict[str, Any], clientip: str
+ self,
+ requester: Requester,
+ request: SynapseRequest,
+ request_body: Dict[str, Any],
+ clientip: str,
  ):
  """
  Checks that the user is who they claim to be, via a UI auth.
@@ -137,6 +141,8 @@ def validate_user_via_ui_auth(
  Args:
  requester: The user, as given by the access token
 
+ request: The request sent by the client.
+
  request_body: The body of the request sent by the client
 
  clientip: The IP address of the client.
@@ -172,7 +178,9 @@ def validate_user_via_ui_auth(
  flows = [[login_type] for login_type in self._supported_login_types]
 
  try:
- result, params, _ = yield self.check_auth(flows, request_body, clientip)
+ result, params, _ = yield self.check_auth(
+ flows, request, request_body, clientip
+ )
  except LoginError:
  # Update the ratelimite to say we failed (`can_do_action` doesn't raise).
  self._failed_uia_attempts_ratelimiter.can_do_action(
@@ -211,7 +219,11 @@ def get_enabled_auth_types(self):
 
  @defer.inlineCallbacks
  def check_auth(
- self, flows: List[List[str]], clientdict: Dict[str, Any], clientip: str
+ self,
+ flows: List[List[str]],
+ request: SynapseRequest,
+ clientdict: Dict[str, Any],
+ clientip: str,
  ):
  """
  Takes a dictionary sent by the client in the login / registration
@@ -231,6 +243,8 @@ def check_auth(
  strings representing auth-types. At least one full
  flow must be completed in order for auth to be successful.
 
+ request: The request sent by the client.
+
  clientdict: The dictionary from the client root level, not the
  'auth' key: this method prompts for auth if none is sent.
 
@@ -270,13 +284,27 @@ def check_auth(
  # email auth link on there). It's probably too open to abuse
  # because it lets unauthenticated clients store arbitrary objects
  # on a homeserver.
- # Revisit: Assumimg the REST APIs do sensible validation, the data
+ # Revisit: Assuming the REST APIs do sensible validation, the data
  # isn't arbintrary.
  session["clientdict"] = clientdict
  self._save_session(session)
  elif "clientdict" in session:
  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 = (request.uri, request.method, clientdict)
+ if "ui_auth" not in session:
+ session["ui_auth"] = comparator
+ elif session["ui_auth"] != comparator:
+ raise SynapseError(
+ 403,
+ "Requested operation has changed during the UI authentication session.",
+ )
+
  if not authdict:
  raise InteractiveAuthIncompleteError(
  self._auth_dict_for_flows(flows, session)
@@ -322,6 +350,7 @@ def check_auth(
  creds,
  list(clientdict),
  )
+
  return creds, clientdict, session["id"]
 
  ret = self._auth_dict_for_flows(flows, session)

synapse/rest/client/v2_alpha/account.py

@@ -234,13 +234,16 @@ async def on_POST(self, request):
  if self.auth.has_access_token(request):
  requester = await self.auth.get_user_by_req(request)
  params = await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
  user_id = requester.user.to_string()
  else:
  requester = None
  result, params, _ = await self.auth_handler.check_auth(
- [[LoginType.EMAIL_IDENTITY]], body, self.hs.get_ip_from_request(request)
+ [[LoginType.EMAIL_IDENTITY]],
+ request,
+ body,
+ self.hs.get_ip_from_request(request),
  )
 
  if LoginType.EMAIL_IDENTITY in result:
@@ -308,7 +311,7 @@ async def on_POST(self, request):
  return 200, {}
 
  await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
  result = await self._deactivate_account_handler.deactivate_account(
  requester.user.to_string(), erase, id_server=body.get("id_server")
@@ -656,7 +659,7 @@ async def on_POST(self, request):
  assert_valid_client_secret(client_secret)
 
  await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
 
  validation_session = await self.identity_handler.validate_threepid_session(

synapse/rest/client/v2_alpha/devices.py

@@ -81,7 +81,7 @@ async def on_POST(self, request):
  assert_params_in_dict(body, ["devices"])
 
  await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
 
  await self.device_handler.delete_devices(
@@ -127,7 +127,7 @@ async def on_DELETE(self, request, device_id):
  raise
 
  await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
 
  await self.device_handler.delete_device(requester.user.to_string(), device_id)

synapse/rest/client/v2_alpha/keys.py

@@ -263,7 +263,7 @@ async def on_POST(self, request):
  body = parse_json_object_from_request(request)
 
  await self.auth_handler.validate_user_via_ui_auth(
- requester, body, self.hs.get_ip_from_request(request)
+ requester, request, body, self.hs.get_ip_from_request(request),
  )
 
  result = await self.e2e_keys_handler.upload_signing_keys_for_user(user_id, body)

synapse/rest/client/v2_alpha/register.py

@@ -499,7 +499,10 @@ async def on_POST(self, request):
  )
 
  auth_result, params, session_id = await self.auth_handler.check_auth(
- self._registration_flows, body, self.hs.get_ip_from_request(request)
+ self._registration_flows,
+ request,
+ body,
+ self.hs.get_ip_from_request(request),
  )
 
  # Check that we're not trying to register a denied 3pid.

tests/rest/client/v2_alpha/test_auth.py

@@ -104,7 +104,7 @@ def test_fallback_captcha(self):
  )
  self.render(request)
 
- # Now we should have fufilled a complete auth flow, including
+ # Now we should have fulfilled a complete auth flow, including
  # the recaptcha fallback step, we can then send a
  # request to the register API with the session in the authdict.
  request, channel = self.make_request(
@@ -115,3 +115,69 @@ def test_fallback_captcha(self):
 
  # We're given a registered user.
  self.assertEqual(channel.json_body["user_id"], "@user:test")
+
+ def test_cannot_change_operation(self):
+ """
+ The initial requested operation cannot be modified during the user interactive authentication session.
+ """
+
+ # Make the initial request to register. (Later on a different password
+ # will be used.)
+ request, channel = self.make_request(
+ "POST",
+ "register",
+ {"username": "user", "type": "m.login.password", "password": "bar"},
+ )
+ self.render(request)
+
+ # Returns a 401 as per the spec
+ self.assertEqual(request.code, 401)
+ # Grab the session
+ session = channel.json_body["session"]
+ # Assert our configured public key is being given
+ self.assertEqual(
+ channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake"
+ )
+
+ request, channel = self.make_request(
+ "GET", "auth/m.login.recaptcha/fallback/web?session=" + session
+ )
+ self.render(request)
+ self.assertEqual(request.code, 200)
+
+ request, channel = self.make_request(
+ "POST",
+ "auth/m.login.recaptcha/fallback/web?session="
+ + session
+ + "&g-recaptcha-response=a",
+ )
+ self.render(request)
+ self.assertEqual(request.code, 200)
+
+ # The recaptcha handler is called with the response given
+ attempts = self.recaptcha_checker.recaptcha_attempts
+ self.assertEqual(len(attempts), 1)
+ self.assertEqual(attempts[0][0]["response"], "a")
+
+ # also complete the dummy auth
+ request, channel = self.make_request(
+ "POST", "register", {"auth": {"session": session, "type": "m.login.dummy"}}
+ )
+ self.render(request)
+
+ # Now we should have fulfilled a complete auth flow, including
+ # the recaptcha fallback step. Make the initial request again, but
+ # with a different password. This causes the request to fail since the
+ # operaiton was modified during the ui auth session.
+ request, channel = self.make_request(
+ "POST",
+ "register",
+ {
+ "username": "user",
+ "type": "m.login.password",
+ "password": "foo", # Note this doesn't match the original request.
+ "auth": {"session": session},
+ },
+ )
+ self.render(request)
+ self.assertEqual(channel.code, 403)

tests/test_terms_auth.py

@@ -53,7 +53,8 @@ def prepare(self, reactor, clock, hs):
 
  def test_ui_auth(self):
  # Do a UI auth request
- request, channel = self.make_request(b"POST", self.url, b"{}")
+ request_data = json.dumps({"username": "kermit", "password": "monkey"})
+ request, channel = self.make_request(b"POST", self.url, request_data)
  self.render(request)
 
  self.assertEquals(channel.result["code"], b"401", channel.result)