On Wed, Jul 15, 2015 at 6:57 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote:
> Hi Scott,
>
> On Wed, Jul 15, 2015 at 7:19 PM, Scott Arciszewski <sc...@paragonie.com>
> wrote:
>>
>> On Wed, Jul 15, 2015 at 4:27 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote:
>> > Hi Sammy,
>> >
>> > On Wed, Jul 15, 2015 at 6:04 AM, Sammy Kaye Powers <m...@sammyk.me> wrote:
>> >
>> >> There are two open PR's for PHP7 to modify the behavior of the
>> >> CSPRNG's:
>> >>
>> >> https://github.com/php/php-src/pull/1397 (main discussion)
>> >> https://github.com/php/php-src/pull/1398
>> >>
>> >> Currently the random_*() functions will issue a warning and return
>> >> false if
>> >> a good source of random cannot be found. This is a potential security
>> >> hole
>> >> in the event the RNG fails and returns false which gets evaluated as 0
>> >> in a
>> >> cryptographic context.
>> >>
>> >> To prevent this exploit the proposed behavior will throw an Exception
>> >> when
>> >> the RNG fails or certain argument validation fails. This also gives the
>> >> developer a graceful way to fall back to an alternate CSPRNG.
>> >>
>> >> Since the core functions in PHP don't throw Exceptions, there is debate
>> >> on
>> >> whether or not this change should be implemented. Some say the CSPRNG's
>> >> should get a special pass since they will be relied on for
>> >> cryptography. If
>> >> we can't throw Exceptions, there were suggestions of raising a fatal
>> >> error
>> >> if the RNG fails.
>> >>
>> >> I think the argument can be boiled down to consistency vs security.
>> >> We'd
>> >> love to hear your feedback to decide what we should do in this context.
>> >> :)
>> >>
>> >
>> > I prefer exception rather than error.
>> >
>> > However, I would not like to see exception in "some" functions.
>> > It's whether we use exception for builtin functions or not.
>> >
>> > I understand the risk, but users should handle all errors properly
>> > to be secure anyway.
>> >
>> > Regards,
>> >
>> > --
>> > Yasuo Ohgaki
>> > yohg...@ohgaki.net
>>
>> -1 I emphatically disagree.
>>
>> > I understand the risk, but users should handle all errors properly
>> > to be secure anyway.
>>
>> Recommended reading:
>> http://cr.yp.to/talks/2015.01.07/slides-djb-20150107-a4.pdf
>>
>> -------------------
>>
>> Instead of placing all the burden on the user to add code to their
>> application to check for errors, lest their application fail open and
>> operate with zero entropy, which is what you've proposed, I say we
>> throw an exception if the random number generator fails. This means
>> that, in the default case, their application will fail closed (i.e.
>> not continue processing with zero entropy), but the developer can
>> still intuitively handle the exception if they want to add code to
>> fail gracefully. Try-catch blocks are far less messy than overriding
>> the error handler.
>>
>> Failing open is a terrible idea. You're going to have people who write
>> code like this:
>>
>> $max = strlen($alphabet) - 1;
>> for ($i = 0; $i < 32; ++$i) {
>> $password .= $alphabet[random_int(0, $max)];
>> }
>>
>> How do I know this? Because people already write code like that with
>> mt_rand().
>>
>> If a random number generator failure occurs, instead of generating a
>> predictable password, we ought to abort. If the developer wants to
>> handle this situation, they ought to do this.
>>
>> try {
>> $max = strlen($alphabet) - 1;
>> for ($i = 0; $i < 32; ++$i) {
>> $password .= $alphabet[random_int(0, $max)];
>> }
>> } catch (Exception $e) {
>> $this->template('error', 'Could not safely generate a passphrase!
>> :(');
>> }
>>
>> In the case of a negligent developer, the uncaught exception is
>> indistinguishable from raising a fatal error. Either way prevents the
>> script from proceeding with zero entropy.
>>
>> TL;DR we have three options:
>>
>> 1. E_WARNING - fail open, possibly cause security problems for the user
>> 2. E_ERROR - fail closed, but make graceful handling a pain in the neck
>> 3. Exception - fail closed by default, developer can write their own
>> graceful failure code if they so choose, would currently be an
>> exception to the rule
>>
>> 1 is bad, 2 is less bad, I think 3 is ideal. Security > continuity.
>
>
> The basis of my thought is user must write code in a way that would
> never raise errors under normal conditions and user must stop execution
> by any errors because they are unexpected.
>
> To do that, user should have custom error handler always to achieve
> graceful exit just like handling uncaught exceptions.
>
> i.e. User should have something like following always.
> <?php
> set_error_handler(function ($severity, $message, $file, $line) {
> throw new ErrorException($message, 0, $severity, $file, $line);
> });
>
> function exception_handler($exception) {
> echo "Uncaught exception: " , $exception->getMessage(), "\n";
> }
> set_exception_handler('exception_handler');
> ?>
>
> Server side web app is simple request/response app basically.
> There is no point to continue execution when unexpected occurs.
> Users should have something similar to above code anyway.
>
> By the way, I agree that random_*() errors are critical. So I don't
> mind to raise E_RECOVERABLE_ERROR from them. IMO, E_RECOVERABLE_ERROR
> is better. The issue with E_ERROR is graceful exit. E_RECOVERABLE_ERROR
> resolves
> the issue.
>
> Regards,
>
> --
> Yasuo Ohgaki
> yohg...@ohgaki.net
>
Hi Yasuo,
> Users should have something similar to above code anyway.
Should, but won't. Take a long look at some of the code on Stack
Overflow. You'll find gems like this (warning: bad crypto code):
function fnEncrypt($sValue, $sSecretKey)
{
return rtrim(
base64_encode(
mcrypt_encrypt(
MCRYPT_RIJNDAEL_256,
$sSecretKey, $sValue,
MCRYPT_MODE_ECB,
mcrypt_create_iv(
mcrypt_get_iv_size(
MCRYPT_RIJNDAEL_256,
MCRYPT_MODE_ECB
),
MCRYPT_RAND)
)
), "\0"
);
}
function fnDecrypt($sValue, $sSecretKey)
{
return rtrim(
mcrypt_decrypt(
MCRYPT_RIJNDAEL_256,
$sSecretKey,
base64_decode($sValue),
MCRYPT_MODE_ECB,
mcrypt_create_iv(
mcrypt_get_iv_size(
MCRYPT_RIJNDAEL_256,
MCRYPT_MODE_ECB
),
MCRYPT_RAND
)
), "\0"
);
}
I could say...
> Oh, developers should KNOW that MCRYPT_RIJNDAEL_256 isn't actually AES
> and they want MCRYPT_RIJNDAEL_128 with a 32-byte key. And ECB mode is
> a terrible mode for encryption which doesn't even use the IV, but developers
> should choose a better one like 'ctr' (there's no MCRYPT_* constant for
> counter
> mode) or MCRYPT_MODE_CBC. Also, MCRYPT_RAND exposes a non-CS
> PRNG and rtrim() on decrypt exposes padding oracles. But developers
> should know better than to implement something like this.
... but anyone who has ever visited Stack Overflow will know that devs
can and DO write
lazy/bad code, then share it with their friends.
class Encrypter {
private static $Key = "dublin";
public static function encrypt ($input) {
$output = base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256,
md5(Encrypter::$Key), $input, MCRYPT_MODE_CBC,
md5(md5(Encrypter::$Key))));
return $output;
}
public static function decrypt ($input) {
$output = rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256,
md5(Encrypter::$Key),
base64_decode($input), MCRYPT_MODE_CBC,
md5(md5(Encrypter::$Key))), "\0");
return $output;
}
}
> Oh, developers shouldn't use md5($key) as the IV in CBC mode.
At what point do we stop blaming the developers for not knowing how to
use our badly designed features, and accept responsibility for
exposing an API that is hostile towards simple, efficient, and correct
implementations?
That's all I have to say on this matter. I rest my case.
Epilogue / Recommended Reading:
* http://swiftonsecurity.tumblr.com/post/98675308034/a-story-about-jessica
* http://www.gaudior.net/alma/johnny.pdf
Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
