RSS LJ

June 11, 2012

Password security update (, , )

by fluffy at 10:17 AM

Thanks to the recent spate of websites' password hash lists getting leaked and a lot of my internal shame over the fact that phpBB2 still uses unsalted md5 for its auth, I finally got around to fixing phpBB2 to salt its damn passwords. It's shameful how many PHP apps don't use the built-in password_hash() function and just use md5() [WARNING: many of the comments on that page show a DANGEROUS lack of understanding of how this works!] instead - because people just don't understand security (even though this is stuff that's been known since the 1940s or something). It's stupid.

Anyway, I debated either replacing it with password_hash() and requiring everyone to change their password, or just wrapping the existing md5 values up in a crypt and making all future passwords a password_hash(md5($password)) instead, and the latter won out.

If (like me) you're running an old phpBB2 instance and don't want to upgrade to phpBB3, I've provided a handy guide for doing this yourself.

March 2, 2015 I updated this to use the newer (available as of PHP 5.5) password_hash/password_verify API that is far superior to crypt.

Places to fix the code

Because the phpBB writers don't understand encapsulation or code reuse, there's a lot of places where they're just doing md5($password) (or variations thereof) instead of calling into a library routine. The following files are ones where I found a need to change things:

  • admin/admin_users.php: Around line 344, replace $password = md5($password) with $password = password_hash(md5($new_password),PASSWORD_DEFAULT)
  • includes/usercp_register.php
    • Around line 373, replace $new_password = md5($new_password) with $new_password = password_hash(md5($new_password),PASSWORD_DEFAULT)
    • Around line 411, replace md5($cur_password) ) != $row['user_password'] with !password_verify(md5($cur_password),$row['user_password'])
  • includes/usercp_sendpassword.php: around line 58, replace md5($user_password) with password_hash(md5($user_password),PASSWORD_DEFAULT)
  • admin/admin_users.php: around line 344, replace md5($password) with password_hash(md5($password),PASSWORD_DEFAULT)
  • login.php: around line 92, replace $row['user_password'] === md5($password) with password_verify(md5($password),$row['user_password'])

This should be all the places in the code that the unsalted md5 hashes are being used or set.

Updating the password database

I wrote this little script that goes into the phpbb root directory and called it fixpasswd.php and ran it once (after backing up my database, of course):

<?

include('config.php');
mysql_connect($dbhost, $dbuser, $dbpasswd);
mysql_select_db($dbname);

mysql_query('ALTER TABLE phpbb_users MODIFY COLUMN user_password VARCHAR(255)');
mysql_query('ALTER TABLE phpbb_users MODIFY COLUMN user_newpasswd VARCHAR(255)');
$q = mysql_query('SELECT user_id, user_password FROM phpbb_users WHERE user_password != \'\' AND user_password NOT LIKE \'$2y$%\'');
while ($row = mysql_fetch_assoc($q)) {

$crypt = password_hash($row['user_password'],PASSWORD_DEFAULT);
echo $row['user_id'] . ' => ' . $crypt . '<br>';
mysql_query('UPDATE phpbb_users SET user_password="' . $crypt . '" where user_id="' . $row['user_id'] . '"') or die(mysql_error());

}

?>

That of course assumes you're using mysql, which you probably are. If not, adapt it for your particular DBMS. I am not the boss of you. (If you don't run this script and would just rather people set new passwords, please note that the ALTER TABLE statements are very important — phpBB by default uses a VARCHAR(32) there, and that isn't long enough to store a proper crypt() result.)

Anyway, it's possible that I missed something, so if someone has trouble with logging in or changing their password or logging in after changing their password or whatever, please let me know.

Comments

#15008 fluffy 08/14/2012 02:38 am
    Oops, there was another field that needed to be modified. Thanks, phpBB. You are such a poorly-written piece of shit. Updating the password fixing script above.
    #16940 fluffy 03/03/2015 11:42 am
      I need to update this thing - crypt() is terrible and you should use password_hash() instead. (It did not exist at the time that I wrote this.)