From caf33b2d9be1b992098a00ee61cf4b4009ee3a09 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:18:19 +0100 Subject: [PATCH 1/6] Protect password when registering using shared secret --- scripts/register_new_matrix_user | 11 ++++++++--- synapse/rest/client/v1/register.py | 11 +++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 27a6250b1..6d055fd01 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -25,12 +25,17 @@ import urllib2 import yaml -def request_registration(user, password, server_location, shared_secret): +def request_registration(user, password, server_location, shared_secret, admin=False): mac = hmac.new( key=shared_secret, - msg=user, digestmod=hashlib.sha1, - ).hexdigest() + ) + + mac.update(user) + mac.update(password) + mac.update("admin" if admin else "notadmin") + + mac = mac.hexdigest() data = { "user": user, diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index d791d5e07..0eb7490e5 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -324,6 +324,8 @@ class RegisterRestServlet(ClientV1RestServlet): raise SynapseError(400, "Shared secret registration is not enabled") user = register_json["user"].encode("utf-8") + password = register_json["password"].encode("utf-8") + admin = register_json.get("admin", None) # str() because otherwise hmac complains that 'unicode' does not # have the buffer interface @@ -331,11 +333,12 @@ class RegisterRestServlet(ClientV1RestServlet): want_mac = hmac.new( key=self.hs.config.registration_shared_secret, - msg=user, digestmod=sha1, - ).hexdigest() - - password = register_json["password"].encode("utf-8") + ) + want_mac.update(user) + want_mac.update(password) + want_mac.update("admin" if admin else "notadmin") + want_mac = want_mac.hexdigest() if compare_digest(want_mac, got_mac): handler = self.handlers.registration_handler From 651faee698d5ff4806d1e0e7f5cd4c438bf434f1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:30:22 +0100 Subject: [PATCH 2/6] Add an admin option to shared secret registration --- scripts/register_new_matrix_user | 19 ++++++++-- synapse/handlers/register.py | 4 +- synapse/rest/client/v1/register.py | 1 + synapse/storage/registration.py | 61 +++++++++++++++++++----------- 4 files changed, 58 insertions(+), 27 deletions(-) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 6d055fd01..987bf32d1 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -42,6 +42,7 @@ def request_registration(user, password, server_location, shared_secret, admin=F "password": password, "mac": mac, "type": "org.matrix.login.shared_secret", + "admin": admin, } server_location = server_location.rstrip("/") @@ -73,7 +74,7 @@ def request_registration(user, password, server_location, shared_secret, admin=F sys.exit(1) -def register_new_user(user, password, server_location, shared_secret): +def register_new_user(user, password, server_location, shared_secret, admin): if not user: try: default_user = getpass.getuser() @@ -104,7 +105,14 @@ def register_new_user(user, password, server_location, shared_secret): print "Passwords do not match" sys.exit(1) - request_registration(user, password, server_location, shared_secret) + if not admin: + admin = raw_input("Make admin [no]: ") + if admin in ("y", "yes", "true"): + admin = True + else: + admin = False + + request_registration(user, password, server_location, shared_secret, bool(admin)) if __name__ == "__main__": @@ -124,6 +132,11 @@ if __name__ == "__main__": default=None, help="New password for user. Will prompt if omitted.", ) + parser.add_argument( + "-a", "--admin", + action="store_true", + help="Register new user as an admin. Will prompt if omitted.", + ) group = parser.add_mutually_exclusive_group(required=True) group.add_argument( @@ -156,4 +169,4 @@ if __name__ == "__main__": else: secret = args.shared_secret - register_new_user(args.user, args.password, args.server_url, secret) + register_new_user(args.user, args.password, args.server_url, secret, args.admin) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 88c82ba7d..8c3381df8 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -90,7 +90,8 @@ class RegistrationHandler(BaseHandler): password=None, generate_token=True, guest_access_token=None, - make_guest=False + make_guest=False, + admin=False, ): """Registers a new client on the server. @@ -141,6 +142,7 @@ class RegistrationHandler(BaseHandler): # If the user was a guest then they already have a profile None if was_guest else user.localpart ), + admin=admin, ) else: # autogen a sequential user ID diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 0eb7490e5..25d63a0b0 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -345,6 +345,7 @@ class RegisterRestServlet(ClientV1RestServlet): user_id, token = yield handler.register( localpart=user, password=password, + admin=bool(admin), ) self._remove_session(session) defer.returnValue({ diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 5c75dbab5..4999175dd 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -77,7 +77,7 @@ class RegistrationStore(SQLBaseStore): @defer.inlineCallbacks def register(self, user_id, token, password_hash, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None): + create_profile_with_localpart=None, admin=False): """Attempts to register an account. Args: @@ -104,6 +104,7 @@ class RegistrationStore(SQLBaseStore): make_guest, appservice_id, create_profile_with_localpart, + admin ) self.get_user_by_id.invalidate((user_id,)) self.is_guest.invalidate((user_id,)) @@ -118,6 +119,7 @@ class RegistrationStore(SQLBaseStore): make_guest, appservice_id, create_profile_with_localpart, + admin, ): now = int(self.clock.time()) @@ -125,29 +127,42 @@ class RegistrationStore(SQLBaseStore): try: if was_guest: - txn.execute("UPDATE users SET" - " password_hash = ?," - " upgrade_ts = ?," - " is_guest = ?" - " WHERE name = ?", - [password_hash, now, 1 if make_guest else 0, user_id]) + txn.execute( + "UPDATE users SET" + " password_hash = ?," + " upgrade_ts = ?," + " is_guest = ?," + " admin = ?" + " WHERE name = ?", + (password_hash, now, 1 if make_guest else 0, admin, user_id,) + ) + self._simple_update_one_txn( + txn, + "users", + keyvalues={ + "name": user_id, + }, + updatevalues={ + "password_hash": password_hash, + "upgrade_ts": now, + "is_guest": 1 if make_guest else 0, + "appservice_id": appservice_id, + "admin": admin, + } + ) else: - txn.execute("INSERT INTO users " - "(" - " name," - " password_hash," - " creation_ts," - " is_guest," - " appservice_id" - ") " - "VALUES (?,?,?,?,?)", - [ - user_id, - password_hash, - now, - 1 if make_guest else 0, - appservice_id, - ]) + self._simple_insert_txn( + txn, + "users", + values={ + "name": user_id, + "password_hash": password_hash, + "creation_ts": now, + "is_guest": 1 if make_guest else 0, + "appservice_id": appservice_id, + "admin": admin, + } + ) except self.database_engine.module.IntegrityError: raise StoreError( 400, "User ID already taken.", errcode=Codes.USER_IN_USE From 4adf93e0f743338c929860a1384beabeae9fded8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:34:25 +0100 Subject: [PATCH 3/6] Fix for postgres --- synapse/storage/registration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 4999175dd..232dcfd9e 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -147,7 +147,7 @@ class RegistrationStore(SQLBaseStore): "upgrade_ts": now, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, - "admin": admin, + "admin": 1 if admin else 0, } ) else: @@ -160,7 +160,7 @@ class RegistrationStore(SQLBaseStore): "creation_ts": now, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, - "admin": admin, + "admin": 1 if admin else 0, } ) except self.database_engine.module.IntegrityError: From be3548f7e14f411b0bb4d176ea0977672ed58252 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:46:51 +0100 Subject: [PATCH 4/6] Remove spurious txn --- synapse/storage/registration.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 232dcfd9e..0a6834149 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -127,15 +127,6 @@ class RegistrationStore(SQLBaseStore): try: if was_guest: - txn.execute( - "UPDATE users SET" - " password_hash = ?," - " upgrade_ts = ?," - " is_guest = ?," - " admin = ?" - " WHERE name = ?", - (password_hash, now, 1 if make_guest else 0, admin, user_id,) - ) self._simple_update_one_txn( txn, "users", From 0da24cac8bde47961396f7da774d8dc8ed847107 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 Jul 2016 11:04:44 +0100 Subject: [PATCH 5/6] Add null separator to hmac --- scripts/register_new_matrix_user | 2 ++ synapse/rest/client/v1/register.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 987bf32d1..12ed20d62 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -32,7 +32,9 @@ def request_registration(user, password, server_location, shared_secret, admin=F ) mac.update(user) + mac.update("\x00") mac.update(password) + mac.update("\x00") mac.update("admin" if admin else "notadmin") mac = mac.hexdigest() diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 25d63a0b0..83872f5f6 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -336,7 +336,9 @@ class RegisterRestServlet(ClientV1RestServlet): digestmod=sha1, ) want_mac.update(user) + want_mac.update("\x00") want_mac.update(password) + want_mac.update("\x00") want_mac.update("admin" if admin else "notadmin") want_mac = want_mac.hexdigest() From 76b18df3d95cd881017a9aa5c8473409928faecd Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 Jul 2016 11:16:10 +0100 Subject: [PATCH 6/6] Check that there are no null bytes in user and passsword --- synapse/rest/client/v1/register.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 83872f5f6..ce7099b18 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -327,6 +327,12 @@ class RegisterRestServlet(ClientV1RestServlet): password = register_json["password"].encode("utf-8") admin = register_json.get("admin", None) + # Its important to check as we use null bytes as HMAC field separators + if "\x00" in user: + raise SynapseError(400, "Invalid user") + if "\x00" in password: + raise SynapseError(400, "Invalid password") + # str() because otherwise hmac complains that 'unicode' does not # have the buffer interface got_mac = str(register_json["mac"])