Skip to content

Commit

Permalink
Resolve mybb#3530 Modern password hashing (mybb#3571)
Browse files Browse the repository at this point in the history
* Add Illuminate\Config\Repository, load old config values

* Add internal password algorithm recognition using Illuminate\Hashing

* add PHPDoc for dynamically accessed objects

* Create basic Config service provider
  • Loading branch information
dvz authored and euantorano committed Mar 23, 2019
1 parent 7ba541a commit d225830
Show file tree
Hide file tree
Showing 14 changed files with 201 additions and 7 deletions.
2 changes: 2 additions & 0 deletions composer.json
Expand Up @@ -22,8 +22,10 @@
},
"require": {
"php": "^7.1",
"illuminate/config": "^5.7",
"illuminate/container": "^5.7",
"illuminate/events": "^5.7",
"illuminate/hashing": "^5.7",
"illuminate/routing": "^5.7",
"twig/twig": "^2.4"
},
Expand Down
5 changes: 5 additions & 0 deletions inc/datahandlers/login.php
Expand Up @@ -333,6 +333,11 @@ function complete_login()
$this->captcha->invalidate_captcha();
}

if (user_password_needs_rehash($user)) {
$data = create_password($this->data['password']);
$db->update_query('users', $data, 'uid=' . (int)$user['uid']);
}

$plugins->run_hooks('datahandler_login_complete_end', $this);

return true;
Expand Down
5 changes: 5 additions & 0 deletions inc/datahandlers/user.php
Expand Up @@ -1085,6 +1085,7 @@ function insert_user()
$this->user_insert_data = array(
"username" => $db->escape_string($user['username']),
"password" => $user['password'],
"password_algorithm" => $user['password_algorithm'],
"salt" => $user['salt'],
"loginkey" => $user['loginkey'],
"email" => $db->escape_string($user['email']),
Expand Down Expand Up @@ -1246,6 +1247,10 @@ function update_user()
{
$this->user_update_data['password'] = $user['password'];
}
if(isset($user['password_algorithm']))
{
$this->user_update_data['password_algorithm'] = $user['password_algorithm'];
}
if(isset($user['salt']))
{
$this->user_update_data['salt'] = $user['salt'];
Expand Down
40 changes: 36 additions & 4 deletions inc/functions_user.php
Expand Up @@ -201,16 +201,25 @@ function create_password($password, $salt = false, $user = false)
}
else
{
// salt may be needed in mechanisms other than passwords
if(!$salt)
{
$salt = generate_salt();
}

$hash = md5(md5($salt).md5($password));
/** @var Illuminate\Hashing\HashManager $hashManager */
$hashManager = \MyBB\app('hash');

$fields = array(
$driverName = $hashManager->getDefaultDriver();

$hash = $hashManager->driver($driverName)->make($password, [
'salt' => $salt,
]);

$fields = array(
'password' => $hash,
'salt' => $salt,
'password_algorithm' => $driverName,
);
}

Expand Down Expand Up @@ -243,12 +252,35 @@ function verify_user_password($user, $password)
}
else
{
$password_fields = create_password($password, $user['salt'], $user);
/** @var Illuminate\Contracts\Hashing\Hasher $hashDriver */
$hashDriver = \MyBB\app('hash')->driver($parameters['user']['password_algorithm']);

return my_hash_equals($user['password'], $password_fields['password']);
return $hashDriver->check($parameters['password'], $parameters['user']['password'], [
'salt' => $user['salt'],
]);
}
}

/**
* Returns whether stored user's password needs to be rehashed.
*
* @param array $user An array containing password-related data.
* @return bool Whether or not the password should be rehashed.
*/
function user_password_needs_rehash($user)
{
$defaultHashDriverName = \MyBB\app('hash')->getDefaultDriver();

/** @var Illuminate\Contracts\Hashing\Hasher $hashDriver */
$hashDriver = \MyBB\app('hash')->driver($user['password_algorithm']);

// prevent downgrading to old md5-based algorithm
return $defaultHashDriverName != 'mybb' && (
$user['password_algorithm'] != $defaultHashDriverName ||
$hashDriver->needsRehash($user['password'])
);
}

/**
* Performs a timing attack safe string comparison.
*
Expand Down
4 changes: 4 additions & 0 deletions inc/init.php
Expand Up @@ -235,6 +235,10 @@

require_once __DIR__.'/src/bootstrap.php';

MyBB\app('config')->set(
array_dot($config)
);

// Load plugins
if(!defined("NO_PLUGINS") && !($mybb->settings['no_plugins'] == 1))
{
Expand Down
4 changes: 4 additions & 0 deletions inc/src/Application.php
Expand Up @@ -139,9 +139,13 @@ protected function registerBaseServiceProviders()

$this->register(new RoutingServiceProvider($this));

$this->register(new Config\ServiceProvider($this));

$this->register(new CoreServiceProvider($this));

$this->registerDeferredProvider(new Twig\ServiceProvider($this));

$this->registerDeferredProvider(new Hashing\ServiceProvider($this));
}

/**
Expand Down
15 changes: 15 additions & 0 deletions inc/src/Config/ServiceProvider.php
@@ -0,0 +1,15 @@
<?php

namespace MyBB\Config;

use Illuminate\Config\Repository;

class ServiceProvider extends \Illuminate\Support\ServiceProvider
{
public function register()
{
$this->app->singleton('config', function () {
return new Repository();
});
}
}
32 changes: 32 additions & 0 deletions inc/src/Hashing/HashManager.php
@@ -0,0 +1,32 @@
<?php

namespace MyBB\Hashing;

use Illuminate\Support\Str;

class HashManager extends \Illuminate\Hashing\HashManager
{
/**
* Create an instance of the Mybb hash Driver.
*
* @return \MyBB\Hashing\MybbHasher
*/
public function createMybbDriver(): MybbHasher
{
return new MybbHasher();
}

/**
* Returns whether a driver of given name exists.
*
* @param string $name
* @return bool
*/
public function driverExists(string $name): bool
{
return (
isset($this->customCreators[$name]) ||
method_exists($this, 'create' . Str::studly($name) . 'Driver')
);
}
}
59 changes: 59 additions & 0 deletions inc/src/Hashing/MybbHasher.php
@@ -0,0 +1,59 @@
<?php

namespace MyBB\Hashing;

use Illuminate\Hashing\AbstractHasher as AbstractHasher;
use Illuminate\Contracts\Hashing\Hasher as HasherContract;

class MybbHasher extends AbstractHasher implements HasherContract
{
/**
* Indicates whether to perform an algorithm check.
*
* @var bool
*/
protected $verifyAlgorithm = false;

/**
* Hash the given value.
*
* @param string $value
* @param array $options
* @return string
*/
public function make($value, array $options = [])
{
$salt = $options['salt'] ?? null;

$hash = md5(md5($salt) . md5($value));

return $hash;
}

/**
* Check the given plain value against a hash.
*
* @param string $value
* @param string $hashedValue
* @param array $options
* @return bool
*/
public function check($value, $hashedValue, array $options = [])
{
$hashedUserString = self::make($value, $options);

return hash_equals($hashedValue, $hashedUserString);
}

/**
* Check if the given hash has been hashed using the given options.
*
* @param string $hashedValue
* @param array $options
* @return bool
*/
public function needsRehash($hashedValue, array $options = [])
{
return false;
}
}
17 changes: 17 additions & 0 deletions inc/src/Hashing/ServiceProvider.php
@@ -0,0 +1,17 @@
<?php

namespace MyBB\Hashing;

class ServiceProvider extends \Illuminate\Hashing\HashServiceProvider
{
public function register()
{
$this->app->singleton('hash', function ($app) {
return new HashManager($app);
});

$this->app->singleton('hash.driver', function ($app) {
return $app['hash']->driver();
});
}
}
16 changes: 16 additions & 0 deletions inc/src/Hashing/WrappableHasher.php
@@ -0,0 +1,16 @@
<?php

namespace MyBB\Hashing;

use Illuminate\Contracts\Hashing\Hasher;

interface WrappableHasher extends Hasher
{
/**
* Hash the given value.
*
* @param string $hash
* @return array
*/
public function wrap(string $hash): string;
}
3 changes: 2 additions & 1 deletion install/resources/mysql_db_tables.php
Expand Up @@ -1055,7 +1055,8 @@
$tables[] = "CREATE TABLE mybb_users (
uid int unsigned NOT NULL auto_increment,
username varchar(120) NOT NULL default '',
password varchar(120) NOT NULL default '',
password varchar(500) NOT NULL default '',
password_algorithm varchar(30) NOT NULL default '',
salt varchar(10) NOT NULL default '',
loginkey varchar(50) NOT NULL default '',
email varchar(220) NOT NULL default '',
Expand Down
3 changes: 2 additions & 1 deletion install/resources/pgsql_db_tables.php
Expand Up @@ -1018,7 +1018,8 @@
$tables[] = "CREATE TABLE mybb_users (
uid serial,
username varchar(120) NOT NULL default '',
password varchar(120) NOT NULL default '',
password varchar(500) NOT NULL default '',
password_algorithm varchar(30) NOT NULL default '',
salt varchar(10) NOT NULL default '',
loginkey varchar(50) NOT NULL default '',
email varchar(220) NOT NULL default '',
Expand Down
3 changes: 2 additions & 1 deletion install/resources/sqlite_db_tables.php
Expand Up @@ -939,7 +939,8 @@
$tables[] = "CREATE TABLE mybb_users (
uid INTEGER PRIMARY KEY,
username varchar(120) NOT NULL default '',
password varchar(120) NOT NULL default '',
password varchar(500) NOT NULL default '',
password_algorithm varchar(30) NOT NULL default '',
salt varchar(10) NOT NULL default '',
loginkey varchar(50) NOT NULL default '',
email varchar(220) NOT NULL default '',
Expand Down

0 comments on commit d225830

Please sign in to comment.