Merge pull request #5200 from CodedInternet/password-hashing

Migrate password hashing away from mysql password()

frappe/data/Framework.sql

@@ -215,7 +215,6 @@ CREATE TABLE `__Auth` (
 	`name` VARCHAR(255) NOT NULL,
 	`fieldname` VARCHAR(140) NOT NULL,
 	`password` VARCHAR(255) NOT NULL,
-	`salt` VARCHAR(140),
 	`encrypted` INT(1) NOT NULL DEFAULT 0,
 	PRIMARY KEY (`doctype`, `name`, `fieldname`)
 ) ENGINE=InnoDB ROW_FORMAT=COMPRESSED CHARACTER SET=utf8mb4 COLLATE=utf8mb4_unicode_ci;

frappe/patches.txt

@@ -206,4 +206,6 @@ frappe.patches.v10_0.refactor_social_login_keys
 frappe.patches.v10_0.enable_chat_by_default_within_system_settings
 frappe.patches.v10_0.remove_custom_field_for_disabled_domain
 execute:frappe.delete_doc("Page", "chat")
-frappe.patches.v11_0.rename_standard_reply_to_email_template
+frappe.patches.v10_0.migrate_passwords_passlib
+frappe.patches.v11_0.drop_column_apply_user_permissions
+frappe.patches.v11_0.rename_standard_reply_to_email_template

frappe/patches/v10_0/migrate_passwords_passlib.py

@@ -0,0 +1,19 @@
+import frappe
+from frappe.utils.password import LegacyPassword
+
+
+def execute():
+	all_auths = frappe.db.sql("""SELECT `name`, `password`, `salt` FROM `__Auth`
+		WHERE doctype='User' AND `fieldname`='password'""",
+		as_dict=True)
+
+	for auth in all_auths:
+		if auth.salt and auth.salt != "":
+			pwd = LegacyPassword.hash(auth.password, salt=auth.salt.encode('UTF-8'))
+			frappe.db.sql("""UPDATE `__Auth` SET `password`=%(pwd)s, `salt`=NULL
+				WHERE `doctype`='User' AND `fieldname`='password' AND `name`=%(user)s""",
+				{'pwd': pwd, 'user': auth.name})
+
+	frappe.reload_doctype("User")
+
+	frappe.db.sql_ddl("""ALTER TABLE `__Auth` DROP COLUMN `salt`""")

frappe/tests/test_password.py

@@ -3,7 +3,7 @@
 from __future__ import unicode_literals
 import frappe
 import unittest
-from frappe.utils.password import update_password, check_password
+from frappe.utils.password import update_password, check_password, passlibctx
 
 class TestPassword(unittest.TestCase):
 	def setUp(self):
@@ -52,14 +52,14 @@ class TestPassword(unittest.TestCase):
 
 		update_password(user, new_password)
 
-		auth = frappe.db.sql('''select `password`, `salt` from `__Auth`
+		auth = frappe.db.sql('''select `password` from `__Auth`
 			where doctype='User' and name=%s and fieldname="password"''', user, as_dict=True)[0]
 
+		# is not plain text
 		self.assertTrue(auth.password != new_password)
-		self.assertTrue(auth.salt)
 
-		# stored password = password(plain_text_password + salt)
-		self.assertEqual(frappe.db.sql('select password(concat(%s, %s))', (new_password, auth.salt))[0][0], auth.password)
+		# is valid hashing
+		self.assertTrue(passlibctx.verify(new_password, auth.password))
 
 		self.assertTrue(check_password(user, new_password))
 

frappe/utils/password.py

@@ -2,10 +2,41 @@
 # MIT License. See license.txt
 
 from __future__ import unicode_literals
+import string
 import frappe
 from frappe import _
 from frappe.utils import cstr, encode
 from cryptography.fernet import Fernet, InvalidToken
+from passlib.hash import pbkdf2_sha256, mysql41
+from passlib.registry import register_crypt_handler
+from passlib.context import CryptContext
+
+
+class LegacyPassword(pbkdf2_sha256):
+	name = "frappe_legacy"
+	ident = "$frappel$"
+
+	def _calc_checksum(self, secret):
+		# check if this is a mysql hash
+		# it is possible that we will generate a false positive if the users password happens to be 40 hex chars proceeded
+		# by an * char, but this seems highly unlikely
+		if not (secret[0] == "*" and len(secret) == 41 and all(c in string.hexdigits for c in secret[1:])):
+			secret = mysql41.hash(secret + self.salt)
+		return super(LegacyPassword, self)._calc_checksum(secret)
+
+
+register_crypt_handler(LegacyPassword, force=True)
+passlibctx = CryptContext(
+	schemes=[
+		"pbkdf2_sha256",
+		"argon2",
+		"frappe_legacy",
+	],
+	deprecated=[
+		"frappe_legacy",
+	],
+)
+
 
 def get_decrypted_password(doctype, name, fieldname='password', raise_exception=True):
 	auth = frappe.db.sql('''select `password` from `__Auth`
@@ -27,24 +58,19 @@ def set_encrypted_password(doctype, name, pwd, fieldname='password'):
 def check_password(user, pwd, doctype='User', fieldname='password'):
 	'''Checks if user and password are correct, else raises frappe.AuthenticationError'''
 
-	auth = frappe.db.sql("""select name, `password`, salt from `__Auth`
-		where doctype=%(doctype)s and name=%(name)s and fieldname=%(fieldname)s and encrypted=0
-		and (
-			(salt is null and `password`=password(%(pwd)s))
-			or `password`=password(concat(%(pwd)s, salt))
-		)""",{ 'doctype': doctype, 'name': user, 'fieldname': fieldname, 'pwd': pwd }, as_dict=True)
+	auth = frappe.db.sql("""select name, `password` from `__Auth`
+		where doctype=%(doctype)s and name=%(name)s and fieldname=%(fieldname)s and encrypted=0""",
+		{'doctype': doctype, 'name': user, 'fieldname': fieldname}, as_dict=True)
 
-	if not auth:
-		raise frappe.AuthenticationError('Incorrect User or Password')
-
-	salt = auth[0].salt
-	if not salt:
-		# sets salt and updates password
-		update_password(user, pwd, doctype, fieldname)
+	if not auth or not passlibctx.verify(pwd, auth[0].password):
+		raise frappe.AuthenticationError(_('Incorrect User or Password'))
 
 	# lettercase agnostic
 	user = auth[0].name
 
+	if not passlibctx.needs_update(auth[0].password):
+		update_password(user, pwd, doctype, fieldname)
+
 	return user
 
 def update_password(user, pwd, doctype='User', fieldname='password', logout_all_sessions=False):
@@ -57,12 +83,12 @@ def update_password(user, pwd, doctype='User', fieldname='password', logout_all_
 		:param fieldname: fieldname (in given doctype) (for encryption)
 		:param logout_all_session: delete all other session
 	'''
-	salt = frappe.generate_hash()
-	frappe.db.sql("""insert into __Auth (doctype, name, fieldname, `password`, salt, encrypted)
-		values (%(doctype)s, %(name)s, %(fieldname)s, password(concat(%(pwd)s, %(salt)s)), %(salt)s, 0)
+	hashPwd = passlibctx.hash(pwd)
+	frappe.db.sql("""insert into __Auth (doctype, name, fieldname, `password`, encrypted)
+		values (%(doctype)s, %(name)s, %(fieldname)s, %(pwd)s, 0)
 		on duplicate key update
-			`password`=password(concat(%(pwd)s, %(salt)s)), salt=%(salt)s, encrypted=0""",
-		{ 'doctype': doctype, 'name': user, 'fieldname': fieldname, 'pwd': pwd, 'salt': salt })
+			`password`=%(pwd)s, encrypted=0""",
+		{'doctype': doctype, 'name': user, 'fieldname': fieldname, 'pwd': hashPwd})
 
 	# clear all the sessions except current
 	if logout_all_sessions:
@@ -95,7 +121,6 @@ def create_auth_table():
 			`name` VARCHAR(255) NOT NULL,
 			`fieldname` VARCHAR(140) NOT NULL,
 			`password` VARCHAR(255) NOT NULL,
-			`salt` VARCHAR(140),
 			`encrypted` INT(1) NOT NULL DEFAULT 0,
 			PRIMARY KEY (`doctype`, `name`, `fieldname`)
 		) ENGINE=InnoDB ROW_FORMAT=COMPRESSED CHARACTER SET=utf8mb4 COLLATE=utf8mb4_unicode_ci""")

requirements.txt

@@ -47,6 +47,7 @@ googlemaps
 mycli
 braintree
 future
+passlib
 google-api-python-client
 google-auth
 google-auth-httplib2