[2.2] [Security] Added Pbkdf2PasswordEncoder #4661

Merged
merged 1 commit into from Oct 8, 2012

6 participants

@sstok

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

This adds the PBKDF2 derived key mechanism (as defined in http://www.ietf.org/rfc/rfc2898.txt) for the Password encoder.

The original implementation comes from http://www.itnewb.com/tutorial/Encrypting-Passwords-with-PHP-for-Storage-Using-the-RSA-PBKDF2-Standard and does not contain any restrictive copyright. I have included the original author.

@travisbot

This pull request passes (merged bce31c13 into 4221239).

@mvrhov

This also warrants a waring that the function is extra slow. Calculation of hash with the default 5000 iterations on small ec2 instance takes approximately 800ms.

@ManuelAC ManuelAC and 1 other commented on an outdated diff Jun 26, 2012
...onent/Security/Core/Encoder/Pbkdf2PasswordEncoder.php
+
+ /**
+ * {@inheritdoc}
+ */
+ public function encodePassword($raw, $salt)
+ {
+ if (!in_array($this->algorithm, hash_algos(), true)) {
+ throw new \LogicException(sprintf('The algorithm "%s" is not supported.', $this->algorithm));
+ }
+
+ // Number of blocks needed to create the derived key
+ $blocks = ceil($this->length / strlen(hash($this->algorithm, null, true)));
+ $digest = '';
+
+ for ($i = 1; $i <= $blocks; $i++) {
+ $ib = $block = hash_hmac($this->algorithm, $salt . pack('N', $i), $raw, true);

$block is an unused variable, same on line 62.

it is not, on that line $block is assigned a value, $ib is just assigned the same values as $block.

I see, totally read over the second parameter of line 62.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sstok

@mvrhov What do you mean exactly? Should I reduce the default number of Iterations?

Edit: Oops, my own class in rollerworks/Crypt also uses 1000, not 5000.
I used the MessageDigestPasswordEncoder as my template and forgot to change that.
Fixed.

@travisbot

This pull request passes (merged 8561e47a into 4221239).

@mvrhov

@sstok: What I meant was that it would be nice to include that info into the PhpDoc block or inside a changelog.
Between the plain salted sha512, sha512 based Pbkdf2 and sha512 based bcrypt, bcrypt was slower than sha512, but way faster than Pbkdf2. I've measured all of them on small ec2 instance.
Oh, and BTW it was 1000 iterations in Pbkdf2 that took 800ms.

@sstok
 * Pbkdf2PasswordEncoder uses the PBKDF2 (Password-Based Key Derivation Function 2).
 *
 * Providing a high level of Cryptographic security,
 *  PBKDF2 is recommended by the National Institute of Standards and Technology (NIST).
 *
 * But also warrants a warning, using PBKDF2 (with a high number of iterations) slows down the process.
 * PBKDF2 should be used with caution and care.

Something like this, any suggestions are welcome ;)

PS: Should I also add this to the SecurityBundle?, but 'algorithm' always passes it to MessageDigestPasswordEncoder when it not plain. So I wonder what to do for that, using something as pbkdf2_[algorithm] like: pbkdf2_sha512

@jalliot

@sstok That would be a really valuable addition to Symfony :)
And I think indeed that you should modify SecurityBundle by adding a simple way to switch from the basic encoder to this one (and surely set it as the default!).

Another nice thing you could do is provide a bcrypt implementation. @elnur's ElnurBlowfishPasswordEncoderBundle might give you some inspiration.

@sstok

@jalliot Thanks for the tip, changing the default is not a good idea as PBKDF2 pretty heavy when compared to Digit.
The only difference between PBKDF2 and Digit is that PBKDF2 uses HMAC and does some extra things, so they are both very secure. But the second is more secure then the other ;)

Implementing bcrypt should be no problem, I will open an new pull request for that one when ready.

Edit: I think I have an idea, setting algorithm to pbkdf2 with hash_algorithm as parameter.

@sstok

@schmittjoh As this is a simple change should it go for 2.1 or 2.2?

@jalliot

IIUC 2.1 is feature frozen so that will surely not be merged before 2.2.

@fabpot
Symfony member

This is indeed scheduled for 2.2.

@sstok

@fabpot ping

@fabpot
Symfony member

Before I merge this PR, can you:

  • add an entry in the CHANGELOG of the component and the bundle
  • squash your commits
  • create a PR on the docs to mention the new encoder (its usage and the limitations as you mentioned them here)

Thanks.

@stof stof commented on an outdated diff Oct 2, 2012
...urityBundle/DependencyInjection/MainConfiguration.php
@@ -378,6 +378,8 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode)
->beforeNormalization()->ifString()->then(function($v) { return array('algorithm' => $v); })->end()
->children()
->scalarNode('algorithm')->cannotBeEmpty()->end()
+ ->scalarNode('hash_algorithm')->defaultValue('sha512')->end()
@stof
Symfony member
stof added a note Oct 2, 2012

you should add an ->info() call explaining it is only used for pbkdf2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof
Symfony member

The XSD also need to be updated

@fabpot
Symfony member

@stof: AFAIR, there is unfortunately no XSD for the Security bundle.... yet

@fabpot
Symfony member

@mvrhov Indeed, it's going to be included in PHP as of PHP 5.5. We need to use it if available.

@stof
Symfony member

@fabpot ah true. and I don't want to try creating an XSD in this bundle as the config tree can be expanded dynamically by any bundle :)

@sstok

@fabpot ping

@stof stof commented on an outdated diff Oct 3, 2012
...onent/Security/Core/Encoder/Pbkdf2PasswordEncoder.php
+ */
+ public function __construct($algorithm = 'sha512', $encodeHashAsBase64 = true, $iterations = 1000, $length = 40)
+ {
+ $this->algorithm = $algorithm;
+ $this->encodeHashAsBase64 = $encodeHashAsBase64;
+ $this->iterations = $iterations;
+ $this->length = $length;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function encodePassword($raw, $salt)
+ {
+ if (function_exists('hash_pbkdf2')) {
+ $digest = \hash_pbkdf2($this->algorithm, $raw, $salt, $this->iterations, $this->length, true);
@stof
Symfony member
stof added a note Oct 3, 2012

Please remove the \ on the function call. Symfony does not use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Oct 3, 2012
...onent/Security/Core/Encoder/Pbkdf2PasswordEncoder.php
+ } else {
+ $digest = $this->hash_pbkdf2($this->algorithm, $raw, $salt, $this->iterations, $this->length);
+ }
+
+ return $this->encodeHashAsBase64 ? base64_encode($digest) : bin2hex($digest);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function isPasswordValid($encoded, $raw, $salt)
+ {
+ return $this->comparePasswords($encoded, $this->encodePassword($raw, $salt));
+ }
+
+ private function hash_pbkdf2($algorithm, $password, $salt, $iterations, $length = 0)
@stof
Symfony member
stof added a note Oct 3, 2012

Please use a camelCased name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff Oct 3, 2012
...rity/Tests/Core/Encoder/Pbkdf2PasswordEncoderTest.php
+ public function testEncodePassword()
+ {
+ $encoder = new Pbkdf2PasswordEncoder('sha256', false, 1, 40);
+ $this->assertSame('c1232f10f62715fda06ae7c0a2037ca19b33cf103b727ba56d870c11f290a2ab106974c75607c8a3', $encoder->encodePassword('password', ''));
+
+ $encoder = new Pbkdf2PasswordEncoder('sha256', true, 1, 40);
+ $this->assertSame('wSMvEPYnFf2gaufAogN8oZszzxA7cnulbYcMEfKQoqsQaXTHVgfIow==', $encoder->encodePassword('password', ''));
+
+ $encoder = new Pbkdf2PasswordEncoder('sha256', false, 2, 40);
+ $this->assertSame('8bc2f9167a81cdcfad1235cd9047f1136271c1f978fcfcb35e22dbeafa4634f6fd2214218ed63ebb', $encoder->encodePassword('password', ''));
+ }
+
+ /**
+ * @expectedException LogicException
+ */
+ public function testEncodePasswordAlgorithmDoesNotExist()
@stof
Symfony member
stof added a note Oct 3, 2012

This will fail on PHP 5.5 as it does not throw a LogicException but a warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Oct 3, 2012
...onent/Security/Core/Encoder/Pbkdf2PasswordEncoder.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function isPasswordValid($encoded, $raw, $salt)
+ {
+ return $this->comparePasswords($encoded, $this->encodePassword($raw, $salt));
+ }
+
+ private function hash_pbkdf2($algorithm, $password, $salt, $iterations, $length = 0)
+ {
+ if (!in_array($algorithm, hash_algos(), true)) {
+ throw new \LogicException(sprintf('The algorithm "%s" is not supported.', $algorithm));
+ }
+
@stof
Symfony member
stof added a note Oct 3, 2012

I think you should always call this method, to do the algo check instead of receiving a warning.:

private function hashPbkdf2($algorithm, $password, $salt, $iterations, $length = 0)
{
    if (!in_array($algorithm, hash_algos(), true)) {
        throw new \LogicException(sprintf('The algorithm "%s" is not supported.', $algorithm));
    }

    if (function_exists('hash_pbkdf2')) {
        return hash_pbkdf2($this->algorithm, $raw, $salt, $this->iterations, $this->length, true);
    }

// The fallback algo implementation here
}
@sstok
sstok added a note Oct 3, 2012

👍 great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sstok sstok [Security] Added Pbkdf2PasswordEncoder
[Security] changed default iterations of Pbkdf2PasswordEncoder to 1000 instead of 5000

[Security] Improved description of PBKDF2 encoder

[SecurityBundle] added PBKDF2 PasswordEncoder

updated CHANGELOG.md

[Security] Use the build-in hash_pbkdf2() when available

[SecurityBundle] added information about hash_algorithm for configuration

[Security] always check algorithm and fixed CS
4534960
@sstok

@fabpot ping

@fabpot fabpot added a commit that referenced this pull request Oct 8, 2012
@fabpot fabpot merged branch sstok/security_encoder_pbkdf2 (PR #4661)
This PR was merged into the master branch.

Commits
-------

4534960 [Security] Added Pbkdf2PasswordEncoder

Discussion
----------

[2.2] [Security] Added Pbkdf2PasswordEncoder

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

This adds the PBKDF2 derived key mechanism (as defined in http://www.ietf.org/rfc/rfc2898.txt) for the Password encoder.

The original implementation comes from http://www.itnewb.com/tutorial/Encrypting-Passwords-with-PHP-for-Storage-Using-the-RSA-PBKDF2-Standard and does not contain any restrictive copyright. I have included the original author.

---------------------------------------------------------------------------

by mvrhov at 2012-06-26T10:33:59Z

This also warrants a waring that the function is extra slow. Calculation of hash with the default 5000 iterations on small ec2 instance takes approximately 800ms.

---------------------------------------------------------------------------

by sstok at 2012-06-26T11:17:25Z

@mvrhov What do you mean exactly? Should I reduce the default number of Iterations?

Edit: Oops, my own class in rollerworks/Crypt also uses 1000, not 5000.
I used the MessageDigestPasswordEncoder as my template and forgot to change that.
Fixed.

---------------------------------------------------------------------------

by mvrhov at 2012-06-26T12:04:28Z

@sstok: What I meant was that it would be nice to include that info into the PhpDoc block or inside a changelog.
Between the plain salted sha512, sha512 based Pbkdf2 and sha512 based bcrypt, bcrypt was slower than sha512, but way faster than Pbkdf2. I've measured all of them on small ec2 instance.
Oh, and BTW it was 1000 iterations in Pbkdf2 that took 800ms.

---------------------------------------------------------------------------

by sstok at 2012-06-26T12:39:46Z

```
 * Pbkdf2PasswordEncoder uses the PBKDF2 (Password-Based Key Derivation Function 2).
 *
 * Providing a high level of Cryptographic security,
 *  PBKDF2 is recommended by the National Institute of Standards and Technology (NIST).
 *
 * But also warrants a warning, using PBKDF2 (with a high number of iterations) slows down the process.
 * PBKDF2 should be used with caution and care.
```
Something like this, any suggestions are welcome ;)

PS: Should I also add this to the SecurityBundle?, but 'algorithm' always passes it to MessageDigestPasswordEncoder when it not plain. So I wonder what to do for that, using something as pbkdf2_[algorithm] like: pbkdf2_sha512

---------------------------------------------------------------------------

by jalliot at 2012-07-06T22:27:22Z

@sstok That would be a really valuable addition to Symfony :)
And I think indeed that you should modify SecurityBundle by adding a simple way to switch from the basic encoder to this one (and surely set it as the default!).

Another nice thing you could do is provide a bcrypt implementation. @elnur's [ElnurBlowfishPasswordEncoderBundle](https://github.com/elnur/ElnurBlowfishPasswordEncoderBundle) might give you some inspiration.

---------------------------------------------------------------------------

by sstok at 2012-07-08T12:25:29Z

@jalliot Thanks for the tip, changing the default is not a good idea as PBKDF2 pretty heavy when compared to Digit.
The only difference between PBKDF2 and Digit is that PBKDF2 uses HMAC and does some extra things, so they are both very secure. But the second is more secure then the other ;)

Implementing bcrypt should be no problem, I will open an new pull request for that one when ready.

Edit: I think I have an idea, setting algorithm to pbkdf2 with hash_algorithm as parameter.

---------------------------------------------------------------------------

by sstok at 2012-07-18T09:54:15Z

@schmittjoh As this is a simple change should it go for 2.1 or 2.2?

---------------------------------------------------------------------------

by jalliot at 2012-07-18T11:02:40Z

IIUC 2.1 is feature frozen so that will surely not be merged before 2.2.

---------------------------------------------------------------------------

by fabpot at 2012-07-23T14:26:30Z

This is indeed scheduled for 2.2.

---------------------------------------------------------------------------

by sstok at 2012-10-02T13:51:59Z

@fabpot ping

---------------------------------------------------------------------------

by fabpot at 2012-10-02T16:20:23Z

Before I merge this PR, can you:

 * add an entry in the CHANGELOG of the component and the bundle
 * squash your commits
 * create a PR on the docs to mention the new encoder (its usage and the limitations as you mentioned them here)

Thanks.

---------------------------------------------------------------------------

by stof at 2012-10-02T16:27:03Z

The XSD also need to be updated

---------------------------------------------------------------------------

by fabpot at 2012-10-02T16:37:53Z

@stof: AFAIR, there is unfortunately no XSD for the Security bundle.... yet

---------------------------------------------------------------------------

by mvrhov at 2012-10-02T16:56:39Z

BTW: http://php.net/manual/en/function.hash-pbkdf2.php

---------------------------------------------------------------------------

by fabpot at 2012-10-02T17:17:57Z

@mvrhov Indeed, it's going to be included in PHP as of PHP 5.5. We need to use it if available.

---------------------------------------------------------------------------

by stof at 2012-10-02T17:28:17Z

@fabpot ah true. and I don't want to try creating an XSD in this bundle as the config tree can be expanded dynamically by any bundle :)

---------------------------------------------------------------------------

by sstok at 2012-10-03T09:29:53Z

@fabpot ping

---------------------------------------------------------------------------

by sstok at 2012-10-08T09:21:09Z

@fabpot ping
700d078
@fabpot fabpot merged commit 4534960 into symfony:master Oct 8, 2012
@stloyd stloyd referenced this pull request Jan 9, 2013
Closed

Pbkdf2 encoder #6633

@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot merged branch sstok/security_encoder_pbkdf2 (PR #4661)
This PR was merged into the master branch.

Commits
-------

4534960 [Security] Added Pbkdf2PasswordEncoder

Discussion
----------

[2.2] [Security] Added Pbkdf2PasswordEncoder

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

This adds the PBKDF2 derived key mechanism (as defined in http://www.ietf.org/rfc/rfc2898.txt) for the Password encoder.

The original implementation comes from http://www.itnewb.com/tutorial/Encrypting-Passwords-with-PHP-for-Storage-Using-the-RSA-PBKDF2-Standard and does not contain any restrictive copyright. I have included the original author.

---------------------------------------------------------------------------

by mvrhov at 2012-06-26T10:33:59Z

This also warrants a waring that the function is extra slow. Calculation of hash with the default 5000 iterations on small ec2 instance takes approximately 800ms.

---------------------------------------------------------------------------

by sstok at 2012-06-26T11:17:25Z

@mvrhov What do you mean exactly? Should I reduce the default number of Iterations?

Edit: Oops, my own class in rollerworks/Crypt also uses 1000, not 5000.
I used the MessageDigestPasswordEncoder as my template and forgot to change that.
Fixed.

---------------------------------------------------------------------------

by mvrhov at 2012-06-26T12:04:28Z

@sstok: What I meant was that it would be nice to include that info into the PhpDoc block or inside a changelog.
Between the plain salted sha512, sha512 based Pbkdf2 and sha512 based bcrypt, bcrypt was slower than sha512, but way faster than Pbkdf2. I've measured all of them on small ec2 instance.
Oh, and BTW it was 1000 iterations in Pbkdf2 that took 800ms.

---------------------------------------------------------------------------

by sstok at 2012-06-26T12:39:46Z

```
 * Pbkdf2PasswordEncoder uses the PBKDF2 (Password-Based Key Derivation Function 2).
 *
 * Providing a high level of Cryptographic security,
 *  PBKDF2 is recommended by the National Institute of Standards and Technology (NIST).
 *
 * But also warrants a warning, using PBKDF2 (with a high number of iterations) slows down the process.
 * PBKDF2 should be used with caution and care.
```
Something like this, any suggestions are welcome ;)

PS: Should I also add this to the SecurityBundle?, but 'algorithm' always passes it to MessageDigestPasswordEncoder when it not plain. So I wonder what to do for that, using something as pbkdf2_[algorithm] like: pbkdf2_sha512

---------------------------------------------------------------------------

by jalliot at 2012-07-06T22:27:22Z

@sstok That would be a really valuable addition to Symfony :)
And I think indeed that you should modify SecurityBundle by adding a simple way to switch from the basic encoder to this one (and surely set it as the default!).

Another nice thing you could do is provide a bcrypt implementation. @elnur's [ElnurBlowfishPasswordEncoderBundle](https://github.com/elnur/ElnurBlowfishPasswordEncoderBundle) might give you some inspiration.

---------------------------------------------------------------------------

by sstok at 2012-07-08T12:25:29Z

@jalliot Thanks for the tip, changing the default is not a good idea as PBKDF2 pretty heavy when compared to Digit.
The only difference between PBKDF2 and Digit is that PBKDF2 uses HMAC and does some extra things, so they are both very secure. But the second is more secure then the other ;)

Implementing bcrypt should be no problem, I will open an new pull request for that one when ready.

Edit: I think I have an idea, setting algorithm to pbkdf2 with hash_algorithm as parameter.

---------------------------------------------------------------------------

by sstok at 2012-07-18T09:54:15Z

@schmittjoh As this is a simple change should it go for 2.1 or 2.2?

---------------------------------------------------------------------------

by jalliot at 2012-07-18T11:02:40Z

IIUC 2.1 is feature frozen so that will surely not be merged before 2.2.

---------------------------------------------------------------------------

by fabpot at 2012-07-23T14:26:30Z

This is indeed scheduled for 2.2.

---------------------------------------------------------------------------

by sstok at 2012-10-02T13:51:59Z

@fabpot ping

---------------------------------------------------------------------------

by fabpot at 2012-10-02T16:20:23Z

Before I merge this PR, can you:

 * add an entry in the CHANGELOG of the component and the bundle
 * squash your commits
 * create a PR on the docs to mention the new encoder (its usage and the limitations as you mentioned them here)

Thanks.

---------------------------------------------------------------------------

by stof at 2012-10-02T16:27:03Z

The XSD also need to be updated

---------------------------------------------------------------------------

by fabpot at 2012-10-02T16:37:53Z

@stof: AFAIR, there is unfortunately no XSD for the Security bundle.... yet

---------------------------------------------------------------------------

by mvrhov at 2012-10-02T16:56:39Z

BTW: http://php.net/manual/en/function.hash-pbkdf2.php

---------------------------------------------------------------------------

by fabpot at 2012-10-02T17:17:57Z

@mvrhov Indeed, it's going to be included in PHP as of PHP 5.5. We need to use it if available.

---------------------------------------------------------------------------

by stof at 2012-10-02T17:28:17Z

@fabpot ah true. and I don't want to try creating an XSD in this bundle as the config tree can be expanded dynamically by any bundle :)

---------------------------------------------------------------------------

by sstok at 2012-10-03T09:29:53Z

@fabpot ping

---------------------------------------------------------------------------

by sstok at 2012-10-08T09:21:09Z

@fabpot ping
ab164ba
@cordoval cordoval referenced this pull request Nov 15, 2013
Closed

PBKDF2 is not supported #9513

@sstok sstok deleted the sstok:security_encoder_pbkdf2 branch Nov 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment