There are a few things that can / should be done.

There are several types of hashes (b-something) that are explicitly designed to be "slow" to hinder brute force attempts. Some versions of PHP support this, though I'm not sure if all do. Though oftentimes the hash can be added through a pure-PHP library.

Failing that, you can just md5() 1000 times or something.

The salt should be different for each user account, and maybe even change on every password change (why not). Using a salt on prevents me from using a list of pre-compiled password hashes against the concrete5.org user database -- I have to know the salt (which is assumed) and then try to match against a list of passwords. But, if all passwords use the same salt, once I crack one user's password, I then can trivially check all other users for the same password. (I mention concrete5.org cause it's got 100,000+ users and I guarantee a lot of reused passwords.)

There shouldn't be limits on password length or symbols. The hash is a standardized length, even if someone's password is a chapter from a book.

I've taken a first stab at resolving this issue:

https://github.com/bdsl/concrete5/commit/217a24a96d1522f013c1a7f5971...

I'll do some more code review and testing in the next couple of days if I can, and then send a pull request. If anyone wants to take a look and let me know what they think in the meantime I'd welcome feedback.

I used phpass, which automatically generates a random salt for each password.

I decided not to use the site wide PASSWORD_SALT. I think it may be useful for site administrators to be able to restore a site from a DB backup even if they lose the config file, and I think the chance of an attacker getting a copy of the DB but not being able to get the config file is relativly low. I also wanted to stay as close as I could to the instructions in the manual for phpass.

However if think it would be safe and easy to switch this to use the PASSWORD_SALT if that's wanted.

I noticed that PASSWORD_SALT is used for a few things other than passwords, including being an encryption key for database backups, so it still exists in my fork, although its now a bit misnamed. I'm not sure whether it would be worth renaming it.

Existing passwords that were generated in a previous version of concrete5 will be automatically upgraded to use the more secure hashing when the user logs in.

I increased the maximum password length from 64 characters to 128 characters, and I removed the rule that said "A password may not contain ", \', >, <, or any spaces". I'm not sure why either of these restrictions existed.

LGTM.

I agree that PASSWORD_SALT isn't necessary as each password in PHP pass is inherently (and individually) salted. (This is based on the "best case" phppass algorithm... I don't know if that's always the case during fallback.) With that being said, it wouldn't break (or weaken) anything to use it, either.

I also agree with the restrictions, and I'm not sure why they ever existed. Even the length -- with hashing (even md5), you could use a book chapter as a password and it'd still play well with the database.

Pull request sent:

https://github.com/concrete5/concrete5/pull/1578...

It's not exactly the same commit as I posted above, because I adjusted the way I waste time in the case a username isn't found in the login function, adjusted formatting very slightly, and rebased my work on top of the latest master branch.

I agree that the current password hashing in concrete5 is pretty terrible — it's just MD5 with a single site-wide 'salt' value. If someone gets a copy of the database from a Concrete5 installation they might be able to easily crack a lot of the passwords, especially if they also get the configuration file with the 'salt'.

I think md5 with sha1 one is a bad idea, as is inventing anything ourselves. We also really shouldn't use the suggestion from http://pbeblog.wordpress.com, where a blogger has invented their own scheme, which I can see several problems with myself. New security inventions should be in research papers, not production software.

Instead we should follow the accepted best practice, which currently means hashing passwords with either PBKDF2, bcrypt, or scrypt. There's a good explanation at http://security.stackexchange.com/a/31846/27677...

If someone writes a good patch to implement more secure password hashing, would the Concrete5 maintainers be willing to accept a pull request?

You're right. Something like phpass would be best.

In my defense, I don't think iterations is exactly inventing something new. In fact, that's what phpass falls back to.

I've also been thinking about securing the password page with an md5 implementation of javascript. This would prevent (in those cases where JS is available) a cleartext password from ever leaving the browser, which seems to be a good thing. It's trivial to capture cleartext (non ssl) password= POSTs from an unsecured wifi*. That could be workable if md5 is used, but not so much with phpass, though that's not a good reason to move from c5's security-vulnerable current md5() method.


* You still have the cookie, which is almost as easy to capture, but at least you haven't given out your password.

Right, I was more thinking about the link to pbeblog.wordpress.com as a new invention, rather than your suggestion.

I do think running md5 on the client is problematic, because then the hash effectively becomes the password, and someone who captures the hash off the wire (or off the database) can send that same hash to the server to log in to the account. I wouldn't try to do anything other than HTTPS to protect passwords on the wire.

Yes, we definitely would. some thoughts on that:

1. I'm leery of re-running any kind of hashing functions more than once. From what I've read that's actually potentially detrimental to both security and performance.
2. That goes for using two different hashing schemes as well.
3. We'd love to (in addition to PASSWORD_SALT sticking around) move to per-user salts as well.
4. We'd like to move to phpass as well.

We haven't been able to implement either 3 or 4 yet, unfortunately. However 5.7 would be a great opportunity for this. We'd be happy to accept a pull request that did one or both of these things.

Sounds good. I'll try to find some time to work on getting PHPPASS into 5.7 then and send a pull request. I think it will still need to keep the existing system for legacy accounts when people upgrade from an older version of Concrete5. When each user logs in their password hash can be upgraded to the modern hash while we have the plain text password in memory.

The existing PASSWORD_SALT is probably only useful if an attacker can read the DB but not the application code. I think normally DB servers are expected to be slightly more secure than web servers so a lot of applications don't bother to protect against this scenario. I'm not sure whether it will work well with PHPPASS or not. Why were you keen to keep it?

One option during the upgrade process might be to take the PASSWORD_SALT out of the config file and copy it into each users record in the DB.

I dunno – just seems like a good idea to rely on something in the database (the per user salt) as well as something hidden from the database (the global constant.) Obviously the per user salt is a good idea, I think it'd be good to keep the global salt around as well, although a discussion around whether that's necessary or not can certainly happen.

Using the 'Portable PHP password hashing framework' from
http://www.openwall.com/phpass/... might be a good way to do this. It's public domain code.

(This is a copy of an old bug submitted against 5.4.2.2 by scalait. I didn't write the description - I just confirmed that the issue still exists.)

This issue is now fixed in the latest development version of Concrete5 on github - seehttps://github.com/concrete5/concrete5/commit/415725ce6a84dcb39bd2e1...

and according to Andrew Embler it will probably be fixed in the 5.6.2.2 release.

Yep! Thanks again @bdsl. This is great.

FYI: This will probably be officially 5.6.3, but we are shooting for a release very soon. Ideally sometime this week.