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).

Joseph, thanks for the detailed problem report(s).
We are starting to discuss how to deal with the on
the development list.

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 password.py.

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 password.py

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

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.

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.

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.

Ralf

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.

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
password.py alone....

Attached is a patch (against trunk r4592) which adds password hash 
migration.

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 
Password.deprecated_schemes.

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 !

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 
passwords.py is test vector I took from rfc3962 
(http://tools.ietf.org/html/rfc3962#appendix-B), 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 
10000)
* 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.

Oops - forgot the actual attachment for my last message.

Thanks Eli,
I've committed this and added a small regression test.