From 25fb1be065ad570c90a6f3932c096dbc6e904ff5 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 29 Jul 2020 19:30:56 +0100 Subject: [PATCH 1/6] Don't raise session_id errors on submit_token if request_token_inhibit_3pid_errors is set --- .../storage/data_stores/main/registration.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/synapse/storage/data_stores/main/registration.py b/synapse/storage/data_stores/main/registration.py index 27d2c5028c42..0e63ccea6168 100644 --- a/synapse/storage/data_stores/main/registration.py +++ b/synapse/storage/data_stores/main/registration.py @@ -911,6 +911,7 @@ def __init__(self, database: Database, db_conn, hs): super(RegistrationStore, self).__init__(database, db_conn, hs) self._account_validity = hs.config.account_validity + self._ignore_unknown_session_error = hs.config.request_token_inhibit_3pid_errors if self._account_validity.enabled: self._clock.call_later( @@ -1315,15 +1316,14 @@ def validate_threepid_session_txn(txn): ) if not row: - raise ThreepidValidationError(400, "Unknown session_id") + if self._ignore_unknown_session_error: + row = {"client_secret": None, "validated_at": None} + else: + raise ThreepidValidationError(400, "Unknown session_id") + retrieved_client_secret = row["client_secret"] validated_at = row["validated_at"] - if retrieved_client_secret != client_secret: - raise ThreepidValidationError( - 400, "This client_secret does not match the provided session_id" - ) - row = self.db.simple_select_one_txn( txn, table="threepid_validation_token", @@ -1339,6 +1339,11 @@ def validate_threepid_session_txn(txn): expires = row["expires"] next_link = row["next_link"] + if retrieved_client_secret != client_secret: + raise ThreepidValidationError( + 400, "This client_secret does not match the provided session_id" + ) + # If the session is already validated, no need to revalidate if validated_at: return next_link From 07828765ceef0a5b33c1e54da2aaa7f692f8a734 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 29 Jul 2020 19:34:00 +0100 Subject: [PATCH 2/6] Changelog --- changelog.d/7991.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7991.misc diff --git a/changelog.d/7991.misc b/changelog.d/7991.misc new file mode 100644 index 000000000000..1562e3af9e55 --- /dev/null +++ b/changelog.d/7991.misc @@ -0,0 +1 @@ +Don't fail `/submit_token` requests on incorrect session ID if `request_token_inhibit_3pid_errors` is turned on. From 8f2118744985506addbd00cca680f162369831f7 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 30 Jul 2020 16:12:52 +0100 Subject: [PATCH 3/6] Also wait some time before responding to /requestToken --- synapse/rest/client/v2_alpha/account.py | 10 ++++++++++ synapse/rest/client/v2_alpha/register.py | 7 +++++++ 2 files changed, 17 insertions(+) diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 3767a809a4a8..727e2b307d8d 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -15,6 +15,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import random from http import HTTPStatus from synapse.api.constants import LoginType @@ -114,6 +115,9 @@ async def on_POST(self, request): if self.config.request_token_inhibit_3pid_errors: # Make the client think the operation succeeded. See the rationale in the # comments for request_token_inhibit_3pid_errors. + # Also wait for some random amount of time between 100ms and 1s to make it + # look like we did something. + await self.hs.clock.sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email not found", Codes.THREEPID_NOT_FOUND) @@ -433,6 +437,9 @@ async def on_POST(self, request): if self.config.request_token_inhibit_3pid_errors: # Make the client think the operation succeeded. See the rationale in the # comments for request_token_inhibit_3pid_errors. + # Also wait for some random amount of time between 100ms and 1s to make it + # look like we did something. + await self.hs.clock.sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email is already in use", Codes.THREEPID_IN_USE) @@ -501,6 +508,9 @@ async def on_POST(self, request): if self.hs.config.request_token_inhibit_3pid_errors: # Make the client think the operation succeeded. See the rationale in the # comments for request_token_inhibit_3pid_errors. + # Also wait for some random amount of time between 100ms and 1s to make it + # look like we did something. + await self.hs.clock.sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index 370742ce59f1..f3514481fbfe 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -16,6 +16,7 @@ import hmac import logging +import random from typing import List, Union import synapse @@ -142,6 +143,9 @@ async def on_POST(self, request): if self.hs.config.request_token_inhibit_3pid_errors: # Make the client think the operation succeeded. See the rationale in the # comments for request_token_inhibit_3pid_errors. + # Also wait for some random amount of time between 100ms and 1s to make it + # look like we did something. + await self.hs.clock.sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError(400, "Email is already in use", Codes.THREEPID_IN_USE) @@ -214,6 +218,9 @@ async def on_POST(self, request): if self.hs.config.request_token_inhibit_3pid_errors: # Make the client think the operation succeeded. See the rationale in the # comments for request_token_inhibit_3pid_errors. + # Also wait for some random amount of time between 100ms and 1s to make it + # look like we did something. + await self.hs.clock.sleep(random.randint(1, 10) / 10) return 200, {"sid": random_string(16)} raise SynapseError( From 56677756da70309a8953c7c025549ba292e87769 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 12 Aug 2020 15:55:54 +0100 Subject: [PATCH 4/6] Incorporate review --- .../storage/databases/main/registration.py | 8 ++++++ tests/storage/test_registration.py | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index 3b1a46a051a5..d9aacd84b551 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1323,6 +1323,14 @@ def validate_threepid_session_txn(txn): if not row: if self._ignore_unknown_session_error: + # If we need to inhibit the error caused by an incorrect session, + # use None as placeholder values for the client secret and the + # validation timestamp if the session ID doesn't exist. + # It shouldn't be an issue because they're both only checked after + # the token check, which should fail. And if it doesn't for some + # reason, the next check is on the client secret, which is NOT NULL, + # so we don't have to worry about the client secret matching by + # accident. row = {"client_secret": None, "validated_at": None} else: raise ThreepidValidationError(400, "Unknown session_id") diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 71a40a0a4911..96145a68ca59 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -17,6 +17,7 @@ from twisted.internet import defer from synapse.api.constants import UserTypes +from synapse.api.errors import ThreepidValidationError from tests import unittest from tests.utils import setup_test_homeserver @@ -116,3 +117,29 @@ def test_is_support_user(self): ) res = yield self.store.is_support_user(SUPPORT_USER) self.assertTrue(res) + + @defer.inlineCallbacks + def test_3pid_inhibit_invalid_validation_session_error(self): + """Tests that enabling the configuration option to inhibit 3PID errors on + /requestToken also inhibits validation errors caused by an unknown session ID. + """ + + # Check that, with the config setting set to false (the default value), a + # validation error is caused by the unknown session ID. + try: + yield self.store.validate_threepid_session( + "fake_sid", "fake_client_secret", "fake_token", 0, + ) + except ThreepidValidationError as e: + self.assertEquals(e.msg, "Unknown session_id", e) + + # Set the config setting to true. + self.store._ignore_unknown_session_error = True + + # Check that now the validation error is caused by the token not matching. + try: + yield self.store.validate_threepid_session( + "fake_sid", "fake_client_secret", "fake_token", 0, + ) + except ThreepidValidationError as e: + self.assertEquals(e.msg, "Validation token not found or has expired", e) From 11847284aae1b06a5d4666e0246c6d56204982bf Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 10:13:10 +0100 Subject: [PATCH 5/6] Update synapse/storage/databases/main/registration.py Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- synapse/storage/databases/main/registration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/registration.py b/synapse/storage/databases/main/registration.py index d9aacd84b551..d4de1ebb06fd 100644 --- a/synapse/storage/databases/main/registration.py +++ b/synapse/storage/databases/main/registration.py @@ -1323,9 +1323,9 @@ def validate_threepid_session_txn(txn): if not row: if self._ignore_unknown_session_error: - # If we need to inhibit the error caused by an incorrect session, + # If we need to inhibit the error caused by an incorrect session ID, # use None as placeholder values for the client secret and the - # validation timestamp if the session ID doesn't exist. + # validation timestamp. # It shouldn't be an issue because they're both only checked after # the token check, which should fail. And if it doesn't for some # reason, the next check is on the client secret, which is NOT NULL, From f80d614d7c05dd06460d63c1a3a6a32da1b55780 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 24 Aug 2020 10:21:24 +0100 Subject: [PATCH 6/6] Incorporate review --- tests/storage/test_registration.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 27308ac5e3fd..58f827d8d329 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -133,8 +133,10 @@ def test_3pid_inhibit_invalid_validation_session_error(self): # Check that, with the config setting set to false (the default value), a # validation error is caused by the unknown session ID. try: - yield self.store.validate_threepid_session( - "fake_sid", "fake_client_secret", "fake_token", 0, + yield defer.ensureDeferred( + self.store.validate_threepid_session( + "fake_sid", "fake_client_secret", "fake_token", 0, + ) ) except ThreepidValidationError as e: self.assertEquals(e.msg, "Unknown session_id", e) @@ -144,8 +146,10 @@ def test_3pid_inhibit_invalid_validation_session_error(self): # Check that now the validation error is caused by the token not matching. try: - yield self.store.validate_threepid_session( - "fake_sid", "fake_client_secret", "fake_token", 0, + yield defer.ensureDeferred( + self.store.validate_threepid_session( + "fake_sid", "fake_client_secret", "fake_token", 0, + ) ) except ThreepidValidationError as e: self.assertEquals(e.msg, "Validation token not found or has expired", e)