Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:87475
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain paragonie.com from 209.85.192.49 cause and error)
MIME-Version: 1.0
In-Reply-To: <CAH-PCH7RuGxsENBNY=iT01SqN1YyXO=38EjRHFgA8cf5KsOoVg@mail.gmail.com>
References: <CALYT1Yo-ae+cemXaaXs5Jv5t2DSzdxdYuxuhEGbXyoo_863+bw@mail.gmail.com>
	<CAH-PCH6Cbr0DeY3Z=Ju0PfxKggeOCZaGbLLRrYD99GEXWO_j6Q@mail.gmail.com>
	<CAKws9z3N=8cx8JLEfy7O=d+4jR-aLz70z3-CaYUazsk_9NbZuQ@mail.gmail.com>
	<CAH-PCH7RuGxsENBNY=iT01SqN1YyXO=38EjRHFgA8cf5KsOoVg@mail.gmail.com>
Date: Fri, 31 Jul 2015 20:30:07 -0400
Message-ID: <CAKws9z0ojJeP7fzW4G5etAs0xaB50E9zgz9jK=tSwMHL5MHAbA@mail.gmail.com>
To: Ferenc Kovacs <tyra3l@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7
From: scott@paragonie.com (Scott Arciszewski)

On Fri, Jul 31, 2015 at 8:23 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote:
>
>
> On Sat, Aug 1, 2015 at 2:00 AM, Scott Arciszewski <scott@paragonie.com>
> wrote:
>>
>> On Fri, Jul 31, 2015 at 6:34 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote:
>> >
>> > On Tue, Jul 14, 2015 at 11:04 PM, Sammy Kaye Powers <me@sammyk.me>
>> > wrote:
>> >
>> > > Hello lovely PHP nerds,
>> > >
>> > > 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 securit=
y
>> > > 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 Exceptio=
n
>> > > 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. :)
>> > >
>> > > Thanks,
>> > > Sammy Kaye Powers
>> > > sammyk.me
>> > >
>> > > Chicago, IL 60604
>> > >
>> >
>> > I would vote for E_WARNING and return false.
>> > This can be wrapped in an oop wrapper in userland if somebody prefers
>> > and
>> > exception but would still keep the procedural style as first class
>> > citizen.
>> > Plus this would be consistent with other security/crypto related error=
s
>> > like mcrypt_encrypt() getting an invalid key/iv
>> > Nikita, Anthony what do you think?
>> >
>> > --
>> > Ferenc Kov=C3=A1cs
>> > @Tyr43l - http://tyrael.hu
>>
>> Your vote is for apps to be insecure by default.
>
>
> my vote is pragmatic.
>
>>
>>
>> > This can be wrapped in an oop wrapper in userland if somebody prefers
>> > and
>> > exception but would still keep the procedural style as first class
>> > citizen.
>>
>> Nobody's going to do that though. The end result is going to be less
>> security because of a cargo cult devotion to consistency.
>
>
> if you want to change the status quo you have to show a decent case how t=
his
> is different than any other crypto related function.
> you can't because it isn't.
>
>>
>>
>> This should be secure by default. The most secure way for an RNG to
>> fail is to interrupt the application. This means:
>>
>> * E_ERROR
>> * throw new Exception (or a subclass)
>> * throw new Error (or a subclass)
>
>
> for some applications this is the best way, for some other applications i=
t's
> none, because they have another source of entropy, or because they want t=
o
> retry or fail gracefully.
> fatal errors should be used when there are no way to continue the executi=
on,
> for this function it isn't always the case.
>
>>
>>
>> Exceptions and Errors have the advantage that a developer who wants to
>> go out of their way to handle them can simply do this:
>>
>>     function randomPassword($length, $alphabet =3D
>> 'abcdefghijklmnopqrstuvwxyz')
>>     {
>>         $sizeOfAlphabetMinusOne =3D strlen($alphabet) - 1;
>>         try {
>>             for ($i =3D 0; $i < $length; ++$i) {
>>                 $password .=3D $alphabet[random_int(0,
>> $sizeOfAlphabetMinusOne)];
>>             }
>>         } catch (Error $e) {
>>             return $this->framework->stylizedErrorMessage("RNG failure
>> message here");
>>         }
>>         return $password;
>>     }
>>
>> Care to guess what returning false will do for $password?
>
>
> I don't have to guess, it is clear, I also understand where are you comin=
g
> from, but I think your suggestion is too strict and it is fairly trivial =
to
> show similar misuse for any feature yet we have to draw a line somewhere =
as
> we can't save the developer from him/herself always.
>
>>
>>
>> Any cryptography-related implementation needs to fail closed, not fail
>> open.
>
>
> yet every entropy source function used by random_bytes() returns failure =
on
> error instead of calling exit.
> developer > application > library
> your library shouldn't guess about the intentions of the applications, no=
r
> should the application guess about the intentions of the developer.
> you create clean contracts and educate your developers to use those
> properly.
>
>>
>>
>> By raising E_WARNING and returning false, you are placing an extra
>> responsibility on the developer.
>
>
> not extra, but see above.
>
>>
>>
>> Or as Daniel J. Bernstein would put it, YOU ARE BLAMING THE IMPLEMENTOR.
>>
>> Ask any competent application security expert, they'll back me up.
>> Don't enforce insecure defaults just because it's more "consistent".
>
>
> https://en.wikipedia.org/wiki/No_true_Scotsman
>>
>>
>> Consistency is important, sure, but security is MORE important.
>
>
> clear contract is more important and having consistency saves the develop=
er
> from the surprises which can be a good source of bugs.
>
>>
>>
>> Also, death to libmcrypt:
>>
>> https://paragonie.com/blog/2015/05/if-you-re-typing-word-mcrypt-into-you=
r-code-you-re-doing-it-wrong
>
>
> agree.

"This is how we've always done it, so it must be the right decision!
Let's not do things differently even when it would provably force
better security habits on lazy developers in a language notorious for
security holes."

That's how I characterize your argument right now.

If a developer has a different intention, they need to simply wrap it
in a try-catch block and handle it as they please.

But hey, what do I expect from someone who runs WordPress on their
personal website? Certainly not security-conscious decisions.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>