From 89b3530fe4fe2c24dd424442ad3b018226b4e04a Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 4 Apr 2022 14:36:26 -0700 Subject: [PATCH 01/10] limit size of device_id --- synapse/rest/client/login.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index c9d44c5964ce..994e992ef165 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -14,6 +14,7 @@ import logging import re +import sys from typing import ( TYPE_CHECKING, Any, @@ -342,6 +343,16 @@ async def _complete_login( user_id = canonical_uid device_id = login_submission.get("device_id") + + # Check that device_id is not greater than 8KB as this causes issues with Postgres indexing + device_id_size = sys.getsizeof(device_id) + if device_id_size > 8000: + raise LoginError( + 400, + "Device id cannot be greater than 8KB.", + errcode=Codes.INVALID_PARAM, + ) + initial_display_name = login_submission.get("initial_device_display_name") ( device_id, From 0cd4345b2b46c29be8d1b48560cade1931dac84d Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 4 Apr 2022 14:36:55 -0700 Subject: [PATCH 02/10] test that a login with an overly large device_id will fail --- tests/rest/client/test_login.py | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 090d2d0a2927..6251b8a164d1 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -11,7 +11,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - +import json +import sys import time import urllib.parse from typing import Any, Dict, List, Optional, Union @@ -384,6 +385,36 @@ def test_session_can_hard_logout_all_sessions_after_being_soft_logged_out( channel = self.make_request(b"POST", "/logout/all", access_token=access_token) self.assertEqual(channel.result["code"], b"200", channel.result) + def test_login_with_large_device_id_fails(self) -> None: + self.register_user("mickey", "cheese") + + # create a device_id larger than 8KB + device_id = 1000**100000 + self.assertGreater(sys.getsizeof(device_id), 8000) + + body = { + "type": "m.login.password", + "user": "mickey", + "password": "cheese", + "device_id": device_id, + } + + # make a login request with the bad device_id + channel = self.make_request( + "POST", + "/_matrix/client/r0/login", + json.dumps(body).encode("utf8"), + custom_headers=None, + ) + self.assertEqual(channel.code, 400) + self.assertEqual( + channel.json_body, + { + "errcode": "M_INVALID_PARAM", + "error": "Device id cannot be greater than 8KB.", + }, + ) + @skip_unless(has_saml2 and HAS_OIDC, "Requires SAML2 and OIDC") class MultiSSOTestCase(unittest.HomeserverTestCase): From 7edd01272695a049b27a5640d6aaedef1e5efce8 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 11 Apr 2022 12:55:41 -0700 Subject: [PATCH 03/10] device id -> device_id --- synapse/rest/client/login.py | 4 ++-- tests/rest/client/test_login.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 994e992ef165..0211c45bcf4f 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -344,12 +344,12 @@ async def _complete_login( device_id = login_submission.get("device_id") - # Check that device_id is not greater than 8KB as this causes issues with Postgres indexing + # Check that device_id is not greater than 8KB as Postgres is unable to index values larger than this device_id_size = sys.getsizeof(device_id) if device_id_size > 8000: raise LoginError( 400, - "Device id cannot be greater than 8KB.", + "Device_id cannot be greater than 8KB.", errcode=Codes.INVALID_PARAM, ) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 6251b8a164d1..37040ab65768 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -385,7 +385,7 @@ def test_session_can_hard_logout_all_sessions_after_being_soft_logged_out( channel = self.make_request(b"POST", "/logout/all", access_token=access_token) self.assertEqual(channel.result["code"], b"200", channel.result) - def test_login_with_large_device_id_fails(self) -> None: + def test_login_with_overly_large_device_id_fails(self) -> None: self.register_user("mickey", "cheese") # create a device_id larger than 8KB @@ -406,12 +406,14 @@ def test_login_with_large_device_id_fails(self) -> None: json.dumps(body).encode("utf8"), custom_headers=None, ) + + # test that the login fails with the correct error code/message self.assertEqual(channel.code, 400) self.assertEqual( channel.json_body, { "errcode": "M_INVALID_PARAM", - "error": "Device id cannot be greater than 8KB.", + "error": "Device_id cannot be greater than 8KB.", }, ) From b72425931c8b83556c4a65835e5da003b44d47e0 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 11 Apr 2022 13:02:02 -0700 Subject: [PATCH 04/10] newsfragement --- changelog.d/12440.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12440.misc diff --git a/changelog.d/12440.misc b/changelog.d/12440.misc new file mode 100644 index 000000000000..e12f04c3a1d5 --- /dev/null +++ b/changelog.d/12440.misc @@ -0,0 +1 @@ +Limit max size of device_id to less than 8KB. From 06df24738fce25fe8a242f469ecedf43906b15c4 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 10:21:05 -0700 Subject: [PATCH 05/10] change limit to 512B --- synapse/rest/client/login.py | 6 +++--- tests/rest/client/test_login.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 0211c45bcf4f..0c365d0d9d31 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -344,12 +344,12 @@ async def _complete_login( device_id = login_submission.get("device_id") - # Check that device_id is not greater than 8KB as Postgres is unable to index values larger than this + # Check that device_id is not greater than a reasonable 512B device_id_size = sys.getsizeof(device_id) - if device_id_size > 8000: + if device_id_size > 512: raise LoginError( 400, - "Device_id cannot be greater than 8KB.", + "Device_id cannot be greater than 512B.", errcode=Codes.INVALID_PARAM, ) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index 37040ab65768..cce45193fd08 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -389,8 +389,8 @@ def test_login_with_overly_large_device_id_fails(self) -> None: self.register_user("mickey", "cheese") # create a device_id larger than 8KB - device_id = 1000**100000 - self.assertGreater(sys.getsizeof(device_id), 8000) + device_id = 1000**1000 + self.assertGreater(sys.getsizeof(device_id), 512) body = { "type": "m.login.password", @@ -413,7 +413,7 @@ def test_login_with_overly_large_device_id_fails(self) -> None: channel.json_body, { "errcode": "M_INVALID_PARAM", - "error": "Device_id cannot be greater than 8KB.", + "error": "Device_id cannot be greater than 512B.", }, ) From 6c8e1c85256794121f7861d429d783a8e6003d83 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 10:35:24 -0700 Subject: [PATCH 06/10] update comment --- tests/rest/client/test_login.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index cce45193fd08..daece9a2a2d8 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -388,7 +388,7 @@ def test_session_can_hard_logout_all_sessions_after_being_soft_logged_out( def test_login_with_overly_large_device_id_fails(self) -> None: self.register_user("mickey", "cheese") - # create a device_id larger than 8KB + # create a device_id larger than 512B device_id = 1000**1000 self.assertGreater(sys.getsizeof(device_id), 512) From 8a0cc993fa08a029f9953e71ad310fdf0a8b11e8 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 10:41:26 -0700 Subject: [PATCH 07/10] fix changelog number --- changelog.d/{12440.misc => 12454.misc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{12440.misc => 12454.misc} (100%) diff --git a/changelog.d/12440.misc b/changelog.d/12454.misc similarity index 100% rename from changelog.d/12440.misc rename to changelog.d/12454.misc From c463789f320d0ce2f1fb0905c4ba76aba81ffb1c Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 13:58:02 -0700 Subject: [PATCH 08/10] limit by charaters vs size --- changelog.d/12454.misc | 2 +- synapse/rest/client/login.py | 7 +++---- tests/rest/client/test_login.py | 9 ++++----- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/changelog.d/12454.misc b/changelog.d/12454.misc index e12f04c3a1d5..cb7ff74b4c8f 100644 --- a/changelog.d/12454.misc +++ b/changelog.d/12454.misc @@ -1 +1 @@ -Limit max size of device_id to less than 8KB. +Limit length of device_id to less than 512 characters. diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 0c365d0d9d31..9b49fc720d31 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -344,12 +344,11 @@ async def _complete_login( device_id = login_submission.get("device_id") - # Check that device_id is not greater than a reasonable 512B - device_id_size = sys.getsizeof(device_id) - if device_id_size > 512: + # Check that device_id is not longer than a reasonable 512 characters + if len(device_id) > 512: raise LoginError( 400, - "Device_id cannot be greater than 512B.", + "Device_id cannot be longer than 512 characters.", errcode=Codes.INVALID_PARAM, ) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index daece9a2a2d8..cdae00f36842 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -385,12 +385,11 @@ def test_session_can_hard_logout_all_sessions_after_being_soft_logged_out( channel = self.make_request(b"POST", "/logout/all", access_token=access_token) self.assertEqual(channel.result["code"], b"200", channel.result) - def test_login_with_overly_large_device_id_fails(self) -> None: + def test_login_with_overly_long_device_id_fails(self) -> None: self.register_user("mickey", "cheese") - # create a device_id larger than 512B - device_id = 1000**1000 - self.assertGreater(sys.getsizeof(device_id), 512) + # create a device_id longer than 512 characters + device_id = "yolo"*512 body = { "type": "m.login.password", @@ -413,7 +412,7 @@ def test_login_with_overly_large_device_id_fails(self) -> None: channel.json_body, { "errcode": "M_INVALID_PARAM", - "error": "Device_id cannot be greater than 512B.", + "error": "Device_id cannot be longer than 512 characters.", }, ) From c2bdd73899385dbb77a94a24e9b73ca4194ba651 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 14:22:37 -0700 Subject: [PATCH 09/10] lints --- synapse/rest/client/login.py | 5 ++--- tests/rest/client/test_login.py | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 9b49fc720d31..2c0d6acb0cb6 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -14,7 +14,6 @@ import logging import re -import sys from typing import ( TYPE_CHECKING, Any, @@ -344,8 +343,8 @@ async def _complete_login( device_id = login_submission.get("device_id") - # Check that device_id is not longer than a reasonable 512 characters - if len(device_id) > 512: + # If device_id is present, check that device_id is not longer than a reasonable 512 characters + if device_id and len(device_id) > 512: raise LoginError( 400, "Device_id cannot be longer than 512 characters.", diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index cdae00f36842..fbe149b35b70 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import json -import sys import time import urllib.parse from typing import Any, Dict, List, Optional, Union @@ -389,7 +388,7 @@ def test_login_with_overly_long_device_id_fails(self) -> None: self.register_user("mickey", "cheese") # create a device_id longer than 512 characters - device_id = "yolo"*512 + device_id = "yolo" * 512 body = { "type": "m.login.password", From 6960ca08e0970fc575c736c7378c46f5d3808308 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 12 Apr 2022 19:45:37 -0700 Subject: [PATCH 10/10] requested changes --- synapse/rest/client/login.py | 2 +- tests/rest/client/test_login.py | 12 +++--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 2c0d6acb0cb6..4a4dbe75de6b 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -347,7 +347,7 @@ async def _complete_login( if device_id and len(device_id) > 512: raise LoginError( 400, - "Device_id cannot be longer than 512 characters.", + "device_id cannot be longer than 512 characters.", errcode=Codes.INVALID_PARAM, ) diff --git a/tests/rest/client/test_login.py b/tests/rest/client/test_login.py index fbe149b35b70..0a3d017dc9b9 100644 --- a/tests/rest/client/test_login.py +++ b/tests/rest/client/test_login.py @@ -400,20 +400,14 @@ def test_login_with_overly_long_device_id_fails(self) -> None: # make a login request with the bad device_id channel = self.make_request( "POST", - "/_matrix/client/r0/login", + "/_matrix/client/v3/login", json.dumps(body).encode("utf8"), custom_headers=None, ) - # test that the login fails with the correct error code/message + # test that the login fails with the correct error code self.assertEqual(channel.code, 400) - self.assertEqual( - channel.json_body, - { - "errcode": "M_INVALID_PARAM", - "error": "Device_id cannot be longer than 512 characters.", - }, - ) + self.assertEqual(channel.json_body["errcode"], "M_INVALID_PARAM") @skip_unless(has_saml2 and HAS_OIDC, "Requires SAML2 and OIDC")