Stricter error handling in mcrypt extension

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.

Hi Nikita,

An implementation of these changes can be found in the PR
https://github.com/php/php-src/pull/610.

Could you take care this, too.
https://github.com/php/php-src/pull/579

Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2014.03.02. 1:13, "Nikita Popov" nikita.ppv@gmail.com ezt írta:

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.

Hi Nikita,

While I think that the overwhelming majority of the voters would support
this change, so an rfc could seem as an overkill, but I still think that it
is better to require an rfc for every BC break.

Which unfortunately would mean that this can't target 5.6.0.
:(

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.

I've added some comments, which those fixed, feel free to commit this.

cheers,
Derick

--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine