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

Commit

Permalink
Validate that the session is not modified during UI-Auth (#7068)
Browse files Browse the repository at this point in the history
  • Loading branch information
clokep authored Mar 26, 2020
1 parent 6ca5e56 commit 1c1242a
Show file tree
Hide file tree
Showing 8 changed files with 117 additions and 14 deletions.
1 change: 1 addition & 0 deletions changelog.d/7068.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.
37 changes: 33 additions & 4 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -322,6 +350,7 @@ def check_auth(
creds,
list(clientdict),
)

return creds, clientdict, session["id"]

ret = self._auth_dict_for_flows(flows, session)
Expand Down
11 changes: 7 additions & 4 deletions synapse/rest/client/v2_alpha/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions synapse/rest/client/v2_alpha/devices.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/v2_alpha/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion synapse/rest/client/v2_alpha/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
68 changes: 67 additions & 1 deletion tests/rest/client/v2_alpha/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
3 changes: 2 additions & 1 deletion tests/test_terms_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 1c1242a

Please sign in to comment.