Vanilla 1 is no longer supported or maintained. If you need a copy, you can get it here.
HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Vanilla password security
It troubles me that Vanilla is still using unsalted md5-single-hash for storing passwords. Basically md5 sucks and if a Vanilla database gets owned it would be trivial for an attacker to decode each and every password in it. Because many people tend to re-use passwords, you now have a mass breach of your users' accounts on all kinds of different systems.
Even Wordpress, one of the more security-retarded projects out there, has moved to a better password system. Vanilla should too.
Check out this article for a good overview of what makes a strong password hash. It's also quite funny in a very, very nerdy way.
It wouldn't be hard to patch this into Vanilla. Right now it can't be done as an extension, but then it probably shouldn't be done as an extension. It should be part of the base. Also, I'm open to ideas on how to upgrade the password strength for existing boards. I see a few options:
Even Wordpress, one of the more security-retarded projects out there, has moved to a better password system. Vanilla should too.
Check out this article for a good overview of what makes a strong password hash. It's also quite funny in a very, very nerdy way.
It wouldn't be hard to patch this into Vanilla. Right now it can't be done as an extension, but then it probably shouldn't be done as an extension. It should be part of the base. Also, I'm open to ideas on how to upgrade the password strength for existing boards. I see a few options:
- Check the strength of the hash in the database when a user tries to log in. If it's an old-style hash, blow away every password in the database. Show users a message about "upgraded password security" and force them all to reset. This is fast and effective, but annoying; users hate being forced to do anything.
- Check the strength of the hash in the database when a user tries to log in. If it's an old-style hash, push the hash through the new, secure crypt function and store the "strengthened" hash. This is convenient and fast, but ineffective; your database is a mix of secure and insecure hashes.
- Check the strength of the hash in the database when a user tries to log in. If it's an old-style hash, push every password hash in the database through the new, secure crypt function and store the "strengthened" hashes. This is convenient and effective, but slow; for extremely large boards it might even break the 30 second limit that most web hosts impose on PHP script execution.
0
This discussion has been closed.
Comments
Codinghorror also has a good write up on the subject.
I don't think it should be in 1.1.5 however, that release is too close to get such a crucial feature done right. I would suggest a user-transparent upgrade that happens when they log in, but accounts that haven't been active for more than several months must re-verify with the 'forgot password' function. This way the old unsecure hashes expire, the active users (and administrators) don't notice a thing, and everybody has to use the secure hash in a relatively short time.
Paranoid administrators can run a SQL script to trash everyone's password if they wish.
Phpass worries me a bit being version 0.1. I'll probably feel better after reading it's code.
Personally I would prefer to see 1.1.5 delayed a bit rather than wait another 10 months to get this into 1.1.6. Vanilla isn't Ubuntu, the project isn't bound by an arbitrary release schedule.
As you pointed out, this has been a problem for well over two years. It should be clear that single-hashed md5 isn't anywhere near "industry standard" these days. I'm not trying to be a zealot about this, I just feel that Vanilla is riding the security FAILboat and needs to stop.
I'll take a look at doing the patch.
Also, is it worth the trouble? It will be as secure as the salt key can be. If someone get access to the server, he's got the passwords' hashes and the salt key.
The big deal about phpass is that it uses Blowfish instead of md5. Blowfish is a much more expensive cipher to begin with and the strength of the cipher can be increased infinitely so it will continue to be useful long after the Pentium 9000 has made md5 cracking a weekend project.
Mark is correct in saying that if your database gets owned, "you've got bigger problems than md5 passwords". The thing is, I don't care about "your" problems, I care about MY problems! If Lussumo.com gets owned, I have a serious problem because now I have to change my password on more sites than I can remember.
But it gets worse. If Lussumo get owned, an attacker also has my email address. If my email password is the same as my Lussumo password, they now own every site I've ever logged into that implements a "reset password" function by email.
Thank God I'm smart enough to use a different password for my email. Are you? Are your users?
The stakes are really very high on this. Much higher than the amount of work needed to fix the problem. I'll try to get a patch done this weekend.
to clarity: freeze db activity for a short period, and run a one-time script to apply to new method to the existing hashes and insert salts. then glue the method on to the existing hash routine and start it up again.
ooo, you could even base the # of hash iterations off the salt. eg.
function newhash($oldhash, $salt) //salt = really big integer { $r = $oldhash; for($i = $salt % 50; $i >= 0; $i--) $r = sha1($r.$salt); return $r; }then both the input /and/ the function would be dynamic.edit2: and the verification key for cookies could be randomly generated each login with the IP address as a salt.
...then again, not sure how well that would go over with dynamic IP users...
edit3: /me is editing it into Authenticator class
edit: goddamned char limit... if you guys want I can post them but otherwise I don't feel like splitting it all up.
As long as we are suppose to be compatible with 4.1, you will have to use md5.
For the change, what about doing something like that:
ALTER TABLE Lum_User ADD TempPassword VARCHAR( 32 ) AFTER Password ; SELECT UserID,Password FROM Lum_User WHERE TempPassword IS NULL LIMIT 0 , 10 UPDATE Lum_User SET TempPassword = $NewPassword WHERE UserID = $UserId [...] LOCK TABLES Lum_User WRITE; ALTER TABLE Lum_User DROP Password; ALTER TABLE Lum_User CHANGE TempPassword Password // Only then does vanilla start using the new authenticator by changing a configuration setting UNLOCK TABLES;I haven't written an update utility yet though.
regarding the update, I would alter the tables before updating the data. so, eg., first lock down the forum (could have a configuration option or something), then:
LOCK TABLES `lum_user` WRITE; ALTER TABLE `lum_user` CHANGE `Password` `Password` VARCHAR( 64 ) NULL DEFAULT NULL; ALTER TABLE `lum_user` CHANGE `VerificationKey` `VerificationKey` VARCHAR( 65 ) NULL DEFAULT NULL; ALTER TABLE `lum_user` ADD `Salt` INT UNSIGNED DEFAULT '0' NOT NULL; UNLOCK TABLES;and after that SELECT all the passwords, rehash them with a generated seed, and update. I suppose you wouldn't really need to lock the forum, but it would make it smoother for users attempting to log in during/before the passwords update.add a global salt key, empty by default. It is up to you to create it and to save it.
No global salt. The salt is a nonce, generated randomly every time a new password hash is created.
Add two columns to the user table: NewPassword (what should be length?) and SaltKey (unique, what should be the length? should it be of variable length?). Having a column for new password allow to update the passwords hashes by different stage; first you just wait that they log-in again, regenerate remember-me keys, remove all old password hashes.
Salt is part of the password hash, just like UNIX crypt. Don't add new columns, just extend the length of the current password column. If you really need to tell an old pw from a new one, use the length.
Update UserManager and User classes; to login a user, it would get his/her password hash, new password hash and salt key. If it has to use the old hash, it create the new hash and salt key and remove the old hash.
You do need to update UserManager (and two or three other places as well, I've found) to actually fetch the contents of the password field. But once you have the hash you can compare it to a straight md5 hash (or plaintext, there is code there for backward compatibility with plaintext passwords). If it matches, generate a new phpass hash and store it. If it doesn't match, validate the user-submitted password against the phpass hash. If it still doesn't match, login fails.
Update the installer and the updater for vanilla 0.9.x (should we bother?)
I wouldn't. Can you even download 0.9.x anymore? If the new patch is backwards compatible, don't bother.
Find a way to update vanilla 1.0.x and 1.1.x
See above. This can be made to upgrade invisibly as users log in.
Add a feature to regenerate all users' "remember me" keys.
Hadn't thought of that. Best might be to just blow them away. Users can suffer through the one-time inconvenience of having to log in again.
Add a feature to remove all old hashes and email all password-less users to update their password.
This might be tricky. You could try feeding the existing md5 hashes through phpass. That would work but it might take a long time. I don't know, I haven't done the performance tests myself.
Change the error message so that on a failed login, it might be that vanilla doesn't have their password any more.
If we decide it's best to blow away everyone's password we could add a check on login to see if that user's password is blank. If it's blank, show a "we're so sorry" message and do an automatic password reset by email.
SirNot:
why even bother with the whole 'password' replacement stuff? how about just tacking on the new system to the old; ie. reencrypting the existing hashes with, eg. sha + random (stored) salt, possibly several times. then you've got the added security of a hash mixture, and you save yourself a lot of hassle.
We can do this with phpass as well, just rehash the existing hashes. I think we don't want to use sha1 for the same reason we don't want md5, it's designed to be too fast. There is a fallback mode in phpass if your PHP doesn't have the mcrypt extension, but it iterates md5 + pseudo-random hash like a thousand times.
I know I keep saying phpass. I'm not married to it, I just know the most about it and I know other high-profile projects that use it. We really, really don't want to cook up our own system for this. The best system is one that was a) designed by people who spend lots of time thinking about security, and b) beat on by lots of other smart people. We get both of that in phpass in a convenient drop-in class.
@Sirnot and Squirrel:
Is it ok to regenerate the new hash from a hash? I read something like that the variability of md5(md5($String)) is less than md5($String). I guess it is fine for something like md5(md5($String).$VariableLengthSaltKey). Is it how it will work?
function HashPassword($password, $salt) { $r = md5($password); $cursalt = $salt; for($i = $salt % 10000 + 100; $i >= 0; $i--) { $r = md5($r.(string)$cursalt); $cursalt = ($cursalt + 67343) % 2100000000; } return $r; }(you'd think php would have the decency to automatically use a library if number values got too big, or at least keep int limitations independant of machine type...)but yeah, if we wanted to ditch php users <= 4.1 we could use a cbc blowfish cipher instead. I'm hesitant to start juggling methods depending on php version b/c, especially with an outdated version like 4.1, it's likely a user will upgrade, which would mean we'd have to either update or reset the db yet another time.
I can't believe I was dumb enough not to realize the salt could be sort as part of the password though... I'll have to alter that. regardless we'd still need to update the db to allow for longer passwords.
To distinguish the old passwords from the new is as easy as a query based on the length of the password field. If we can make this change without adding overhead to the database then I think we should.
As I understand it, re-hashing a hash is only a problem if you're worried about collisions. Not really an issue for us. You're right, whatever new hash we settle on probably should just be wrapped around the existing md5 hashes.
@SirNot:
I don't think it's likely that forum admins will be switching server configurations very often, but if we're really worried about portability we could use the phpass "portable" hash method. By default this is 2^8 (256) iterations of salted md5 but we can configure it anywhere from 2^4 up to 2^31. Regardless, it's md5 so it will work with any PHP from 3.0.18 up.
@squirrel: I've looked at phpass, and considering the knowledge that's behind the salt generation it may be a better route to take in the end, but I'd like explore this whole region a little first. you can probably tell by my comment about a diff clone that I always have to try inventing things myself :-P
The migration should be easy.
- Vanilla create a new column, then create a new hash from the old one for each user; while the migration, Vanilla can still use the Password column It doesn't require an explicit lock. At the end it just need to delete the old password column and rename the new one to Password. This part require a lock. Finally we just set $Configuration['PASSWORD_HASH_CLASS'] to the new class and $Configuration['REHASH_PASSWORD'] to 1; It tells a password hash object to hash the password first (with md5) before create or checking a hash.
- or Vanilla delete all user password and send them a reset password email and then alter the password field definition.
We still have to write the migration page and to agree on the salt and hash algorithms. I used SirNot's solution (except that it pack the salt with the hash) so that we can work on the migration page. It doesn't matter for the migration process if the algorithms change before the release of 1.1.5 (as long as it can use the old hash to create the new one).
1.1.5 should probably have 3 password hash classes: the md5 one (the one before migration), a strong and portable one (default migration) and a stronger one.
Here is a build with the last modifications:
vanilla-1.1.5-revamped-security.zip
$Configuration['PASSWORD_HASH_MODULE'] = 'People/phpass_file_name.php'; $Configuration['PASSWORD_HASH_CLASS'] = 'VanillaPasswordHash';now rename People.Class.PasswordHash.php to People.Class.VanillaPasswordHash.php and replace its content with the following:<?php /** * Password hash * ...copyright, yada yada yada... */ /** * Password hash ...big comment... */ class VanillaPasswordHash { /** * @var Context */ var $Context; /** * @var string */ var $Hash; var $phpass; /** * Constructor * * * @param Context $Context * @param string $Hash * @return PasswordHash */ function VanillaPasswordHash(&$Context, $Hash = '') { $this->Context =& $Context; $this->phpass = new PasswordHash(10, 0); $this->Hash = $Hash; } /** * Create a hash from the plain text password * * @param string $Password * @return string */ function Hash($Password) { $this->Hash = $this->phpass->HashPassword($Password); return $this->Hash; } function IsOldStyle($Hash = 0) { if(!$Hash) $Hash = $this->Hash; if(!$Hash) return false; //PHPass prefixes all its hashes with either _ or $ return !($Hash[0] == '_' || $Hash[0] == '$'); } /** * Check a plain text password against the hash * * @param string $Password * @return boolean */ function Check($Password) { //nothing there if(!$this->Hash) return false; //old style? if($this->IsOldStyle()) return md5($Password) == $this->Hash; //supah new style return $this->phpass->CheckPassword($Password, $this->Hash); } /** * @return string */ function __toString() { return $this->Hash; } }and lastly, here is a diff for Authenticator and UserManager:--- People.Class.Authenticator_old.php Wed Sep 3 01:21:00 2008 +++ People.Class.Authenticator.php Thu Sep 4 00:49:12 2008 @@ -65,6 +65,14 @@ if(!$Credentials->PasswordHash->Check($Password)) { return 0; } else { + + //migration... + if($Credentials->PasswordHash->IsOldStyle()) + { + $Credentials->PasswordHash->Hash($Password); + $UserManager->UpdatePassword($Credentials->UserID, $Credentials->PasswordHash); + } + // Update the user's information $this->UpdateLastVisit($Credentials->UserID, $Credentials->VerificationKey); --- People.Class.UserManager_old.php Wed Sep 3 01:21:00 2008 +++ People.Class.UserManager.php Thu Sep 4 00:50:49 2008 @@ -500,6 +500,19 @@ return false; } } + + //migration related + function UpdatePassword($UserID, $PasswordHash) + { + $s = $this->Context->ObjectFactory->NewContextObject($this->Context, 'SqlBuilder'); + $s->SetMainTable('User', 'u'); + + $s->AddFieldNameValue('Password', $PasswordHash->__toString()); + $s->AddWhere('u', 'UserID', '', $UserID, '='); + + return $this->Context->Database->Update($s, 'UserManager', 'UpdatePassword', + 'An error occurred while attempting to update your password.'); + } function CheckVerificationKey($UserID, $VerificationKey) { $UserID = ForceInt($UserID, 0);...that should work. you still need to enlarge the password field though. it's a tad redundant what with a hashing class nested in another, but this allows for easy updates of the phpass file.