search

zendframework / zend-crypt

Cryptographic component from Zend Framework
BSD 3-Clause "New" or "Revised" License
90 stars 38 forks source link

Minor Modes to Modes + Padding #41

Closed dbierer closed 7 years ago

dbierer commented 7 years ago

Should remove "ecb" as a mode and add "ctr". Also, if padding is set to a value of "0", openssl_encrypt() does PKCS7 padding internally

ezimuel commented 7 years ago

@dbierer Why you want to remove ecb mode? Zend\Crypt\Symmetric\Openssl is an adapter of SymmetricInterface, in that sense is a low-level component that should not be used directly. People can have the need to use ecb even if it's not secure, for instance legacy code. We provided Zend\Crypt\BlockCipher as main component to encrypt/decrypt using best practices (the default mode is cbc here). If someone is using the SymmetricInterface directly that means know how to use it and know about ecb.

Moreover, PKCS7 is not the only padding mode that we want to support. We provided also adapters here to be more open to implementations, not just OpenSSL.

The only point that I agree is to add the ctr mode, but most of the OpenSSL version offers it only for AES. We should check for availability. Do you want to send a PR to manage that?

dbierer commented 7 years ago

Enrico -- ok legacy support for ecb mode makes sense. I'll work up support for ctr mode and get the PR to you in the next week.

Back to padding modes. The way you've encoded it, these flags are currently hard coded into the call to openssl_encrypt(): OPENSSL_RAW_DATA | OPENSSL_ZERO_PADDING. This is not the same as supplying a value of "0" which lets openssll_encrypt() do its own internal padding using PKCS7 internally. Please have a look here: [http://php.net/openssl_encrypt]() at the comment made by "openssl at mailismagic dot com". This is the appropriate part of his comment:

So as we can see here, OPENSSL_ZERO_PADDING has a direct impact on the OpenSSL context. EVP_CIPHER_CTX_set_padding() enables or disables padding (enabled by default). So, OPENSSL_ZERO_PADDING disables padding for the context, which means that you will have to manually apply your own padding out to the block size. Without using OPENSSL_ZERO_PADDING, you will automatically get PKCS#7 padding.

So, although I can appreciate allowing users to use the options of OPENSSL_RAW_DATA and/or OPENSSL_ZERO_PADDING, I still think it would be a good idea to also allow for an option of "0" which will allow openssl_encrypt() to do its own padding, and avoid forcing users to do their own padding externally.

ezimuel commented 7 years ago

@dbierer Thanks!