[Security] Fix BCryptPasswordEncoder to encode password using salt

[kylecannon]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

After changing the bcrypt logic from Symfony to ircmaxell/password-compat in b2e553a the salt was never passed to the password_hash method.

This actually still occurs in 9d089ef as well:

if (function_exists('password_hash')) {
   return password_hash($raw, PASSWORD_BCRYPT, array('cost' => $this->cost));
}

I don't know if there wasn't salt support previously which is why it was never implemented before. Since a lot of the logic was changed in the 2.3 branch in the class BCryptPasswordEncoder i thought it would be better to issue a pull request here.

[mvrhov]

You don't need the salt. bcrypt adds it automatically!

[fabpot]

We should add a comment to make it clear that the salt is not needed.

[jakzal]

I guess the confusion comes from the php docs. The example you can find there uses the bcrypt encryption with salt.

[stloyd]

Isn't this PR correct? In method params we have $salt:

public function encodePassword($raw, $salt);

Yet it's not used when calling password_hash():

return password_hash($raw, PASSWORD_BCRYPT, array('cost' => $this->cost));

And according to documentation:

If omitted, a random salt will be created and the default cost will be used.

IMO it's not expected behavior, as we can pass salt, but it will be not used anyway, or I'm wrong? =)

[mvrhov]

IMHO the bcrypt will generate a better salt than whatever algorithm most developers will come up with.

[stloyd]

I can agree, but this behavior (adding ability to pass salt and not using it) is not correct.

[bendavies]

here is how php would generate a salt if not supplied
http://lxr.php.net/xref/PHP_5_5/ext/standard/password.c#1111

[jakzal]

How about using the provided salt if it's not empty and rely on php otherwise?

[mvrhov]

Well you'll have to be pretty smart when using provided salt.

• it has to be >= 22 characters long
• if the passed salt is in already base64 encoded and it's the default encoding you are going to loose bits

and probably more.

[kylecannon]

The way I looked at this is if you're giving someone the option to pass the salt and it's not being used then why is it even an option? I now see that it is generated if it's not passed but I assumed that when I would pass the salt it would use the salt I provided. Either I think it should be removed from the method or we should pass it so the developer has the option to use the automatically generated one or their own.

[mvrhov]

Because the encodePassword and isPasswordValid functions are part of an interface!

[kylecannon]

It's not an interface when you're using ircmaxell/password_compat because you're not running PHP 5.5

[stof]

@kylecannon The fact that the password_compat library is used does not change the fact that the PAssswordEncocerInterface implemented by the class requires the salt in the signature.

[fabpot]

I've submitted another PR (#8266) that adds the salt, but also document why it is almost never a good idea to do so.

[greg0ire]

@stof : Maybe the interface method signature should be changed to add null as default value for the salt argument, like this :
public function encodePassword($raw, $salt = null);, to avoid a client code like this : $encoder->encodePassword($user->getPlainPassword(), null) ?

[stof]

@greg0ire this cannot be changed until 3.0. Making a param optional in an interface is a BC break, because it forces all implementations to update their signtures to make it optional as well

[greg0ire]

You're right, I did not think of that. Should I open a new issue? I think this change is important since bcrypt is recommended in the best practices document.