From 22959bf39889e7e6e77e8d9cd28bb823f1d7608e Mon Sep 17 00:00:00 2001 From: schlatterbeck Date: Thu, 14 Apr 2011 18:10:58 +0000 Subject: [PATCH] Add new config-option 'migrate_passwords' in section 'web' to auto-migrate passwords at web-login time. Default for the new option is "yes" so if you don't want that passwords are auto-migrated to a more secure password scheme on user login, set this to "no" before running your tracker(s) after the upgrade. git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4593 57a73879-2fb5-44c3-a270-3262357dd7e2 --- CHANGES.txt | 5 +++++ doc/upgrading.txt | 7 +++++++ roundup/cgi/actions.py | 16 +++++++++----- roundup/configuration.py | 4 ++++ roundup/password.py | 45 +++++++++++++++++++++++++++------------- test/test_cgi.py | 24 +++++++++++++++++++++ 6 files changed, 82 insertions(+), 19 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8e16754..4fe82cf 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -79,6 +79,11 @@ Fixed: - Fix Password handling security issue2550688 (thanks Joseph Myers for reporting and Eli Collins for fixing) -- this fixes all observations by Joseph Myers except for auto-migration of existing passwords. +- Add new config-option 'migrate_passwords' in section 'web' to + auto-migrate passwords at web-login time. Default for the new option + is "yes" so if you don't want that passwords are auto-migrated to a + more secure password scheme on user login, set this to "no" before + running your tracker(s) after the upgrade. 2010-10-08 1.4.16 (r4541) diff --git a/doc/upgrading.txt b/doc/upgrading.txt index 43051a1..a1bd77f 100644 --- a/doc/upgrading.txt +++ b/doc/upgrading.txt @@ -16,6 +16,13 @@ steps. Migrating from 1.4.x to 1.4.17 ============================== +There is a new config-option 'migrate_passwords' in section 'web' to +auto-migrate passwords at web-login time to a more secure storage +scheme. Default for the new option is "yes" so if you don't want that +passwords are auto-migrated to a more secure password scheme on user +login, set this to "no" before running your tracker(s) after the +upgrade. + Searching now requires either read-permission without a check method, or you will have to add a "Search" permission for a class or a list of properties for a class (if you want to allow searching). For the classic diff --git a/roundup/cgi/actions.py b/roundup/cgi/actions.py index 40bbfda..41f5979 100755 --- a/roundup/cgi/actions.py +++ b/roundup/cgi/actions.py @@ -1005,12 +1005,18 @@ class LoginAction(Action): raise exceptions.LoginError(self._( "You do not have permission to login")) - def verifyPassword(self, userid, password): - '''Verify the password that the user has supplied''' - stored = self.db.user.get(userid, 'password') - if password == stored: + def verifyPassword(self, userid, givenpw): + '''Verify the password that the user has supplied. + Optionally migrate to new password scheme if configured + ''' + db = self.db + stored = db.user.get(userid, 'password') + if givenpw == stored: + if db.config.WEB_MIGRATE_PASSWORDS and stored.needs_migration(): + db.user.set(userid, password=password.Password(givenpw)) + db.commit() return 1 - if not password and not stored: + if not givenpw and not stored: return 1 return 0 diff --git a/roundup/configuration.py b/roundup/configuration.py index 8b86262..9a8d165 100644 --- a/roundup/configuration.py +++ b/roundup/configuration.py @@ -579,6 +579,10 @@ SETTINGS = ( "Setting this option makes Roundup display error tracebacks\n" "in the user's browser rather than emailing them to the\n" "tracker admin."), + (BooleanOption, "migrate_passwords", "yes", + "Setting this option makes Roundup migrate passwords with\n" + "an insecure password-scheme to a more secure scheme\n" + "when the user logs in via the web-interface."), )), ("rdbms", ( (Option, 'name', 'roundup', diff --git a/roundup/password.py b/roundup/password.py index 545baeb..92ada54 100644 --- a/roundup/password.py +++ b/roundup/password.py @@ -116,6 +116,25 @@ class PasswordValueError(ValueError): """ The password value is not valid """ pass +def pbkdf2_unpack(pbkdf2): + """ unpack pbkdf2 encrypted password into parts, + assume it has format "{rounds}${salt}${digest} + """ + if isinstance(pbkdf2, unicode): + pbkdf2 = pbkdf2.encode("ascii") + try: + rounds, salt, digest = pbkdf2.split("$") + except ValueError: + raise PasswordValueError, "invalid PBKDF2 hash (wrong number of separators)" + if rounds.startswith("0"): + raise PasswordValueError, "invalid PBKDF2 hash (zero-padded rounds)" + try: + rounds = int(rounds) + except ValueError: + raise PasswordValueError, "invalid PBKDF2 hash (invalid rounds)" + raw_salt = h64decode(salt) + return rounds, salt, raw_salt, digest + def encodePassword(plaintext, scheme, other=None): """Encrypt the plaintext password. """ @@ -123,20 +142,7 @@ def encodePassword(plaintext, scheme, other=None): plaintext = "" if scheme == "PBKDF2": if other: - #assume it has format "{rounds}${salt}${digest}" - if isinstance(other, unicode): - other = other.encode("ascii") - try: - rounds, salt, digest = other.split("$") - except ValueError: - raise PasswordValueError, "invalid PBKDF2 hash (wrong number of separators)" - if rounds.startswith("0"): - raise PasswordValueError, "invalid PBKDF2 hash (zero-padded rounds)" - try: - rounds = int(rounds) - except ValueError: - raise PasswordValueError, "invalid PBKDF2 hash (invalid rounds)" - raw_salt = h64decode(salt) + rounds, salt, raw_salt, digest = pbkdf2_unpack(other) else: raw_salt = getrandbytes(20) salt = h64encode(raw_salt) @@ -249,6 +255,17 @@ class Password(JournalPassword): self.password = None self.plaintext = None + def needs_migration(self): + """ Password has insecure scheme or other insecure parameters + and needs migration to new password scheme + """ + if self.scheme != 'PBKDF2': + return True + rounds, salt, raw_salt, digest = pbkdf2_unpack(self.password) + if rounds < 1000: + return True + return False + def unpack(self, encrypted, scheme=None, strict=False): """Set the password info from the scheme: string (the inverse of __str__) diff --git a/test/test_cgi.py b/test/test_cgi.py index b8371bd..17e2f37 100644 --- a/test/test_cgi.py +++ b/test/test_cgi.py @@ -425,6 +425,30 @@ class FormTestCase(unittest.TestCase): ':confirm:password': ''}, 'user', nodeid), ({('user', nodeid): {}}, [])) + def testPasswordMigration(self): + chef = self.db.user.lookup('Chef') + form = dict(__login_name='Chef', __login_password='foo') + cl = self._make_client(form) + # assume that the "best" algorithm is the first one and doesn't + # need migration, all others should be migrated. + for scheme in password.Password.known_schemes[1:]: + pw1 = password.Password('foo', scheme=scheme) + self.assertEqual(pw1.needs_migration(), True) + self.db.user.set(chef, password=pw1) + self.db.commit() + actions.LoginAction(cl).handle() + pw = self.db.user.get(chef, 'password') + self.assertEqual(pw, 'foo') + self.assertEqual(pw.needs_migration(), False) + pw1 = pw + self.assertEqual(pw1.needs_migration(), False) + scheme = password.Password.known_schemes[0] + self.assertEqual(scheme, pw1.scheme) + actions.LoginAction(cl).handle() + pw = self.db.user.get(chef, 'password') + self.assertEqual(pw, 'foo') + self.assertEqual(pw, pw1) + # # Boolean # -- 2.30.2