From 353f2407b76a2c8081d153b7c83d7b1bef9bb547 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 12:31:25 +0000 Subject: [PATCH 1/9] Fix fallback to signing key for macaroon-secret-key --- synapse/config/key.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/key.py b/synapse/config/key.py index 279c47bb4..a7998c941 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -57,8 +57,8 @@ class KeyConfig(Config): # Unfortunately, there are people out there that don't have this # set. Lets just be "nice" and derive one from their secret key. logger.warn("Config is missing missing macaroon_secret_key") - seed = self.signing_key[0].seed - self.macaroon_secret_key = hashlib.sha256(seed) + seed = bytes(self.signing_key[0]) + self.macaroon_secret_key = hashlib.sha256(seed).digest() self.expire_access_token = config.get("expire_access_token", False) From 566947ff34dee38eaa6c9ea1090c743366d709eb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 12:41:13 +0000 Subject: [PATCH 2/9] Skip macaroon check for access tokens in the db --- synapse/api/auth.py | 67 ++++++++---------- tests/api/test_auth.py | 149 +---------------------------------------- 2 files changed, 29 insertions(+), 187 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 5bbbe8e2e..7d76dbd66 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -288,20 +288,28 @@ class Auth(object): Raises: AuthError if no user by that token exists or the token is invalid. """ + + if rights == "access": + # first look in the database + r = yield self._look_up_user_by_access_token(token) + if r: + defer.returnValue(r) + + # otherwise it needs to be a valid macaroon try: user_id, guest = self._parse_and_validate_macaroon(token, rights) - except _InvalidMacaroonException: - # doesn't look like a macaroon: treat it as an opaque token which - # must be in the database. - # TODO: it would be nice to get rid of this, but apparently some - # people use access tokens which aren't macaroons - r = yield self._look_up_user_by_access_token(token) - defer.returnValue(r) - - try: user = UserID.from_string(user_id) - if guest: + if rights == "access": + if not guest: + # non-guest access tokens must be in the database + logger.warning("Unrecognised access token - not in store.") + raise AuthError( + self.TOKEN_NOT_FOUND_HTTP_STATUS, + "Unrecognised access token.", + errcode=Codes.UNKNOWN_TOKEN, + ) + # Guest access tokens are not stored in the database (there can # only be one access token per guest, anyway). # @@ -342,31 +350,15 @@ class Auth(object): "device_id": None, } else: - # This codepath exists for several reasons: - # * so that we can actually return a token ID, which is used - # in some parts of the schema (where we probably ought to - # use device IDs instead) - # * the only way we currently have to invalidate an - # access_token is by removing it from the database, so we - # have to check here that it is still in the db - # * some attributes (notably device_id) aren't stored in the - # macaroon. They probably should be. - # TODO: build the dictionary from the macaroon once the - # above are fixed - ret = yield self._look_up_user_by_access_token(token) - if ret["user"] != user: - logger.error( - "Macaroon user (%s) != DB user (%s)", - user, - ret["user"] - ) - raise AuthError( - self.TOKEN_NOT_FOUND_HTTP_STATUS, - "User mismatch in macaroon", - errcode=Codes.UNKNOWN_TOKEN - ) + raise RuntimeError("Unknown rights setting %s", rights) defer.returnValue(ret) - except (pymacaroons.exceptions.MacaroonException, TypeError, ValueError): + except ( + _InvalidMacaroonException, + pymacaroons.exceptions.MacaroonException, + TypeError, + ValueError, + ) as e: + logger.warning("Invalid macaroon in auth: %s %s", type(e), e) raise AuthError( self.TOKEN_NOT_FOUND_HTTP_STATUS, "Invalid macaroon passed.", errcode=Codes.UNKNOWN_TOKEN @@ -496,11 +488,8 @@ class Auth(object): def _look_up_user_by_access_token(self, token): ret = yield self.store.get_user_by_access_token(token) if not ret: - logger.warn("Unrecognised access token - not in store.") - raise AuthError( - self.TOKEN_NOT_FOUND_HTTP_STATUS, "Unrecognised access token.", - errcode=Codes.UNKNOWN_TOKEN - ) + defer.returnValue(None) + # we use ret.get() below because *lots* of unit tests stub out # get_user_by_access_token in a way where it only returns a couple of # the fields. diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index a82d737e7..1faeb92f1 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -198,8 +198,6 @@ class AuthTestCase(unittest.TestCase): @defer.inlineCallbacks def test_get_user_from_macaroon(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. self.store.get_user_by_access_token = Mock( return_value={ "name": "@baldrick:matrix.org", @@ -228,6 +226,7 @@ class AuthTestCase(unittest.TestCase): self.store.get_user_by_id = Mock(return_value={ "is_guest": True, }) + self.store.get_user_by_access_token = Mock(return_value=None) user_id = "@baldrick:matrix.org" macaroon = pymacaroons.Macaroon( @@ -247,152 +246,6 @@ class AuthTestCase(unittest.TestCase): self.assertTrue(is_guest) self.store.get_user_by_id.assert_called_with(user_id) - @defer.inlineCallbacks - def test_get_user_from_macaroon_user_db_mismatch(self): - self.store.get_user_by_access_token = Mock( - return_value={"name": "@percy:matrix.org"} - ) - - user = "@baldrick:matrix.org" - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key) - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - macaroon.add_first_party_caveat("user_id = %s" % (user,)) - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_access_token(macaroon.serialize()) - self.assertEqual(401, cm.exception.code) - self.assertIn("User mismatch", cm.exception.msg) - - @defer.inlineCallbacks - def test_get_user_from_macaroon_missing_caveat(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key) - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_access_token(macaroon.serialize()) - self.assertEqual(401, cm.exception.code) - self.assertIn("No user caveat", cm.exception.msg) - - @defer.inlineCallbacks - def test_get_user_from_macaroon_wrong_key(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - user = "@baldrick:matrix.org" - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key + "wrong") - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - macaroon.add_first_party_caveat("user_id = %s" % (user,)) - - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_access_token(macaroon.serialize()) - self.assertEqual(401, cm.exception.code) - self.assertIn("Invalid macaroon", cm.exception.msg) - - @defer.inlineCallbacks - def test_get_user_from_macaroon_unknown_caveat(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - user = "@baldrick:matrix.org" - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key) - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - macaroon.add_first_party_caveat("user_id = %s" % (user,)) - macaroon.add_first_party_caveat("cunning > fox") - - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_access_token(macaroon.serialize()) - self.assertEqual(401, cm.exception.code) - self.assertIn("Invalid macaroon", cm.exception.msg) - - @defer.inlineCallbacks - def test_get_user_from_macaroon_expired(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - user = "@baldrick:matrix.org" - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key) - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - macaroon.add_first_party_caveat("user_id = %s" % (user,)) - macaroon.add_first_party_caveat("time < -2000") # ms - - self.hs.clock.now = 5000 # seconds - self.hs.config.expire_access_token = True - # yield self.auth.get_user_by_access_token(macaroon.serialize()) - # TODO(daniel): Turn on the check that we validate expiration, when we - # validate expiration (and remove the above line, which will start - # throwing). - with self.assertRaises(AuthError) as cm: - yield self.auth.get_user_by_access_token(macaroon.serialize()) - self.assertEqual(401, cm.exception.code) - self.assertIn("Invalid macaroon", cm.exception.msg) - - @defer.inlineCallbacks - def test_get_user_from_macaroon_with_valid_duration(self): - # TODO(danielwh): Remove this mock when we remove the - # get_user_by_access_token fallback. - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - self.store.get_user_by_access_token = Mock( - return_value={"name": "@baldrick:matrix.org"} - ) - - user_id = "@baldrick:matrix.org" - macaroon = pymacaroons.Macaroon( - location=self.hs.config.server_name, - identifier="key", - key=self.hs.config.macaroon_secret_key) - macaroon.add_first_party_caveat("gen = 1") - macaroon.add_first_party_caveat("type = access") - macaroon.add_first_party_caveat("user_id = %s" % (user_id,)) - macaroon.add_first_party_caveat("time < 900000000") # ms - - self.hs.clock.now = 5000 # seconds - self.hs.config.expire_access_token = True - - user_info = yield self.auth.get_user_by_access_token(macaroon.serialize()) - user = user_info["user"] - self.assertEqual(UserID.from_string(user_id), user) - @defer.inlineCallbacks def test_cannot_use_regular_token_as_guest(self): USER_ID = "@percy:matrix.org" From efc522c55e996e420271de2d9094835dda52ade4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 12:41:56 +0000 Subject: [PATCH 3/9] Fix macaroon_secret_key fallback logic --- synapse/config/key.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/config/key.py b/synapse/config/key.py index a7998c941..c26b7529f 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -71,14 +71,14 @@ class KeyConfig(Config): base_key_name = os.path.join(config_dir_path, server_name) if is_generating_file: - macaroon_secret_key = random_string_with_symbols(50) + macaroon_secret_key = '"%s"' % random_string_with_symbols(50) form_secret = '"%s"' % random_string_with_symbols(50) else: - macaroon_secret_key = None + macaroon_secret_key = 'null' form_secret = 'null' return """\ - macaroon_secret_key: "%(macaroon_secret_key)s" + macaroon_secret_key: %(macaroon_secret_key)s # Used to enable access token expiration. expire_access_token: False From 4f24452ead67f8d282e44a7ffc697a513012069e Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 14:00:23 +0000 Subject: [PATCH 4/9] changelog --- changelog.d/4373.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4373.bugfix diff --git a/changelog.d/4373.bugfix b/changelog.d/4373.bugfix new file mode 100644 index 000000000..e50697cc5 --- /dev/null +++ b/changelog.d/4373.bugfix @@ -0,0 +1 @@ +Fix problem reading macaroon_secret_key from config From ba41aeed6a6565e3c5348aaa568f5fd30e42e97a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 14:09:26 +0000 Subject: [PATCH 5/9] Revert "Fix macaroon_secret_key fallback logic" This is already fixed in 0.34.1, by 59f93bb This reverts commit efc522c55e996e420271de2d9094835dda52ade4. --- synapse/config/key.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/config/key.py b/synapse/config/key.py index c26b7529f..a7998c941 100644 --- a/synapse/config/key.py +++ b/synapse/config/key.py @@ -71,14 +71,14 @@ class KeyConfig(Config): base_key_name = os.path.join(config_dir_path, server_name) if is_generating_file: - macaroon_secret_key = '"%s"' % random_string_with_symbols(50) + macaroon_secret_key = random_string_with_symbols(50) form_secret = '"%s"' % random_string_with_symbols(50) else: - macaroon_secret_key = 'null' + macaroon_secret_key = None form_secret = 'null' return """\ - macaroon_secret_key: %(macaroon_secret_key)s + macaroon_secret_key: "%(macaroon_secret_key)s" # Used to enable access token expiration. expire_access_token: False From de80e979c904985a15003eadb728d292c2fc3c99 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 10 Jan 2019 14:26:01 +0000 Subject: [PATCH 6/9] changelog --- changelog.d/4373.bugfix | 1 - changelog.d/4374.bugfix | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/4373.bugfix create mode 100644 changelog.d/4374.bugfix diff --git a/changelog.d/4373.bugfix b/changelog.d/4373.bugfix deleted file mode 100644 index e50697cc5..000000000 --- a/changelog.d/4373.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix problem reading macaroon_secret_key from config diff --git a/changelog.d/4374.bugfix b/changelog.d/4374.bugfix new file mode 100644 index 000000000..0c8b0da5e --- /dev/null +++ b/changelog.d/4374.bugfix @@ -0,0 +1 @@ +Fix spontaneous logout on upgrade From 6dc06c3775e05d3ae160f25475b03ad92447955f Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 11 Jan 2019 01:56:37 +1100 Subject: [PATCH 7/9] version --- synapse/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/__init__.py b/synapse/__init__.py index 2935238fa..d6a191ccc 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -27,4 +27,4 @@ try: except ImportError: pass -__version__ = "0.34.1" +__version__ = "0.34.1.1" From 5c792ee5c39d0660c7ca1341ee12556358a262c8 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 11 Jan 2019 01:59:10 +1100 Subject: [PATCH 8/9] changelog --- CHANGES.md | 14 ++++++++++++++ changelog.d/4374.bugfix | 1 - 2 files changed, 14 insertions(+), 1 deletion(-) delete mode 100644 changelog.d/4374.bugfix diff --git a/CHANGES.md b/CHANGES.md index e403a65d1..6fdd31daa 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,17 @@ +Synapse 0.34.1.1 (2019-01-11) +============================= + +This release fixes CVE-2019-5885 and is recommended for all users of Synapse 0.34.1. + +This release is compatible with Python 2.7 and 3.5+. Python 3.7 is fully supported. + +Bugfixes +-------- + +- Fix spontaneous logout on upgrade + ([\#4374](https://github.com/matrix-org/synapse/issues/4374)) + + Synapse 0.34.1 (2019-01-09) =========================== diff --git a/changelog.d/4374.bugfix b/changelog.d/4374.bugfix deleted file mode 100644 index 0c8b0da5e..000000000 --- a/changelog.d/4374.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix spontaneous logout on upgrade From c0dba73aa0575da91d627d279d8d0f69a2578c42 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Fri, 11 Jan 2019 02:20:29 +1100 Subject: [PATCH 9/9] changelog, for debian --- debian/changelog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/debian/changelog b/debian/changelog index 921f7021d..09bb0001e 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (0.34.1.1) stable; urgency=high + + * New synapse release 0.34.1.1 + + -- Synapse Packaging team Thu, 10 Jan 2019 15:04:52 +0000 + matrix-synapse-py3 (0.34.1+1) stable; urgency=medium * Remove 'Breaks: matrix-synapse-ldap3'. (matrix-synapse-py3 includes