Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create hookable functions for password hashing #2499

Merged
merged 2 commits into from Nov 14, 2016

Conversation

Projects
None yet
3 participants
@dvz
Copy link
Contributor

commented Nov 8, 2016

Fixes #2498

  • removed hardcoded md5() pre-hashing,
  • salt_password() marked as deprecated,
  • create_password_hash() and verify_user_password() added with new hooks, accepting the user row and raw password,
  • complete user row fetched on login attempt,
  • my_hash_equals() added, performing comparison using hash_equals() on PHP >= 5.6 or custom implementation

Session keys comparison should be also rewritten to use my_hash_equals() in the future.

Proof-of-concept plugin taking advantage of those changes available at: https://github.com/Devilshakerz/mybb-dvzHash


if(is_string($parameters))
{
return $parameters;

This comment has been minimized.

Copy link
@JN-Jones

JN-Jones Nov 9, 2016

Contributor

I guess this is for plugins who validated the password? Why not use a normal return within the plugin call using a new variable? IMHO that would look cleaner

This comment has been minimized.

Copy link
@dvz

dvz Nov 9, 2016

Author Contributor

The plugin class doesn't handle hook return values well: https://github.com/mybb/mybb/blob/mybb_1808/inc/class_plugins.php#L142-L145
If a custom function returns false, the hook outputs the original $arguments variable - this leads to messy implementations one way or another.

This comment has been minimized.

Copy link
@JN-Jones

JN-Jones Nov 9, 2016

Contributor

I still don't see why we shouldn't use the return value:

$plugin_validation = false;

$plugin_validation = $plugins->run_hook(...);

if(is_string($plugin_validation))
{
    return $plugin_validation;
}

Even if we stay with using the parameter as return variable I'd much more prefer to have a seperate index for the return value (eg $parameters['hash']) as overwriting the normal parameters is something I as a plugin author would never guess without looking at the code. It simply seems wrong.

This comment has been minimized.

Copy link
@euantorano

euantorano Nov 9, 2016

Member

Yeah, I'd find overwriting the parameters weird personally.

This comment has been minimized.

Copy link
@dvz

dvz Nov 9, 2016

Author Contributor

That's how the plugin system works - if the custom function's return value evaluates to true, $parameters is being overwritten with it. $hash = $plugins->run_hooks(...); can be used instead of operating on $parameters in create_password_hash(), but the variable will be changed anyway. This is not applicable to verify_user_password() as the code needs to receive a boolean value in order to recognize it as a plugin's output (false as well as empty hook would return $parameters).


if(is_bool($parameters))
{
return $parameters;

This comment has been minimized.

Copy link
@JN-Jones

JN-Jones Nov 9, 2016

Contributor

Same here

@euantorano

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

I understand that, but modifying it as a plugin author isn't the obvious thing to do.

I think it'll be fine here though, as it's not like there will be hundreds of plugins using these hooks or anything (like global_start, etc.).

On 9 Nov 2016, at 17:18, Tomasz Mlynski notifications@github.com wrote:

@devilshakerz commented on this pull request.

In inc/functions_user.php:

  • * @return string The password hash.
  • */
    +function create_password_hash($password, $salt, $user = false)
    +{
  • global $plugins;
    +
  • $parameters = compact('password', 'salt', 'user');
    +
  • if(!defined('IN_INSTALL') && !defined('IN_UPGRADE'))
  • {
  • $plugins->run_hooks('create_password_hash', $parameters);
    
  • }
    +
  • if(is_string($parameters))
  • {
  • return $parameters;
    
    That's how the plugin system works - if the custom function's return value evaluates to true, $parameters is being overwritten with it. $hash = $plugins->run_hooks(...); can be used instead of operating on $parameters in create_password_hash(), but the variable will be changed anyway. This is not applicable to verify_user_password() as the code needs to receive a boolean value in order to recognize it as a plugin's output (false as well as empty hook would return $parameters).


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@JN-Jones

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2016

Ok, I see that it wouldn't work with the second hook. However I still think that an additional index would be better than overwriting the whole parameter. @devilshakerz @euantorano

@euantorano euantorano merged commit 3b8da40 into mybb:feature Nov 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.