Fix for the issue #3541 - salt size for Encrypt/Decrypt Filter #3550

Closed
wants to merge 6 commits into from

5 participants

@ezimuel
Zend Framework member

This PR fix the issue #3541. In the BlockCipher used by Encrypt/Decrypt Filter the salt (vector) value has been resize to the correct value during the authentication , according to the encryption algorithm.
This PR is a replacement of the pull #3547.

More changes have been introduced, see comments below.

@Maks3w
Zend Framework member

There is a CS issue in the Mcrypt class

Also I think that all the issue must be fixed inside of Mcrypt class without to truncate the salt value outside. Maybe throw an exception on setSalt if salt length is more than saltSize. Optionally add an argument to setSalt for auto truncate the value

@Maks3w Maks3w was assigned Jan 24, 2013
@radnan

@Maks3w unfortunately you can't do that since different algorithms have different salt sizes and you can change/set the algorithm after setting the salt but before calling encrypt.

@Maks3w
Zend Framework member

Then we could add something like getCorrectSalt in Mcrypt class

@ezimuel
Zend Framework member

@Maks3w you cannot truncate the salt during the setSalt in Mcrypt because if you change the encryption algorithm using the setAlgorithm() you will need a different salt size. Also the auto truncate argument to the setSalt is not a good idea, it will confuse people.
Maybe, it would be better to change the getSalt() in Mcrypt, truncating the output according to the actual encryption algorithm. I will try this approach and let you know. I'm also fixing the CS issue.

@Maks3w
Zend Framework member

Thank you @ezimuel, we could change getSalt for return algorithm specific salt and getOriginalSalt for return the original value

@Maks3w Maks3w and 1 other commented on an outdated diff Jan 25, 2013
tests/ZendTest/Crypt/Symmetric/McryptTest.php
$this->mcrypt->setKey($this->key);
$this->mcrypt->setPadding(new PKCS7());
foreach ($this->mcrypt->getSupportedAlgorithms() as $algo) {
foreach ($this->mcrypt->getSupportedModes() as $mode) {
$this->mcrypt->setAlgorithm($algo);
$this->mcrypt->setMode($mode);
+ $this->mcrypt->setSalt($this->salt);
@Maks3w
Zend Framework member
Maks3w added a note Jan 25, 2013

I think that this should be undo too for to have a test with the same salt stored in the object and different algos.

@ezimuel
Zend Framework member
ezimuel added a note Jan 25, 2013

Yes, that's true.

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

I changed the get/setSalt in Mcrypt and BlockCipher managing the correct size for the salt according to the encryption algorithm. Only getSalt() truncate the size. As suggested by @Maks3w I added a getOriginalSalt() to retrieve the original salt value.

Moreover, I refactored the implementation of the Zend\Filter\Encrypt and Decrypt adding a setKey() and removing the default key 'ZendFramework'. In this way you must specify a key in order to use it. You can omit to initialize the Vector because we are using the BlockCipher as default adapter and it will generate a random Vector (salt) by default.

This last changed solve finally the issue #3541. I'm going to change also the documentation of Zend\Filter\Encrypt and Zend\Filter\Decrypt.

@Freeaqingme
Zend Framework member

@ezimuel You're requesting this PR to be merged against the master branch. I'm however seeing some (minor) BC changes and perhaps new functionality. Is this because it is considered a security issue?

@Maks3w Maks3w commented on an outdated diff Jan 25, 2013
library/Zend/Crypt/Symmetric/Mcrypt.php
@@ -247,13 +250,19 @@ public function setKey($key)
*/
public function getKey()
{
- return $this->key;
+ if (strlen($this->key) < $this->getKeySize()) {
@Maks3w
Zend Framework member
Maks3w added a note Jan 25, 2013

I think that check this in set and get is redundant. We the check in the setter is enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on an outdated diff Jan 25, 2013
library/Zend/Filter/Encrypt/BlockCipher.php
@@ -34,15 +34,13 @@ class BlockCipher implements EncryptionAlgorithmInterface
* 'key_iteration' => the number of iterations for the PBKDF2 key generation
* 'algorithm => cipher algorithm to use
* 'hash' => algorithm to use for the authentication
- * 'iv' => initialization vector
+ * 'vector => initialization vector
@Maks3w
Zend Framework member
Maks3w added a note Jan 25, 2013

missing ' after vector

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

@Freeaqingme Yes, the changes in Zend\Filter\Encrypt and Decrypt are for security issue. The only new features are the setKey() in Zend\Filter\Encrypt and Decrypt and the getOriginalSalt() in BlockCipher and Mcrypt.

@weierophinney weierophinney added a commit that referenced this pull request Jan 25, 2013
@weierophinney weierophinney Merge branch 'hotfix/3550' into develop
Forward port #3550
303eee7
@weierophinney weierophinney added a commit that closed this pull request Jan 25, 2013
@weierophinney weierophinney Merge branch 'hotfix/3550'
Close #3550
Fixes #3541
9c7c806
@ezimuel ezimuel referenced this pull request in zendframework/zf2-documentation Jan 25, 2013
Merged

Fixed the doc for Filter\Encrypt and Decrypt according with the PR #3550 #605

@ezimuel ezimuel added a commit to ezimuel/zf2 that referenced this pull request Jan 29, 2013
@ezimuel ezimuel Fixed the unit test for Zend\Filter\File\Encrypt and Decrypt based on…
… PR #3550
e7094bd
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'hotfix/3550'
Close #3550
Fixes #3541
bfff3f3
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'hotfix/3550' into develop
Forward port #3550
c32cda5
@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@ezimuel ezimuel Fixed the unit test for Zend\Filter\File\Encrypt and Decrypt based on…
… PR #3550
7664010
@weierophinney weierophinney added a commit to zendframework/zend-crypt that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3550' dacba6f
@weierophinney weierophinney added a commit to zendframework/zend-crypt that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3550' into develop 40a0449
@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3550' cef4359
@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/3550' into develop ddb5852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment