users: remove all connected user states upon deletion

- users: fix security vuln with deterministic password_hash

deterministic password_hash enables users to guess the tokens
of existing deleted users, given the user id. we use password_hash
as the key to the signer.
This commit is contained in:
Luna 2018-12-08 19:12:57 -03:00
parent de3c6d00e6
commit ed7b873e0d
2 changed files with 25 additions and 8 deletions

View File

@ -491,9 +491,8 @@ async def _del_from_table(table: str, user_id: int):
async def delete_account(): async def delete_account():
"""Delete own account. """Delete own account.
There isn't any inherent need to dispatch This removes the account from all tables and
events to connected users, so this is mostly forces all currently connected clients to reconnect.
DB operations.
""" """
user_id = await token_check() user_id = await token_check()
@ -524,6 +523,10 @@ async def delete_account():
new_username = f'Deleted User {rand_hex()}' new_username = f'Deleted User {rand_hex()}'
# by using a random hex in password_hash
# we break attempts at using the default '123' password hash
# to issue valid tokens for deleted users.
await app.db.execute(""" await app.db.execute("""
UPDATE users UPDATE users
SET SET
@ -535,10 +538,10 @@ async def delete_account():
flags = 0, flags = 0,
premium_since = NULL, premium_since = NULL,
phone = '', phone = '',
password_hash = '123' password_hash = $2
WHERE WHERE
id = $2 id = $3
""", new_username, user_id) """, new_username, rand_hex(32), user_id)
# remove the user from various tables # remove the user from various tables
await _del_from_table('user_settings', user_id) await _del_from_table('user_settings', user_id)
@ -563,4 +566,19 @@ async def delete_account():
await _del_from_table('member_roles', user_id) await _del_from_table('member_roles', user_id)
await _del_from_table('channel_overwrites', user_id) await _del_from_table('channel_overwrites', user_id)
# after removing the user from all tables, we need to force
# all known user states to reconnect, causing the user to not
# be online anymore.
user_states = app.state_manager.user_states(user_id)
for state in user_states:
# make it unable to resume
app.state_manager.remove(state)
if not state.ws:
continue
# force a close, 4000 should make the client reconnect.
await state.ws.close(4000)
return '', 204 return '', 204

View File

@ -227,7 +227,6 @@ class StateManager:
) )
log.info('made {} shutdown tasks', len(tasks)) log.info('made {} shutdown tasks', len(tasks))
return tasks return tasks
def close(self): def close(self):