Re: Stricter error handling in mcrypt extension
On Sat, Mar 1, 2014 at 7:12 PM, Nikita Popov <[email protected]> wrote:
> Hi internals!
>
> I would like to add a number of additional error checks in
> php_mcrypt_do_crypt - which affects the mcrypt_encrypt, mcrypt_decrypt and
> mcrypt_{BLOCK_CHAINING_MODE} userland functions.
>
> The proposed changes are:
> * Throw a warning and return bool(false) if the IV size is invalid. The
> old behavior was to throw a warning and use a NUL-byte IV.
> * Throw a warning and return bool(false) if no IV was specified, but the
> block chaining mode requires an IV. The old behavior was to throw a warning
> and use a NUL-byte IV.
> * Throw a warning and return bool(false) if the key size is invalid. The
> old behavior was to **silently** pad the string to the next valid key size
> with NUL bytes or, if the key is too long, to throw a warning and truncate
> it to the maximum valid key size.
>
> An implementation of these changes can be found in the PR
> https://github.com/php/php-src/pull/610.
>
> The reason why I'd like to make the mcrypt input handling stricter is to
> ensure that incorrectly implemented encryption will fail and fail
> violently. With the current lax input checking it is very easy to
> completely compromise confidentiality through simple mistakes - like using
> a password as the encryption key.
>
> Thanks,
> Nikita
>
> PS: I'm running this change by the list because stricter error handling is
> technically a BC break. Of course this change is targeting PHP 5.6 only.
>
I think that's a good idea given the number of people I've heard complaints
from about confusion around using mcrypt and the default NUL-byte IV. My
only concern is, isn't it a bit late in the game to push this to 5.6? Or is
that when we're at beta? Overall I think it's a smart change though despite
the BC.
Thread (14 messages)