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
This discussion has been closed.
Comments
the db will still have old md5 hash, but we can leave for Vanilla 1.1.6 or for an extension the job to delete all old md5 hash.
No modification to phpass, no wrapper around it. Pulls all password checking and updating code into the Authenticator class where it belongs. Passwords are updated in place when users log in. Keeps compatibility with (very old) plaintext passwords.
It uses the portable (md5-based) hashes instead of Blowfish. I know I said a bunch of stuff about how awesome Blowfish is but Dinoboff is right, we can't count on it. The phpass md5 hashes are still many orders of magnitude better than what we use now.
Execute the following command in your vanilla-1.1.5-rc2 directory:
patch -p1 < phpass.diffI haven't tested in at all yet. Sorry about the inevitable typos.
http://www.digitalsquirrel.com/vanilla/phpass.diff
http://www.digitalsquirrel.com/vanilla/phpass.diff
I also wrote an upgrader. Check the end of appg/settings.php. I couldn't find a better place for it.
Try the last version:
http://dl.getdropbox.com/u/83967/vanilla-1.1.5-revamped-security.zip
For the modifications to PasswordHash, they are just in the constructor, they would be easy to track. I could just extends it. But since I wasn't sure the settings are that useful (Can you change the settings after some password has been created?) I might just remove them.
I released rc3 to get feedback. Thanks to subversion (TortoiseSVN), changes are very easy to roll back.
I still haven't heard why it's so important to get 1.1.5 out by a specific date, but again, it's not my decision to make.
I appreciate the diffs but they're not what I asked for. I'm trying to figure out what changes were made just for the password issue since that issue has had less testing than anything else.
I want to help but I have no idea what's going on here. Last week you didn't even want this change in 1.1.5. I understand the issue of making a change like this so close to a release so I tried to propose a patch with as few code changes as possible. But a few days later and now we're reviewing what looks like a complete overhaul of the Authenticator class with half of its functionality shifted to the UserManager class.
Where is this coming from? It wasn't the fix I suggested. What makes this the best fix for 1.1.5?
I wasn't worried about custom Authenticators because I didn't think they were officially supported. When did that change?
- Authenticator::Authenticate($Username, $Password, $PersistentSession)
- Authenticator::DeAuthenticate()
- Authenticator::GetIdentity()
Vanilla expect them and no more. (I think it should also manage the creation and modification of the credentials, instead of having them in the UserManager; UserManager for the credentials part should only be responsible of saving them in the user table - in the case on the default authenticator).
Sirnot's patch, yours and mine change them or add some methods and it could affect some forum.
Authenticator::Authenticate()
UserManager::ChangePassword()
UserManager::CreateUser()
UserManager::ResetPassword()
If the goal is to fix the password encoding, do it as gently as possible. It can be done without making this change to ObjectFactory which has nothing to do with password encoding. If you're going to drop changes like that between release candidates then please don't tell me that 1.1.5 is being pushed back "just for this fix".
I like the changes you're proposing to Authenticator and UserManager but they are way, way more than what is needed to fix this issue.
With the following changes they would only save the password with the new hash method if the authenticator has been updated:
Index: People.Class.Authenticator.php =================================================================== --- People.Class.Authenticator.php (revision 115) +++ People.Class.Authenticator.php (working copy) @@ -130,6 +130,14 @@ return $UserID; } + function HashPassword($Password) { + $PasswordHash = $this->Context->ObjectFactory->NewObject( + $this->Context, 'PasswordHash', + $this->Context->Configuration['PASSWORD_HASH_ITERATION'], + $this->Context->Configuration['PASSWORD_HASH_PORTABLE']); + return $PasswordHash->HashPassword($Password); + } + // All methods below this point are specific to this authenticator and // should not be treated as interface methods. The only required interface // properties and methods appear above. Index: People.Class.UserManager.php =================================================================== --- People.Class.UserManager.php (revision 115) +++ People.Class.UserManager.php (working copy) @@ -579,12 +579,19 @@ return $this->Context->Database->Select($s, $this->Name, 'GetUserSearch', 'An error occurred while retrieving search results.'); } + /** + * Create a password hash. + * + * @deprecated + * @param string $Password + * @return string + */ function HashPassword($Password) { - $PasswordHash = $this->Context->ObjectFactory->NewObject( - $this->Context, 'PasswordHash', - $this->Context->Configuration['PASSWORD_HASH_ITERATION'], - $this->Context->Configuration['PASSWORD_HASH_PORTABLE']); - return $PasswordHash->HashPassword($Password); + if (method_exists($this->Context->Authenticator, 'HashPassword')) { + return $this->Context->Authenticator->HashPassword($Password); + } else { + return md5($Password); + } } function GetSearchBuilder($Search) {