Roundup Tracker - Issues

Issue 2550688

Password handling security
Type: security Severity: normal
Components: Infrastructure Versions: 1.4
Status: closed fixed
: : ber, elic, joseph_myers, richard, schlatterbeck
Priority: : patch

Created on 2011-02-22 19:59 by joseph_myers, last changed 2011-04-15 08:10 by schlatterbeck.

File name Uploaded Description Edit Remove
pbkdf2.patch elic, 2011-04-13 22:28
obscure_passwords.patch elic, 2011-04-14 00:26
obscurepw_fix_anydbm.patch schlatterbeck, 2011-04-14 12:44 fix for obscure_passwords.patch
password_migration.patch elic, 2011-04-14 18:02
password_rounds_config.patch elic, 2011-04-14 19:17 added password rounds config option
msg4244 Author: [hidden] (joseph_myers) Date: 2011-02-22 19:59
There are some problems with the security of how Roundup handles passwords.

(a) Password hashes default to SHA-1 with no salt.  The absence of a
salt means that precomputed rainbow tables can be used to attack
passwords (in case the same password was used on another system, say) if
the password hash is obtained.

(b) This is only a single iteration of SHA-1, which means that
brute-force searches can test passwords very quickly; good practice is
to use many iterations.

(c) If a user has access to see another user's item page, the history
section shows past password changes - showing the most recent password
as *encrypted*, but all previous passwords as their full hashes.  I
don't think storing past password hashes is a particularly good idea, as
it gives more information to an attacker who gets the database contents
about passwords that might still be used on other systems, but giving
out the past hashes freely to everyone certainly isn't a good idea.

To fix (a) and (b) I'd advise using an existing algorithm for salted
password hashes such as PBKDF2, based on HMAC with some convenient
underlying hash algorithm and a sufficiently large number of iterations
(say 10000) for modern processor speeds (the number of iterations should
be stored in the hash in the database to facilitate increasing the
default value over time).  Various Python implementations of PBKDF2 are
available.  The salt should be at least 64 bits (in general, the number
of salts should be on the order of the square of the number of passwords
that might be hashed using a given scheme, so that most passwords will
need cracking individually without effort cracking one password helping
on another, and 64 bits is a reasonable approximation to the square of
the number of people alive).

To fix (c) I advise both not journaling password changes in future and
showing all passwords in existing journals as *encrypted*, rather than
just the current one.

Ideally the fix for (a) and (b) would include automatically upgrading
passwords to a stronger hash when users log in, though that only makes
sense in conjunction with a fix for (c).
msg4248 Author: [hidden] (ber) Date: 2011-02-25 11:26
Joseph, thanks for the detailed problem report(s).
We are starting to discuss how to deal with the on
the development list.
msg4272 Author: [hidden] (elic) Date: 2011-04-13 22:28
Attached is a patch which addresses points (a) and (b) raised by Joseph. 

This patch adds a support for a new encrypt format based on PBKDF2, and 
makes it the default for new passwords; as well as some other cleanups I 
made in an attempt to move some parts of the password parsing out of 
roundup proper, and into

The new hash has the following:

* It uses the format "{PBKDF2}<rounds>$<salt>$<digest>", with an encoding 
format adapted from what Linux's SHA512-Crypt uses, for simplicity. 
* It defaults to 10000 rounds (I would like to make this configurable via 
config.ini, but can't quite figure out how to get a hold of a config 
instance from inside encryptPassword). 
* For new passwords, it generates a random 20-byte salt (which can be 
easily increased in the future). 
* It uses a 20-byte digest to match the underlying HMAC-SHA1 prf used by 
the PBKDF2 implementation. 
* For the backend, it uses M2Crypto.EVP.pbkdf2 if available, else falls 
back to a pure-python implementation built into

If this looks useful, I'm willing to take a stab writing code to migrate 
existing hashes (though I may have some questions for roundup-devel in 
that case).

- Eli
msg4273 Author: [hidden] (elic) Date: 2011-04-14 00:26
Attached is an unrelated patch which addresses most of point (c).

This patch adds a .dummystr() method to the Password class, which 
returns a string containing the password's scheme, eg "
{SHA}*encrypted*". This patch updates the journal code so dummystr() is 
all that's stored in the password's history for any future password 
changes. It also updates the templating code so dummystr() is all that's 
displayed for past passwords still stored in journal, as well as when 
displaying the current password (for uniformity). 

I would have used the string "*encrypted*", but when stored in the 
journal, that string gets re-hashed when the history was displayed, 
which was somewhat confusing to the user, and wasted processing time. 

The one thing in point (c) this doesn't fix is that some code needs to 
be written to clean up existing journal entries to replace existing hash 
references with "{SHA}*encrypted*" - they won't be displayed on the web 
interface, but currently will remain in db.
msg4277 Author: [hidden] (ber) Date: 2011-04-14 07:15
Eli: Cool, thanks for the contribution. This is highly appreciated!
Now we need someone to review it. This looks like it is a bit over my 
head to do it quickly.
msg4278 Author: [hidden] (schlatterbeck) Date: 2011-04-14 09:52
Eli, thanks a lot for the patches. I'm now looking into some failing
regression tests and if these are only due to asumptions on the default
encryption scheme, I'll fix these and commit.

msg4279 Author: [hidden] (schlatterbeck) Date: 2011-04-14 12:44
OK the first patch is in.
Small change to file479 pbkdf2.patch: We still accept plaintext
passwords (in known_schemes) when parsing encrypted password (e.g. from
database). This way existing databases with plaintext passwords
continue to work (I don't know of any, this would need patching on the
users side) and all regression tests pass.

Does anybody think this is a security issue (I don't, you'll have to be
quite creative to use plaintext storage)?

Concerning the second patch:
- This breaks some of the regression tests, notably import/export
  (only for mysql, and memorydb and not always, seems to be an issue
  with sorting and since the sort order changes it doesn't happen
  everytime), needs further work to make sure we don't try to convert
  the string in the journal to a Password instance.
- It has a small bug for anydbm, we want the obscured value in the
  journal, not in the user instance (!), patch attached.
- after applying the anydbm fix, the regression test for Import/Export
  also fails for memorydb (but interestingly *not* for anydbm).

I'll further look into this when time permits, just documenting
everything here in case someone else wants to contribute.
msg4280 Author: [hidden] (schlatterbeck) Date: 2011-04-14 15:57
Second patch (with some changes) applied.
Now only auto-migration of passwords on logon (then we do have the
plaintext password for migrating to a new scheme) is missing. This
probably should be a config-option and therefore cannot live in alone....
msg4281 Author: [hidden] (elic) Date: 2011-04-14 18:02
Attached is a patch (against trunk r4592) which adds password hash 

It adds a bool flag named "upgrade_passwords_on_web_login" to [main], 
which defaults to "no". If set to set, the LoginAction will automatically 
re-encode any passwords using a scheme listed in 
msg4282 Author: [hidden] (schlatterbeck) Date: 2011-04-14 18:19
Oops. Looks like we implemented the same thing in parallel.
I've just committed and was about to write the following:

Implemented auto-migration to more secure password scheme.
This uses the new config-option 'migrate_passwords' in section 'web'
which is on by default.

I'll look into your patch to see if we take something from it.
Also note that I've made migration the default... and put a note into
the "upgrading" document.

BTW: I'll probably look into the following two things:
- making the number of iterations configurable (by adding a parameter
  to password generation which is given by all callers, I don't see
  another option)
- add regression tests for the new scheme from rfc6070

anything you've already done in this direction (before we again work in
parallel :-)

Thanks a lot for taking the time to contribute these fixes !
msg4283 Author: [hidden] (elic) Date: 2011-04-14 19:14
Haha :) Hope the parallel patch helps. Happy to contribute them!

Related to your additional points -

The "ATHENA.MIT.EDUraeburn" pbkdf2() test vector I added at the bottom of is test vector I took from rfc3962 
(, which has some 
additional test vectors for PBKDF2 (as part of some AES test vectors).

Attached is a patch that adds an iteration config parameter for PBKDF2. I 
haven't tested it as well as the others (not sure that I found all the 
places a password might be created), but I'll hand it off anyways since 
it lays down the basic framework: 

* It adds a "password_pbkdf2_default_rounds" config parameter (default 
* It passes a config object into the Password constructor wherever a new 
password is created; 
* The Password class in turn hands config off to encodePassword, so that 
additional hash configuration options could be added in the future. For 
now, the PBKDF2 encodePassword routine will use the config parameter if 
config is present, or fallback to the hardcoded default.
msg4284 Author: [hidden] (elic) Date: 2011-04-14 19:17
Oops - forgot the actual attachment for my last message.
msg4290 Author: [hidden] (schlatterbeck) Date: 2011-04-15 08:10
Thanks Eli,
I've committed this and added a small regression test.
Date User Action Args
2011-04-15 08:10:46schlatterbecksetmessages: + msg4290
2011-04-14 19:17:32elicsetfiles: + password_rounds_config.patch
messages: + msg4284
2011-04-14 19:14:33elicsetmessages: + msg4283
2011-04-14 18:19:25schlatterbecksetstatus: new -> closed
resolution: fixed
messages: + msg4282
2011-04-14 18:02:15elicsetfiles: + password_migration.patch
messages: + msg4281
2011-04-14 15:57:50schlatterbecksetmessages: + msg4280
2011-04-14 12:44:54schlatterbecksetfiles: + obscurepw_fix_anydbm.patch
messages: + msg4279
2011-04-14 09:52:25schlatterbecksetnosy: + schlatterbeck
messages: + msg4278
2011-04-14 07:15:35bersetnosy: + richard
messages: + msg4277
2011-04-14 00:26:53elicsetfiles: + obscure_passwords.patch
messages: + msg4273
2011-04-13 22:28:49elicsetfiles: + pbkdf2.patch
keywords: + patch
messages: + msg4272
nosy: + elic
2011-02-25 11:26:19bersetnosy: + ber
messages: + msg4248
2011-02-22 19:59:35joseph_myerscreate