From 7cd1a31fa16d0a90a70b74c22dd0baaa7c662041 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Apr 10 2017 12:16:02 +0000 Subject: [PATCH 1/4] Fix UnicodeEncode on entering non-ascii password The above changes encode the password to ``UTF_8`` to make the password compatible for ascii as well as non-ascii character. Signed-off-by: Farhaan Bukhsh --- diff --git a/pagure/lib/login.py b/pagure/lib/login.py index b821e20..937e332 100644 --- a/pagure/lib/login.py +++ b/pagure/lib/login.py @@ -51,7 +51,8 @@ def get_session_by_visitkey(session, sessionid): def generate_hashed_value(password): """ Generate hash value for password """ - return '$2$' + bcrypt.hashpw(to_unicode(password), bcrypt.gensalt()) + return '$2$' + bcrypt.hashpw(to_unicode(password).encode('UTF_8'), + bcrypt.gensalt()) def check_password(entered_password, user_password, seed=None): @@ -65,7 +66,8 @@ def check_password(entered_password, user_password, seed=None): _, version, user_password = user_password.split('$', 2) if version == '2': - password = bcrypt.hashpw(to_unicode(entered_password), user_password) + password = bcrypt.hashpw(to_unicode(entered_password).encode('UTF_8'), + user_password) elif version == '1': password = '%s%s' % (to_unicode(entered_password), seed) From 28b8ff1f8e21734250bde9fc94254b83ed041646 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Apr 10 2017 12:16:02 +0000 Subject: [PATCH 2/4] Fix unicode in password and add test After a long discussion with ``@jcline`` and as he pointed out that using ``to_unincode`` is a really bad option for storing password since after unicoding some non-ascii value it starts giving the same character. This could be really harmful for security reasons. The tests are provided to check the same that things don't break on entering any non-ascii characters. Signed-off-by: Farhaan Bukhsh --- diff --git a/pagure/lib/login.py b/pagure/lib/login.py index 937e332..0e8bbae 100644 --- a/pagure/lib/login.py +++ b/pagure/lib/login.py @@ -11,9 +11,9 @@ import random import string +import hashlib import bcrypt -import hashlib import pagure from pagure.lib import model from kitchen.text.converters import to_unicode, to_bytes @@ -51,7 +51,7 @@ def get_session_by_visitkey(session, sessionid): def generate_hashed_value(password): """ Generate hash value for password """ - return '$2$' + bcrypt.hashpw(to_unicode(password).encode('UTF_8'), + return '$2$' + bcrypt.hashpw(password.encode('UTF_8'), bcrypt.gensalt()) @@ -66,7 +66,7 @@ def check_password(entered_password, user_password, seed=None): _, version, user_password = user_password.split('$', 2) if version == '2': - password = bcrypt.hashpw(to_unicode(entered_password).encode('UTF_8'), + password = bcrypt.hashpw(entered_password.encode('UTF_8'), user_password) elif version == '1': diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index c59d50c..9badbb2 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -79,6 +79,15 @@ class PagureFlaskLogintests(tests.Modeltests): 'confirm_password': 'barpass', } + # This has all the data needed + data_non_ascii = { + 'user': 'foo_bar', + 'fullname': 'user foo', + 'email_address': 'bar@foo.com', + 'password': 'ö', + 'confirm_password': 'ö', + } + # Submit this form - Doesn't work since there is no csrf token output = self.app.post('/user/new', data=data) self.assertEqual(output.status_code, 200) @@ -114,9 +123,18 @@ class PagureFlaskLogintests(tests.Modeltests): 'User created, please check your email to activate the account', output.data) + # Submit the form with proper data with password being non-ascii + data_non_ascii['csrf_token'] = csrf_token + output = self.app.post('/user/new', data=data_non_ascii, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + 'User created, please check your email to activate the account', + output.data) + # Check after: items = pagure.lib.search_user(self.session) - self.assertEqual(3, len(items)) + self.assertEqual(4, len(items)) def test_do_login(self): """ Test the do_login endpoint. """ @@ -157,7 +175,7 @@ class PagureFlaskLogintests(tests.Modeltests): self.test_new_user() items = pagure.lib.search_user(self.session) - self.assertEqual(3, len(items)) + self.assertEqual(4, len(items)) # Submit the form with the csrf token - but user not confirmed data['csrf_token'] = csrf_token @@ -179,6 +197,16 @@ class PagureFlaskLogintests(tests.Modeltests): '
', output.data) self.assertIn('Username or password invalid.', output.data) + # User in the DB, csrf provided - but wrong password submitted + # And checking for non-ascii character + data['password'] = 'ö' + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn('Username or password invalid.', output.data) + # When account is not confirmed i.e user_obj != None data['password'] = 'barpass' output = self.app.post('/dologin', data=data, follow_redirects=True) @@ -235,12 +263,45 @@ class PagureFlaskLogintests(tests.Modeltests): '', output.data) + # Login but cannot save the session to the DB due to the missing IP + # address in the flask request + # This has all the data needed + + # Confirm the user so that we can log in + item = pagure.lib.search_user(self.session, username='foo_bar') + self.assertEqual(item.user, 'foo_bar') + self.assertNotEqual(item.token, None) + + # Remove the token + item.token = None + self.session.add(item) + self.session.commit + + # Check the user + item = pagure.lib.search_user(self.session, username='foo_bar') + self.assertEqual(item.user, 'foo_bar') + self.assertEqual(item.token, None) + + data_non_ascii = { + 'username': 'foo_bar', + 'password': 'ö', + } + data_non_ascii['csrf_token'] = csrf_token + print data_non_ascii + output = self.app.post('/dologin', + data=data_non_ascii, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Home - Pagure', output.data) + self.assertIn( + '', output.data) + # I'm not sure if the change was in flask or werkzeug, but in older # version flask.request.remote_addr was returning None, while it # now returns 127.0.0.1 making our logic pass where it used to # partly fail if hasattr(flask, '__version__') and \ - tuple(flask.__version__.split('.')) <= (0,12,0): + tuple(flask.__version__.split('.')) <= (0, 12, 0): self.assertIn( 'Could not set the session in the db, please report ' 'this error to an admin', output.data) @@ -299,7 +360,7 @@ class PagureFlaskLogintests(tests.Modeltests): # now returns 127.0.0.1 making our logic pass where it used to # partly fail if hasattr(flask, '__version__') and \ - tuple(flask.__version__.split('.')) <= (0,12,0): + tuple(flask.__version__.split('.')) <= (0, 12, 0): self.assertIn( 'Could not set the session in the db, please report ' 'this error to an admin', output.data) @@ -317,7 +378,7 @@ class PagureFlaskLogintests(tests.Modeltests): self.test_new_user() items = pagure.lib.search_user(self.session) - self.assertEqual(3, len(items)) + self.assertEqual(4, len(items)) item = pagure.lib.search_user(self.session, username='foouser') self.assertEqual(item.user, 'foouser') self.assertTrue(item.password.startswith('$2$')) From 2d61192a3be7073197500b922324ddc461cbd5e2 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Apr 10 2017 12:16:02 +0000 Subject: [PATCH 3/4] Organize test for non-ascii password As pointed out by ``@jcline`` a separate test is provided for using non-ascii password for user creating a user and logging in pagure. Signed-off-by: Farhaan Bukhsh --- diff --git a/tests/test_pagure_flask_ui_login.py b/tests/test_pagure_flask_ui_login.py index 9badbb2..b2c99ef 100644 --- a/tests/test_pagure_flask_ui_login.py +++ b/tests/test_pagure_flask_ui_login.py @@ -79,15 +79,6 @@ class PagureFlaskLogintests(tests.Modeltests): 'confirm_password': 'barpass', } - # This has all the data needed - data_non_ascii = { - 'user': 'foo_bar', - 'fullname': 'user foo', - 'email_address': 'bar@foo.com', - 'password': 'ö', - 'confirm_password': 'ö', - } - # Submit this form - Doesn't work since there is no csrf token output = self.app.post('/user/new', data=data) self.assertEqual(output.status_code, 200) @@ -123,18 +114,9 @@ class PagureFlaskLogintests(tests.Modeltests): 'User created, please check your email to activate the account', output.data) - # Submit the form with proper data with password being non-ascii - data_non_ascii['csrf_token'] = csrf_token - output = self.app.post('/user/new', data=data_non_ascii, follow_redirects=True) - self.assertEqual(output.status_code, 200) - self.assertIn('Login - Pagure', output.data) - self.assertIn( - 'User created, please check your email to activate the account', - output.data) - # Check after: items = pagure.lib.search_user(self.session) - self.assertEqual(4, len(items)) + self.assertEqual(3, len(items)) def test_do_login(self): """ Test the do_login endpoint. """ @@ -175,7 +157,7 @@ class PagureFlaskLogintests(tests.Modeltests): self.test_new_user() items = pagure.lib.search_user(self.session) - self.assertEqual(4, len(items)) + self.assertEqual(3, len(items)) # Submit the form with the csrf token - but user not confirmed data['csrf_token'] = csrf_token @@ -197,16 +179,6 @@ class PagureFlaskLogintests(tests.Modeltests): '', output.data) self.assertIn('Username or password invalid.', output.data) - # User in the DB, csrf provided - but wrong password submitted - # And checking for non-ascii character - data['password'] = 'ö' - output = self.app.post('/dologin', data=data, follow_redirects=True) - self.assertEqual(output.status_code, 200) - self.assertIn('Login - Pagure', output.data) - self.assertIn( - '', output.data) - self.assertIn('Username or password invalid.', output.data) - # When account is not confirmed i.e user_obj != None data['password'] = 'barpass' output = self.app.post('/dologin', data=data, follow_redirects=True) @@ -263,39 +235,6 @@ class PagureFlaskLogintests(tests.Modeltests): '', output.data) - # Login but cannot save the session to the DB due to the missing IP - # address in the flask request - # This has all the data needed - - # Confirm the user so that we can log in - item = pagure.lib.search_user(self.session, username='foo_bar') - self.assertEqual(item.user, 'foo_bar') - self.assertNotEqual(item.token, None) - - # Remove the token - item.token = None - self.session.add(item) - self.session.commit - - # Check the user - item = pagure.lib.search_user(self.session, username='foo_bar') - self.assertEqual(item.user, 'foo_bar') - self.assertEqual(item.token, None) - - data_non_ascii = { - 'username': 'foo_bar', - 'password': 'ö', - } - data_non_ascii['csrf_token'] = csrf_token - print data_non_ascii - output = self.app.post('/dologin', - data=data_non_ascii, follow_redirects=True) - self.assertEqual(output.status_code, 200) - self.assertIn('Home - Pagure', output.data) - self.assertIn( - '', output.data) - # I'm not sure if the change was in flask or werkzeug, but in older # version flask.request.remote_addr was returning None, while it # now returns 127.0.0.1 making our logic pass where it used to @@ -365,6 +304,159 @@ class PagureFlaskLogintests(tests.Modeltests): 'Could not set the session in the db, please report ' 'this error to an admin', output.data) + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_non_ascii_password(self): + """ Test login and create user functionality when the password is + non-ascii. + """ + + # Check before: + items = pagure.lib.search_user(self.session) + self.assertEqual(2, len(items)) + + # First access the new user page + output = self.app.get('/user/new') + self.assertEqual(output.status_code, 200) + self.assertIn('New user - Pagure', output.data) + self.assertIn( + '', output.data) + + # Create the form to send there + # This has all the data needed + + data = { + 'user': 'foo', + 'fullname': 'user foo', + 'email_address': 'foo@bar.com', + 'password': 'ö', + 'confirm_password': 'ö', + } + + # Submit this form - Doesn't work since there is no csrf token + output = self.app.post('/user/new', data=data) + self.assertEqual(output.status_code, 200) + self.assertIn('New user - Pagure', output.data) + self.assertIn( + '', output.data) + + csrf_token = output.data.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + # Submit the form with the csrf token + data['csrf_token'] = csrf_token + output = self.app.post('/user/new', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('New user - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn('Username already taken.', output.data) + + # Submit the form with another username + data['user'] = 'foobar' + output = self.app.post('/user/new', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('New user - Pagure', output.data) + self.assertIn('Email address already taken.', output.data) + + # Submit the form with proper data + data['email_address'] = 'foobar@foobar.com' + output = self.app.post('/user/new', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + 'User created, please check your email to activate the account', + output.data) + + # Check after: + items = pagure.lib.search_user(self.session) + self.assertEqual(3, len(items)) + + # Checking for the /login page + output = self.app.get('/login/') + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + + # This has all the data needed + data = { + 'username': 'foob_bar', + 'password': 'ö', + } + + # Submit this form - Doesn't work since there is no csrf token + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn('Insufficient information provided', output.data) + + # Submit the form with the csrf token - but invalid user + data['csrf_token'] = csrf_token + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn('Username or password invalid.', output.data) + + # Submit the form with the csrf token - but user not confirmed + data['username'] = "foobar" + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn( + 'Invalid user, did you confirm the creation with the url ' + 'provided by email?', output.data) + + # User in the DB, csrf provided - but wrong password submitted + data['password'] = 'öö' + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn('Username or password invalid.', output.data) + + # When account is not confirmed i.e user_obj != None + data['password'] = 'ö' + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Login - Pagure', output.data) + self.assertIn( + '', output.data) + self.assertIn( + 'Invalid user, did you confirm the creation with the url ' + 'provided by email?', output.data) + + # Confirm the user so that we can log in + item = pagure.lib.search_user(self.session, username='foobar') + self.assertEqual(item.user, 'foobar') + self.assertNotEqual(item.token, None) + + # Remove the token + item.token = None + self.session.add(item) + self.session.commit() + + # Login but cannot save the session to the DB due to the missing IP + # address in the flask request + data['password'] = 'ö' + output = self.app.post('/dologin', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn('Home - Pagure', output.data) + self.assertIn( + '', output.data) + + # Check the user + item = pagure.lib.search_user(self.session, username='foobar') + self.assertEqual(item.user, 'foobar') + self.assertEqual(item.token, None) + def test_confirm_user(self): """ Test the confirm_user endpoint. """ @@ -378,7 +470,7 @@ class PagureFlaskLogintests(tests.Modeltests): self.test_new_user() items = pagure.lib.search_user(self.session) - self.assertEqual(4, len(items)) + self.assertEqual(3, len(items)) item = pagure.lib.search_user(self.session, username='foouser') self.assertEqual(item.user, 'foouser') self.assertTrue(item.password.startswith('$2$')) From e10226a90b198a3da14019cb78ad1b32a2eccbb5 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Apr 10 2017 12:16:02 +0000 Subject: [PATCH 4/4] Add Tests and exception for non-unicode password This commit introduces raising ``ValueError`` exception if the password is not unicoded. And the tests for the same are included. Signed-off-by: Farhaan Bukhsh --- diff --git a/pagure/lib/login.py b/pagure/lib/login.py index 0e8bbae..954983b 100644 --- a/pagure/lib/login.py +++ b/pagure/lib/login.py @@ -5,7 +5,7 @@ Authors: Pierre-Yves Chibon - Farhaan Bukhsh + Farhaan Bukhsh """ @@ -13,10 +13,11 @@ import random import string import hashlib import bcrypt +import six import pagure from pagure.lib import model -from kitchen.text.converters import to_unicode, to_bytes +from kitchen.text.converters import to_bytes from cryptography.hazmat.primitives import constant_time @@ -49,15 +50,32 @@ def get_session_by_visitkey(session, sessionid): def generate_hashed_value(password): - """ Generate hash value for password - """ + ''' Generate hash value for password. + + :arg password: password for which the hash has to be generated. + :type password: str (Python 3) or unicode (Python 2) + :return: a hashed string of characters. + :rtype: an encoded string(bytes). + ''' + if not isinstance(password, six.text_type): + raise ValueError("Password supplied is not unicode text") + return '$2$' + bcrypt.hashpw(password.encode('UTF_8'), bcrypt.gensalt()) def check_password(entered_password, user_password, seed=None): - """ Version checking and returning the password - """ + ''' Version checking and returning the password + + :arg entered_password: password entered by the user. + :type entered_password: str (Python 3) or unicode (Python 2) + :arg user_password: the hashed string fetched from the database. + :return: a Boolean depending upon the entered_password, True if the + password matches + ''' + if not isinstance(entered_password, six.text_type): + raise ValueError("Entered password is not unicode text") + if not user_password.count('$') >= 2: raise pagure.exceptions.PagureException( 'Password of unknown version found in the database' @@ -70,8 +88,8 @@ def check_password(entered_password, user_password, seed=None): user_password) elif version == '1': - password = '%s%s' % (to_unicode(entered_password), seed) - password = hashlib.sha512(password).hexdigest() + password = '%s%s' % (entered_password, seed) + password = hashlib.sha512(password.encode('utf-8')).hexdigest() else: raise pagure.exceptions.PagureException( diff --git a/tests/test_pagure_lib_login.py b/tests/test_pagure_lib_login.py index 935b0e5..81991fd 100644 --- a/tests/test_pagure_lib_login.py +++ b/tests/test_pagure_lib_login.py @@ -48,7 +48,7 @@ class PagureLibLogintests(tests.Modeltests): def test_generate_hashed_value(self): ''' Test pagure.lib.login.generate_hashed_value. ''' - password = pagure.lib.login.generate_hashed_value('foo') + password = pagure.lib.login.generate_hashed_value(u'foo') self.assertTrue(password.startswith('$2$')) self.assertEqual(len(password), 63) @@ -56,25 +56,25 @@ class PagureLibLogintests(tests.Modeltests): ''' Test pagure.lib.login.check_password. ''' # Version 2 - password = pagure.lib.login.generate_hashed_value('foo') + password = pagure.lib.login.generate_hashed_value(u'foo') self.assertTrue( - pagure.lib.login.check_password('foo', password)) + pagure.lib.login.check_password(u'foo', password)) self.assertFalse( - pagure.lib.login.check_password('bar', password)) + pagure.lib.login.check_password(u'bar', password)) # Version 1 - password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None)) + password = '%s%s' % (u'foo', APP.config.get('PASSWORD_SEED', None)) password = '$1$' + hashlib.sha512(password).hexdigest() - self.assertTrue(pagure.lib.login.check_password('foo', password)) - self.assertFalse(pagure.lib.login.check_password('bar', password)) + self.assertTrue(pagure.lib.login.check_password(u'foo', password)) + self.assertFalse(pagure.lib.login.check_password(u'bar', password)) # Invalid password - No version - password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None)) + password = '%s%s' % (u'foo', APP.config.get('PASSWORD_SEED', None)) password = hashlib.sha512(password).hexdigest() self.assertRaises( PagureException, pagure.lib.login.check_password, - 'foo', password + u'foo', password ) # Invalid password - Invalid version @@ -82,15 +82,15 @@ class PagureLibLogintests(tests.Modeltests): self.assertRaises( PagureException, pagure.lib.login.check_password, - 'foo', + u'foo', password ) - password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None)) + password = '%s%s' % (u'foo', APP.config.get('PASSWORD_SEED', None)) password = hashlib.sha512(password).hexdigest() self.assertRaises( PagureException, pagure.lib.login.check_password, - 'foo', password + u'foo', password ) # Invalid password - Invalid version @@ -98,10 +98,27 @@ class PagureLibLogintests(tests.Modeltests): self.assertRaises( PagureException, pagure.lib.login.check_password, - 'foo', + u'foo', password ) + def test_unicode_required(self): + ''' Test to check for non-ascii password + ''' + self.assertRaises( + ValueError, + pagure.lib.login.generate_hashed_value, + u'hunter2'.encode('utf-8') + ) + password = pagure.lib.login.generate_hashed_value(u'foo') + self.assertRaises( + ValueError, + pagure.lib.login.check_password, + u'foo'.encode('utf-8'), + password + ) + + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureLibLogintests) unittest.TextTestRunner(verbosity=2).run(SUITE)