Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Rework UI Auth session validation #7455

Merged
merged 13 commits into from
May 8, 2020
1 change: 1 addition & 0 deletions changelog.d/7455.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure that a user inteactive authentication session is tied to a single request.
54 changes: 40 additions & 14 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
@@ -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)
clokep marked this conversation as resolved.
Show resolved Hide resolved
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)
1 change: 1 addition & 0 deletions synapse/rest/client/v2_alpha/register.py
Original file line number Diff line number Diff line change
@@ -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.
21 changes: 21 additions & 0 deletions synapse/storage/data_stores/main/ui_auth.py
Original file line number Diff line number Diff line change
@@ -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
Loading