Fix for the issue #3541 - salt size for Encrypt/Decrypt Filter #3550
Conversation
|
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 |
ghost
assigned 
|
@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. |
|
Then we could add something like getCorrectSalt in Mcrypt class |
|
@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. |
|
Thank you @ezimuel, we could change |
| $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); |
There was a problem hiding this comment.
I think that this should be undo too for to have a test with the same salt stored in the object and different algos.
|
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. |
|
@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? |
| @@ -247,13 +250,19 @@ public function setKey($key) | |||
| */ | |||
| public function getKey() | |||
| { | |||
| return $this->key; | |||
| if (strlen($this->key) < $this->getKeySize()) { | |||
There was a problem hiding this comment.
I think that check this in set and get is redundant. We the check in the setter is enough
|
@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. |
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.