Core functions throwing exceptions in PHP7

10 years ago by Dean Eigenmann — view source — reply

unread

Would a PHP Error not work in this case? Or would the error then be interpreted as the result?

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 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. :)

Thanks,
Sammy Kaye Powers
sammyk.me

Chicago, IL 60604

10 years ago by Scott Arciszewski — view source — reply

unread

On Tue, Jul 14, 2015 at 5:10 PM, Dean Eigenmann
dean.eigenmann@icloud.com wrote:

Would a PHP Error not work in this case? Or would the error then be interpreted as the result?

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 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. :)

Thanks,
Sammy Kaye Powers
sammyk.me

Chicago, IL 60604

--

How can a developer gracefully and simply handle a fatal error?
Recommending that developers mess with set_error_handler() for one
very specific function seems like a bad design choice in my opinion.
An uncaught exception should kill the script, a caught exception can
let the developer decide how to proceed.

I'd vote in favor of Exceptions (which, by default, will fail closed
but can be handled gracefully through try-catch blocks) rather than
E_WARNING (which fails open) or E_ERROR (which doesn't allow for
graceful handling).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Sammy,

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
yohgaki@ohgaki.net

10 years ago by Scott Arciszewski — view source — reply

unread

Hi Sammy,

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
yohgaki@ohgaki.net

-1 I emphatically disagree.

I understand the risk, but users should handle all errors properly
to be secure anyway.

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:

E_WARNING - fail open, possibly cause security problems for the user
E_ERROR - fail closed, but make graceful handling a pain in the neck
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.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Scott,

On Wed, Jul 15, 2015 at 7:19 PM, Scott Arciszewski scott@paragonie.com
wrote:

Hi Sammy,

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
yohgaki@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:

E_WARNING - fail open, possibly cause security problems for the user

E_ERROR - fail closed, but make graceful handling a pain in the neck

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
yohgaki@ohgaki.net

10 years ago by Scott Arciszewski — view source — reply

unread

Hi Scott,

On Wed, Jul 15, 2015 at 7:19 PM, Scott Arciszewski scott@paragonie.com
wrote:
Hi Sammy,

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
yohgaki@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:

E_WARNING - fail open, possibly cause security problems for the user

E_ERROR - fail closed, but make graceful handling a pain in the neck

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
yohgaki@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:

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

E_WARNING - fail open, possibly cause security problems for the user

E_ERROR - fail closed, but make graceful handling a pain in the neck

Can't you just catch Error just as you would catch an Exception? Of
course, it's less clean than specialized exception but I don't see why
it's not an option.

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Rowan Collins — view source — reply

unread

Sammy Kaye Powers wrote on 14/07/2015 22:04:

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.

Given that the engine itself now throws exceptions, and we have
bundled extensions which throw exceptions, the blanket ban on exceptions
"in core functions" seems increasingly out-dated. This particular
function is in ext/standard, which is about as "core" as you can get,
but it is a brand new function with no BC considerations and an explicit
mandate to be as secure as possible.

To me, using an exception here makes perfect sense, but I'm not sure how
to formulate a policy that allows that without opening the floodgates
too quickly. I'm also a big fan of proper exception hierarchies - I've
seen so many times that you should never catch the base Exception
class, so throwing a base Exception seems very wrong to me.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rowan,

On Wed, Jul 15, 2015 at 7:57 PM, Rowan Collins rowan.collins@gmail.com
wrote:

Sammy Kaye Powers wrote on 14/07/2015 22:04:

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.

Given that the engine itself now throws exceptions, and we have bundled
extensions which throw exceptions, the blanket ban on exceptions "in core
functions" seems increasingly out-dated. This particular function is in
ext/standard, which is about as "core" as you can get, but it is a brand
new function with no BC considerations and an explicit mandate to be as
secure as possible.

To me, using an exception here makes perfect sense, but I'm not sure how
to formulate a policy that allows that without opening the floodgates too
quickly. I'm also a big fan of proper exception hierarchies - I've seen so
many times that you should never catch the base Exception class, so
throwing a base Exception seems very wrong to me.

As I wrote earlier, I prefer exceptions rather than errors.

Engine exception is a little different. The main motivation for engine
exception is
to give a chance to users for graceful program termination. Functions can
achieve
the objective by E_RECOVERABLE_ERROR mostly.

I don't want to see one function raise exception and the other raise error.
It's "should be avoided inconsistency". IMHO.
If we are going to adopt exception for "functions", it would be better to
have
switch that convert all errors to exceptions.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Rowan Collins — view source — reply

unread

Yasuo Ohgaki wrote on 15/07/2015 12:20:

Engine exception is a little different. The main motivation for engine
exception is
to give a chance to users for graceful program termination. Functions
can achieve
the objective by E_RECOVERABLE_ERROR mostly.

This is simply incorrect, as demonstrated by conversion of existing
E_RECOVERABLE_ERROR states in the engine to Throwable, and nullified
RFCs which would have, had EngineExceptions not passed, convert
engine-issued E_ERRORs into E_RECOVERABLE_ERROR.

If we are going to adopt exception for "functions", it would be better
to have
switch that convert all errors to exceptions.

Strongly disagree. A runtime switch would be a horrible consistency
nightmare for userland, and many of the existing functions which
desperately need better error-handling (e.g. file I/O) don't issue an
error at the moment anyway.

The only reason (and it's a massive reason, don't get me wrong) not to
start throwing exceptions all over the core is backwards compatibility.
New functions do not, by definition, have anything to be backwards
compatible with.

So my suggestion is, to come up with some reasonable definition of "new
area of functionality" where we can do things in a way which everyone
seems to agree is basically better, i.e. throw Exceptions.

The rule shouldn't apply for new variations or close cousins of
existing functions (e.g. the json_decode_to() that was briefly discussed
should use similar conventions to json_decode() if it were implemented),
only in cases like this where we're adding a new set of functions (the
manual puts random_bytes and random_int in their own "book":
http://php.net/manual/en/book.csprng.php).
It also shouldn't be used for "routine" errors (like File Not Found),
only in place of conditions which can justify a fatal error - both for
the intuitive reason that an uncaught exception is a fatal error, and
the procedural reason that we may want to decide a policy for "routine"
error handling at a later date.

At some point, we should revisit the general error-handling strategy of
both the language and the core library, but 7.0 having been somewhat
derailed (in my opinion) by the endless debates about scalar type hints,
it will have to wait until 8.0 before we can do anything radical to
existing functions.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rowan,

On Wed, Jul 15, 2015 at 9:00 PM, Rowan Collins rowan.collins@gmail.com
wrote:

If we are going to adopt exception for "functions", it would be better to

have
switch that convert all errors to exceptions.

Strongly disagree. A runtime switch would be a horrible consistency
nightmare for userland, and many of the existing functions which
desperately need better error-handling (e.g. file I/O) don't issue an error
at the moment anyway.

The only reason (and it's a massive reason, don't get me wrong) not to
start throwing exceptions all over the core is backwards compatibility. New
functions do not, by definition, have anything to be backwards compatible
with.

So my suggestion is, to come up with some reasonable definition of "new
area of functionality" where we can do things in a way which everyone seems
to agree is basically better, i.e. throw Exceptions.

The rule shouldn't apply for new variations or close cousins of existing
functions (e.g. the json_decode_to() that was briefly discussed should use
similar conventions to json_decode() if it were implemented), only in cases
like this where we're adding a new set of functions (the manual puts
random_bytes and random_int in their own "book":
http://php.net/manual/en/book.csprng.php).

It also shouldn't be used for "routine" errors (like File Not Found),
only in place of conditions which can justify a fatal error - both for the
intuitive reason that an uncaught exception is a fatal error, and the
procedural reason that we may want to decide a policy for "routine" error
handling at a later date.

At some point, we should revisit the general error-handling strategy of
both the language and the core library, but 7.0 having been somewhat
derailed (in my opinion) by the endless debates about scalar type hints, it
will have to wait until 8.0 before we can do anything radical to existing
functions.

I prefer exception also. I agree with your discussion in general.
The only difference is how PHP module functions should adopt exceptions.

When PHP adopted naming standard, we didn't enforce new naming standard
for all functions. As a result, we have "module_func_name" and
"modulefuncname"
still. The same thing will happen for exception/error.

I would like to avoid such situation (at least for modules provided with
PHP).
i.e. One function raises exception, another raises error.

I understand many of us dislike INI switch nor runtime switch that changes
behavior. However, it's impossible to move to exception at once because we
have no control for users' code including 3rd party modules.

IMHO. Rather than adding exception adopted functions gradually, having the
switch
is better for in the long run. We may deprecate/remove the switch 10 years
later or more.

BTW, I'm thinking to have
"ModulenameException"/"ModulenameFunctionException"
or like rather than simply calling/converting errors to ErrorException when
exception
is enabled for functions.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Pierre Joye — view source — reply

unread

I understand many of us dislike INI switch nor runtime switch that changes
behavior. However, it's impossible to move to exception at once because we
have no control for users' code including 3rd party modules.

Well, ini settiing remains ugly. Code will have to take care of it and
this is not going to be a good thing.

IMHO. Rather than adding exception adopted functions gradually, having the
switch
is better for in the long run. We may deprecate/remove the switch 10 years
later or more.

BTW, I'm thinking to have
"ModulenameException"/"ModulenameFunctionException"
or like rather than simply calling/converting errors to ErrorException when
exception
is enabled for functions.

I am not sure I like this idea. I would prefer to define namespace and
rely on standard names (when possible) and per extension namespace
instead.

I also agree to actually deal with exceptions globally at once instead
of incremental additions in more or less random places, functions or
methods.

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

Given that the engine itself now throws exceptions, and we have
bundled extensions which throw exceptions, the blanket ban on exceptions
"in core functions" seems increasingly out-dated. This particular

I agree. I think once we agreed we want to convert from fatals to
exceptions, we need to recognize exceptions as a first-class citizen.
Which means using it in internal functions is OK.

Now, there is a big gap between E_WARNING on one side and
E_ERROR/exception on the other side. Whether particular function crosses
this gap or not is a case-by-case discussion, but "we can not use
exceptions in core" should no longer be a part of it.

too quickly. I'm also a big fan of proper exception hierarchies - I've
seen so many times that you should never catch the base Exception
class, so throwing a base Exception seems very wrong to me.

Agreed. Maybe we should have something like RuntimeException or Java?
That is pretty generic too, but at least not Exception.

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Brian Moon — view source — reply

unread

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 security hole
in the event the RNG fails and returns false which gets evaluated as 0 in a
cryptographic context.

On the surface, this sounds like a good thing. Although, I question that
if a user is not checking $result === false, then will they end up just
wrapping this in an empty try/catch so their code does not fail? There
is a mechanism to detect the error now.

I question why the cryptographic functions would not force an integer to
be passed. Those should not accept a boolean and evaluate it as false. I
am not sure what functions you are talking about though. Maybe 3rd party
user land code? Accepting a boolean in those cases is a bug in that code
IMO.

--

Brian.

http://brian.moonspot.net/

10 years ago by Rowan Collins — view source — reply

unread

Brian Moon wrote on 16/07/2015 17:19:

On the surface, this sounds like a good thing. Although, I question
that if a user is not checking $result === false, then will they end
up just wrapping this in an empty try/catch so their code does not
fail? There is a mechanism to detect the error now.

I question why the cryptographic functions would not force an integer
to be passed. Those should not accept a boolean and evaluate it as
false. I am not sure what functions you are talking about though.
Maybe 3rd party user land code? Accepting a boolean in those cases is
a bug in that code IMO.

Scott provided an example elsewhere in the thread:

 $max = strlen($alphabet) - 1;
 for ($i = 0; $i < 32; ++$i) {
     $password .= $alphabet[random_int(0, $max)];
 }

That demonstrates both a situation where booleans can't be excluded, and
where you'd have to try pretty hard to silently catch the exception in a
way that left your code broken. I suppose you could forget to check if
the generated password is empty, but that doesn't seem all that likely.
If you've gone to the trouble of putting a try block in, you're at least
aware that the function CAN fail.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

On the surface, this sounds like a good thing. Although, I question that
if a user is not checking $result === false, then will they end up just
wrapping this in an empty try/catch so their code does not fail? There
is a mechanism to detect the error now.

True, but not checking for false is an error of omission, and an easy
one to make ("How can random fail? It's just creating random numbers! I
won't bother to check"), while wrapping in try/catch has to be done
explicitly - and can be easily caught on review, even often with
automatic tools, and people can be trained not to do empty try/catches
(it is much easier to train for "do not do this specific thing" than
"always do these 1000 things in 1000 different special cases").

I question why the cryptographic functions would not force an integer to
be passed. Those should not accept a boolean and evaluate it as false. I
am not sure what functions you are talking about though. Maybe 3rd party
user land code? Accepting a boolean in those cases is a bug in that code
IMO.

Given PHP is a weakly-typed language, I don't thing you can rely on this
kind of type checking. While in many cases we can tell people "just
check for errors or accept the fallout", when we come to security I
think the price is too high.

Stas Malyshev
smalyshev@gmail.com

10 years ago by Larry Garfield — view source — reply

unread

Hi!

On the surface, this sounds like a good thing. Although, I question that
if a user is not checking $result === false, then will they end up just
wrapping this in an empty try/catch so their code does not fail? There
is a mechanism to detect the error now.
True, but not checking for false is an error of omission, and an easy
one to make ("How can random fail? It's just creating random numbers! I
won't bother to check"),

Another point here is that 0 is a perfectly legitimate random number in
many cases, so callers would need to do a === check, not just a boolean
check. How many people already forget to do that for strpos() on a daily
basis? Too many. :-)

Whatever the exact mechanism, a "break hard so you can't continue"
approach seems far better in this case than a "return false and hope the
user knows what they're doing", even if the latter is currently more
common in PHP itself. Especially as we're talking not about a user
error but a "your system is not setup right so it will never work"
situation, as I understand it.

--Larry Garfield

10 years ago by kelerest123@gmail.com — view source — reply

unread

Hi Larry!

2015-07-26 1:29 GMT+02:00 Larry Garfield larry@garfieldtech.com:

Another point here is that 0 is a perfectly legitimate random number in many
cases, so callers would need to do a === check, not just a boolean check.
What "boolean check" are you talkin' about? I've never seen a code
using e.g. strpos() like follows:

<?php

if(!is_bool($pos = strpos('foobar', 'baz'))) {
// we are correct, use $pos' value somewhere
} else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

Rather it is most frequently being done like below:

<?php

if(FALSE !== $pos = strpos('foobar', 'baz')) {
// we are correct, use $pos' value somewhere
} else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

I think in both cases you do a kind of "boolean check".

Especially as we're talking not about a user error but a "your system is not setup
right so it will never work" situation, as I understand it.
So, I generally agree with this approach. It is a better thing to
always fail closed if your system isn't set up, e.g. missing some
required things, to work properly. It's the same if your code uses
some vendor extension not included in the core by default -- it can
not work work properly without having this extension available.

I say +1 for raising a E_ERROR on random_int()/random_bytes() fail.

Best regards,
Kubo2, Jakub Kubíček

10 years ago by Larry Garfield — view source — reply

unread

Hi Larry!

2015-07-26 1:29 GMT+02:00 Larry Garfield larry@garfieldtech.com:

Another point here is that 0 is a perfectly legitimate random number in many
cases, so callers would need to do a === check, not just a boolean check.
What "boolean check" are you talkin' about? I've never seen a code
using e.g. strpos() like follows:

<?php

if(!is_bool($pos = strpos('foobar', 'baz'))) {
// we are correct, use $pos' value somewhere
} else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

Rather it is most frequently being done like below:

<?php

if(FALSE !== $pos = strpos('foobar', 'baz')) {
// we are correct, use $pos' value somewhere
} else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

I think in both cases you do a kind of "boolean check".

Yes, I am referring to the second, or variations therein. When using
strpos() you need to do a strict check against FALSE (===) to avoid a
legit 0 getting misinterpreted. It's also a very commonly forgotten
check, especially among newer devs that haven't been burned by it a few
times.

My point is that when talking about crypto-related functions, relying on
people to get burned a few times and then remember to do a === check
against FALSE is a horrible, horrible idea, hence random_int() should
fail much harder, as is being suggested here (be that via E_FATAL,
exception, error throw, or whatever, as long as the execution does NOT
continue). That is, I am agreeing with the proposal to have it die
hard. I moderately prefer the throw SecurityError approach as it's not
a user-input error but a system config error, but anything that prevents
execution from continuing is acceptable security-wise.

--Larry Garfield

10 years ago by Anatol Belski — view source — reply

unread

Hi,

-----Original Message-----
From: Larry Garfield [mailto:larry@garfieldtech.com]
Sent: Sunday, July 26, 2015 9:38 PM
To: internals@lists.php.net
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7

Hi Larry!

2015-07-26 1:29 GMT+02:00 Larry Garfield larry@garfieldtech.com:

Another point here is that 0 is a perfectly legitimate random number
in many cases, so callers would need to do a === check, not just a boolean
check.
What "boolean check" are you talkin' about? I've never seen a code
using e.g. strpos() like follows:

<?php

if(!is_bool($pos = strpos('foobar', 'baz'))) {
// we are correct, use $pos' value somewhere } else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

Rather it is most frequently being done like below:

<?php

if(FALSE !== $pos = strpos('foobar', 'baz')) {
// we are correct, use $pos' value somewhere } else {
// strpos() produced a boolean, thus no 'baz' found in 'foobar'
}

?>

I think in both cases you do a kind of "boolean check".

Yes, I am referring to the second, or variations therein. When using
strpos() you need to do a strict check against FALSE (===) to avoid a legit 0
getting misinterpreted. It's also a very commonly forgotten check, especially
among newer devs that haven't been burned by it a few times.

My point is that when talking about crypto-related functions, relying on people
to get burned a few times and then remember to do a === check against FALSE is
a horrible, horrible idea, hence random_int() should fail much harder, as is being
suggested here (be that via E_FATAL, exception, error throw, or whatever, as
long as the execution does NOT continue). That is, I am agreeing with the
proposal to have it die hard. I moderately prefer the throw SecurityError
approach as it's not a user-input error but a system config error, but anything
that prevents execution from continuing is acceptable security-wise.

When implementing a security related code, people that are tired or asleep should stay home :) But going seriously - what it is about is changing the paradigm. IMHO this kind of thing should not be done making a special case. Since Trowable is a good base, there is a need on a strategy.

The keyword about the "exception hierarchy" is not seldom to hear it this regard nowadays. So besides SecurityError, what is with IOError, MemoryError, other common cases? Also be aware and prepared that many cases can't currently be served by exceptions, the very low level errors. But basically what I would refer to is a concept catching the common cases, so then

we have a set of rules about exceptions in functions
those rules produce consistent behaviors
the "case by case basis" argument is eliminated by them in 99% of cases

Regards

Anatol

10 years ago by Aaron Piotrowski — view source — reply

unread

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 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. :)

Thanks,
Sammy Kaye Powers
sammyk.me

Chicago, IL 60604

How about instead of throwing an instance of Exception, the random_*() functions throw an instance of Error on failure. A subclass of Error, such as SecurityError could also be added. As it is unlikely that the failure of these functions to generate a random value could be handled at runtime, throwing an instance of Error makes the most sense imho.

Many internal functions can result in an instance of Error being thrown, particularly with declare(strict_types=1). So those looking for consistency can be satisfied that other internal functions already can behave similarly, and those looking to fail closed can be satisfied that an exception will be thrown if securely generating a random value fails.

Aaron Piotrowski

10 years ago by Scott Arciszewski — view source — reply

unread

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 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. :)

Thanks,
Sammy Kaye Powers
sammyk.me

Chicago, IL 60604

How about instead of throwing an instance of Exception, the random_*() functions throw an instance of Error on failure. A subclass of Error, such as SecurityError could also be added. As it is unlikely that the failure of these functions to generate a random value could be handled at runtime, throwing an instance of Error makes the most sense imho.

Many internal functions can result in an instance of Error being thrown, particularly with declare(strict_types=1). So those looking for consistency can be satisfied that other internal functions already can behave similarly, and those looking to fail closed can be satisfied that an exception will be thrown if securely generating a random value fails.

Aaron Piotrowski

I must have overlooked a detail here.

According to https://github.com/tpunt/PHP7-Reference#throwable-interface
there are Throwables called Error, as a separate designation from an
exception. I didn't see this in the engine exceptions RFC, so I was
unaware that was even a thing.

In this case, yes, as long as you can wrap it in try/catch blocks,
SecurityError which extends Error and/or implements Throwable is an
excellent suggestion.

Previously, I thought the suggestion was to stick to triggering errors
(E_ERROR, E_RECOVERABLE_ERROR, etc.).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Aaron Piotrowski — view source — reply

unread

I must have overlooked a detail here.

According to https://github.com/tpunt/PHP7-Reference#throwable-interface
there are Throwables called Error, as a separate designation from an
exception. I didn't see this in the engine exceptions RFC, so I was
unaware that was even a thing.

In this case, yes, as long as you can wrap it in try/catch blocks,
SecurityError which extends Error and/or implements Throwable is an
excellent suggestion.

Previously, I thought the suggestion was to stick to triggering errors
(E_ERROR, E_RECOVERABLE_ERROR, etc.).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--

I believe some were suggesting triggering an E_ERROR, though most E_ERRORs in the engine have been replaced with thrown Error exceptions, so I think using E_ERROR in this case would be inappropriate.

As I suggested in my prior email, I think throwing an instance of Error would be appropriate when the functions random_bytes() and random_int() fail.

There are several conditions that already cause the engine to throw an Error (or subclass thereof):

(1)->method(); // Throws Error
declare(strict_types=1); array_map(1, 1); // Throws TypeError
require 'file-with-parse-error.php'; // Throws ParseError
eval("$a[ = 1;"); // Throws ParseError
1 << -1; // Throws ArithmeticError
intdiv(1, 0); // Throws DivisionByZeroError
1 % 0; // Throws DivisionByZeroError

Of particular interest in the above examples is intdiv(), an internal function that can throw an instance of Error if the denominator is zero.

I propose that random_bytes() and random_int() should throw an instance of Error if the parameters are not as expected or if generating a random number fails. (To avoid further debate about a subclass, the function should throw just a plain instance of Error, it can always be subclassed later without BC concerns).

random_bytes() and random_int() failing closed is very important to prevent misguided or lazy programmers from using false in place of a random value. A return of false can easily be overlooked and unintentionally be cast to a zero or empty string. A thrown instance of Error must be purposely caught and ignored to produce the same behavior. As Larry pointed out, it is a very common error for programmers to not do a strict check using === against false when calling strpos().

Does anyone have a strong objection to the above proposal? If not, then I think Sammy should update his PRs to throw an Error so they can be merged before the next beta release.

Aaron Piotrowski

10 years ago by Scott Arciszewski — view source — reply

unread

+1 for Error

I must have overlooked a detail here.

According to https://github.com/tpunt/PHP7-Reference#throwable-interface
there are Throwables called Error, as a separate designation from an
exception. I didn't see this in the engine exceptions RFC, so I was
unaware that was even a thing.

In this case, yes, as long as you can wrap it in try/catch blocks,
SecurityError which extends Error and/or implements Throwable is an
excellent suggestion.

Previously, I thought the suggestion was to stick to triggering errors
(E_ERROR, E_RECOVERABLE_ERROR, etc.).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--

I believe some were suggesting triggering an E_ERROR, though most E_ERRORs
in the engine have been replaced with thrown Error exceptions, so I think
using E_ERROR in this case would be inappropriate.

As I suggested in my prior email, I think throwing an instance of Error
would be appropriate when the functions random_bytes() and random_int()
fail.

There are several conditions that already cause the engine to throw an
Error (or subclass thereof):

(1)->method(); // Throws Error
declare(strict_types=1); array_map(1, 1); // Throws TypeError
require 'file-with-parse-error.php'; // Throws ParseError
eval("$a[ = 1;"); // Throws ParseError
1 << -1; // Throws ArithmeticError
intdiv(1, 0); // Throws DivisionByZeroError
1 % 0; // Throws DivisionByZeroError

Of particular interest in the above examples is intdiv(), an internal
function that can throw an instance of Error if the denominator is zero.

I propose that random_bytes() and random_int() should throw an instance of
Error if the parameters are not as expected or if generating a random
number fails. (To avoid further debate about a subclass, the function
should throw just a plain instance of Error, it can always be subclassed
later without BC concerns).

random_bytes() and random_int() failing closed is very important to
prevent misguided or lazy programmers from using false in place of a random
value. A return of false can easily be overlooked and unintentionally be
cast to a zero or empty string. A thrown instance of Error must be
purposely caught and ignored to produce the same behavior. As Larry pointed
out, it is a very common error for programmers to not do a strict check
using === against false when calling strpos().

Does anyone have a strong objection to the above proposal? If not, then I
think Sammy should update his PRs to throw an Error so they can be merged
before the next beta release.

Aaron Piotrowski

10 years ago by Anatol Belski — view source — reply

unread

Hi Aaron,

-----Original Message-----
From: Aaron Piotrowski [mailto:aaron@icicle.io]
Sent: Monday, July 27, 2015 5:32 AM
To: Sammy Kaye Powers me@sammyk.me; Scott Arciszewski
scott@paragonie.com
Cc: Anatol Belski anatol.php@belski.net; larry@garfieldtech.com; Jakub
Kubíček kelerest123@gmail.com; Stanislav Malyshev
smalyshev@gmail.com; scott@paragonie.com; rowan.collins@gmail.com;
pierre.php@gmail.com; Dean Eigenmann dean.eigenmann@icloud.com;
Yasuo Ohgaki yohgaki@ohgaki.net; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7

I must have overlooked a detail here.

According to
https://github.com/tpunt/PHP7-Reference#throwable-interface
there are Throwables called Error, as a separate designation from an
exception. I didn't see this in the engine exceptions RFC, so I was
unaware that was even a thing.

In this case, yes, as long as you can wrap it in try/catch blocks,
SecurityError which extends Error and/or implements Throwable is an
excellent suggestion.

Previously, I thought the suggestion was to stick to triggering errors
(E_ERROR, E_RECOVERABLE_ERROR, etc.).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--
To unsubscribe,
visit: http://www.php.net/unsub.php

I believe some were suggesting triggering an E_ERROR, though most E_ERRORs
in the engine have been replaced with thrown Error exceptions, so I think
using
E_ERROR in this case would be inappropriate.

As I suggested in my prior email, I think throwing an instance of Error
would be
appropriate when the functions random_bytes() and random_int() fail.

There are several conditions that already cause the engine to throw an
Error (or
subclass thereof):

(1)->method(); // Throws Error
declare(strict_types=1); array_map(1, 1); // Throws TypeError require
'file-with-
parse-error.php'; // Throws ParseError eval("$a[ = 1;"); // Throws
ParseError
1 << -1; // Throws ArithmeticError
intdiv(1, 0); // Throws DivisionByZeroError
1 % 0; // Throws DivisionByZeroError

Of particular interest in the above examples is intdiv(), an internal
function that
can throw an instance of Error if the denominator is zero.

I propose that random_bytes() and random_int() should throw an instance of
Error if the parameters are not as expected or if generating a random
number
fails. (To avoid further debate about a subclass, the function should
throw just a
plain instance of Error, it can always be subclassed later without BC
concerns).

random_bytes() and random_int() failing closed is very important to
prevent
misguided or lazy programmers from using false in place of a random value.
A
return of false can easily be overlooked and unintentionally be cast to a
zero or
empty string. A thrown instance of Error must be purposely caught and
ignored
to produce the same behavior. As Larry pointed out, it is a very common
error
for programmers to not do a strict check using === against false when
calling
strpos().

Does anyone have a strong objection to the above proposal? If not, then I
think
Sammy should update his PRs to throw an Error so they can be merged before
the next beta release.

Yes, there is an objection at least on my side. As we was discussing the PR
on the github page, the primary goal of the suggestion to move the
discussion to the intarnals list was to create an approach about exceptions
in functions. This seems not be the case here. It is not the question
whether exceptions should be thrown in functions - it's almost obvious that
they should now after it's the engine behavior. But it is a huge question of
the consistency with anything else. That was also the reason why me and
Kalle have changed our minds about your PR converting fatals in the
functions.

The only case where exception is thrown in a function is intdiv() now. But
also as mentioned elsewhere - it's something tightly coupled with the
behavior of div/mod in the engine. IMHO it is not building any base for
randomly adding exceptions elsewhere. Neither I don't see as a base that the
CSPRNG functions are new. Neither the argument "we can change it later to
xyz". The main concern is still not addressed, and even wasn't started to be
addressed. It is for nothing to have new "nice" functions with nice and
correct behavior while leaving the old "ugly" functions ugly.

IMO we should stop building special cases and move to the conceptualization
as first. The more special cases exist, the harder it will be in the future
to bring the behaviors inline without BC breaks. Till now I haven't seen
even any gentle hint about such conceptualization work going on, no RFC page
still exists, but it's being continued pushing on a special case. Solving an
immediate issue might be tactically justifiable, but without a strategical
thinking will lead to even a bigger issue. It is not something we want to
leave behind us for the next generations. I would kindly call therefore, to
think about this in a more global way, discuss and show at least a rough
draft about what should be done regarding the existing functions, new
functions, backward compatibility, exact warning/error types and how they
should be converted, etc. before pushing further on this case.

Thanks

Anatol

10 years ago by Scott Arciszewski — view source — reply

unread

My only concern is that, we have a fixed timetable for the 7.0.0
release. It's certainly a good idea to develop a cogent strategy
before moving forward, but I worry that bikeshedding will get involved
and we won't make the deadline.

I propose that, if a better strategy cannot be presented in time for
RC1, consistency should take a back seat to security. Fail hard, throw
an Error, deal with the details later.
Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises

Hi Aaron,

-----Original Message-----
From: Aaron Piotrowski [mailto:aaron@icicle.io]
Sent: Monday, July 27, 2015 5:32 AM
To: Sammy Kaye Powers me@sammyk.me; Scott Arciszewski
scott@paragonie.com
Cc: Anatol Belski anatol.php@belski.net; larry@garfieldtech.com; Jakub
Kubíček kelerest123@gmail.com; Stanislav Malyshev
smalyshev@gmail.com; scott@paragonie.com; rowan.collins@gmail.com;
pierre.php@gmail.com; Dean Eigenmann dean.eigenmann@icloud.com;
Yasuo Ohgaki yohgaki@ohgaki.net; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7

I must have overlooked a detail here.

According to
https://github.com/tpunt/PHP7-Reference#throwable-interface
there are Throwables called Error, as a separate designation from an
exception. I didn't see this in the engine exceptions RFC, so I was
unaware that was even a thing.

In this case, yes, as long as you can wrap it in try/catch blocks,
SecurityError which extends Error and/or implements Throwable is an
excellent suggestion.

Previously, I thought the suggestion was to stick to triggering errors
(E_ERROR, E_RECOVERABLE_ERROR, etc.).

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--
To unsubscribe,
visit: http://www.php.net/unsub.php

I believe some were suggesting triggering an E_ERROR, though most E_ERRORs
in the engine have been replaced with thrown Error exceptions, so I think
using
E_ERROR in this case would be inappropriate.

As I suggested in my prior email, I think throwing an instance of Error
would be
appropriate when the functions random_bytes() and random_int() fail.

There are several conditions that already cause the engine to throw an
Error (or
subclass thereof):

(1)->method(); // Throws Error
declare(strict_types=1); array_map(1, 1); // Throws TypeError require
'file-with-
parse-error.php'; // Throws ParseError eval("$a[ = 1;"); // Throws
ParseError
1 << -1; // Throws ArithmeticError
intdiv(1, 0); // Throws DivisionByZeroError
1 % 0; // Throws DivisionByZeroError

Of particular interest in the above examples is intdiv(), an internal
function that
can throw an instance of Error if the denominator is zero.

I propose that random_bytes() and random_int() should throw an instance of
Error if the parameters are not as expected or if generating a random
number
fails. (To avoid further debate about a subclass, the function should
throw just a
plain instance of Error, it can always be subclassed later without BC
concerns).

random_bytes() and random_int() failing closed is very important to
prevent
misguided or lazy programmers from using false in place of a random value.
A
return of false can easily be overlooked and unintentionally be cast to a
zero or
empty string. A thrown instance of Error must be purposely caught and
ignored
to produce the same behavior. As Larry pointed out, it is a very common
error
for programmers to not do a strict check using === against false when
calling
strpos().

Does anyone have a strong objection to the above proposal? If not, then I
think
Sammy should update his PRs to throw an Error so they can be merged before
the next beta release.

Yes, there is an objection at least on my side. As we was discussing the PR
on the github page, the primary goal of the suggestion to move the
discussion to the intarnals list was to create an approach about exceptions
in functions. This seems not be the case here. It is not the question
whether exceptions should be thrown in functions - it's almost obvious that
they should now after it's the engine behavior. But it is a huge question of
the consistency with anything else. That was also the reason why me and
Kalle have changed our minds about your PR converting fatals in the
functions.

The only case where exception is thrown in a function is intdiv() now. But
also as mentioned elsewhere - it's something tightly coupled with the
behavior of div/mod in the engine. IMHO it is not building any base for
randomly adding exceptions elsewhere. Neither I don't see as a base that the
CSPRNG functions are new. Neither the argument "we can change it later to
xyz". The main concern is still not addressed, and even wasn't started to be
addressed. It is for nothing to have new "nice" functions with nice and
correct behavior while leaving the old "ugly" functions ugly.

IMO we should stop building special cases and move to the conceptualization
as first. The more special cases exist, the harder it will be in the future
to bring the behaviors inline without BC breaks. Till now I haven't seen
even any gentle hint about such conceptualization work going on, no RFC page
still exists, but it's being continued pushing on a special case. Solving an
immediate issue might be tactically justifiable, but without a strategical
thinking will lead to even a bigger issue. It is not something we want to
leave behind us for the next generations. I would kindly call therefore, to
think about this in a more global way, discuss and show at least a rough
draft about what should be done regarding the existing functions, new
functions, backward compatibility, exact warning/error types and how they
should be converted, etc. before pushing further on this case.

Thanks

Anatol

10 years ago by Anatol Belski — view source — reply

unread

Hi Scott,

-----Original Message-----
From: Scott Arciszewski [mailto:scott@paragonie.com]
Sent: Monday, July 27, 2015 8:58 AM
To: Anatol Belski anatol.php@belski.net
Cc: Aaron Piotrowski aaron@icicle.io; Sammy Kaye Powers
me@sammyk.me; Larry Garfield larry@garfieldtech.com; Jakub Kubíček
kelerest123@gmail.com; Stanislav Malyshev smalyshev@gmail.com;
rowan.collins@gmail.com; Pierre Joye pierre.php@gmail.com; Dean
Eigenmann dean.eigenmann@icloud.com; Yasuo Ohgaki
yohgaki@ohgaki.net; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7

My only concern is that, we have a fixed timetable for the 7.0.0 release. It's
certainly a good idea to develop a cogent strategy before moving forward, but I
worry that bikeshedding will get involved and we won't make the deadline.

Clear, it is not something arising overnight. And bikeshedding is good because it reveals many potential issues. Clear, it needs time. RC1 might come after beta3 (or might not) which is roughly 1 month from now on. It is in your guys hands.

I propose that, if a better strategy cannot be presented in time for RC1,
consistency should take a back seat to security. Fail hard, throw an Error, deal
with the details later.
Clear, it can't be complete and accepted/declined in such a short period (though much time is lost discussing only this one case). That's why I wrote literally about a "rough draft". But it also can't arise if no one cares. Then yes - we end up in a situation like "A over B" or "B over C" without really knowing the consequences.

Cheers

Anatol

10 years ago by Rowan Collins — view source — reply

unread

Scott Arciszewski wrote on 27/07/2015 07:57:

My only concern is that, we have a fixed timetable for the 7.0.0
release. It's certainly a good idea to develop a cogent strategy
before moving forward, but I worry that bikeshedding will get involved
and we won't make the deadline.

I propose that, if a better strategy cannot be presented in time for
RC1, consistency should take a back seat to security. Fail hard, throw
an Error, deal with the details later.

I think taking on board Anatol's concerns about proper planning and
consistency, and yours about short timescales, the reasonable solution
is to create as small a possibility for future BC as possible, while
maintaining the "fail hard" policy.

My suggestion would therefore be to raise an E_ERROR for this case in
7.0. Unlike an E_RECOVERABLE_ERROR or any kind of Throwable, there is no
way that code can be written to explicitly handle that case, it will
simply halt execution. As such, there are no limits to what we can
introduce later, other than that unmodified code should continue to fail
hard.

This buys us time to put together a cogent policy for what if any
built-in functions can make use of Throwables in the 7.x release series,
with a likely result of throwing some class of Error for this function
in 7.1.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Scott Arciszewski — view source — reply

unread

Scott Arciszewski wrote on 27/07/2015 07:57:

My only concern is that, we have a fixed timetable for the 7.0.0
release. It's certainly a good idea to develop a cogent strategy
before moving forward, but I worry that bikeshedding will get involved
and we won't make the deadline.

I propose that, if a better strategy cannot be presented in time for
RC1, consistency should take a back seat to security. Fail hard, throw
an Error, deal with the details later.

I think taking on board Anatol's concerns about proper planning and
consistency, and yours about short timescales, the reasonable solution is to
create as small a possibility for future BC as possible, while maintaining
the "fail hard" policy.

My suggestion would therefore be to raise an E_ERROR for this case in 7.0.
Unlike an E_RECOVERABLE_ERROR or any kind of Throwable, there is no way that
code can be written to explicitly handle that case, it will simply halt
execution. As such, there are no limits to what we can introduce later,
other than that unmodified code should continue to fail hard.

This buys us time to put together a cogent policy for what if any built-in
functions can make use of Throwables in the 7.x release series, with a
likely result of throwing some class of Error for this function in 7.1.

Regards,

Rowan Collins
[IMSoP]

--

The problem with fatal errors is that "This function can fail
irrecoverably outside of your control" isn't going to encourage
adoption.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises

10 years ago by Rowan Collins — view source — reply

unread

Scott Arciszewski wrote on 27/07/2015 15:45:

The problem with fatal errors is that "This function can fail
irrecoverably outside of your control" isn't going to encourage
adoption.

Well... rats! ;)

It seems whatever we do here is going to carry some kind of risk /
downside. :(

10 years ago by Scott Arciszewski — view source — reply

unread

Scott Arciszewski wrote on 27/07/2015 15:45:

The problem with fatal errors is that "This function can fail
irrecoverably outside of your control" isn't going to encourage
adoption.

Well... rats! ;)

It seems whatever we do here is going to carry some kind of risk / downside.
:(

Aye, unfortunately, we need to decide what our priorities are.

I've said my piece before, but it bears repeating: If you are going to
offer a Cryptographically Secure Pseudo-Random Number Generator
feature, you need it to be secure first and foremost. An E_ERROR
satisfies this requirement (it fails closed), but it's a horrible UX
decision.

There's an old trope that security and usability are opposites. This
isn't necessarily true.

I understand that some of the Internals contributors are allergic to
special cases. There's some validity to wanting the language to be
predictable and consistent across the board, because that might be a
UX concern in and of itself. (What do you mean this one function has
to be wrapped in try-catch blocks when the others can be prefixed with
@ to silence errors? I'm telling PHPSadness!)

However, there's already a precedent with a core function throwing a
Throwable (i.e. intdiv()), so making a special case for random_bytes()
and random_int() doesn't introduce an unprecedented inconsistency.

My priorities can be summed up as follows:

Don't fail open.
Don't make developers want to avoid a secure RNG like the plague.
Although it hasn't come up yet: Don't scare or mislead developers
when documenting these features.

Or in other words:

Security >= User Friendly > Consistency with the rest of the language features

Your priorities might be different. That's okay, monocultures lead to fragility.

But the CSPRNG is a security feature and security should come first.
Any deviation from this priority, in the context of a security
feature, is a high caliber foot-bullet.

I strongly recommend anyone who argues against Exceptions consider how
their proposal will affect security and the adoption of the new API in
their response.

I'm open to change and being proven wrong (i.e. with Errors not
meaning E_ERROR earlier in this thread), but don't cripple a security
feature because "I want consistency at any cost."

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Rowan Collins — view source — reply

unread

Scott Arciszewski wrote on 27/07/2015 18:35:

I understand that some of the Internals contributors are allergic to
special cases. There's some validity to wanting the language to be
predictable and consistent across the board, because that might be a
UX concern in and of itself. (What do you mean this one function has
to be wrapped in try-catch blocks when the others can be prefixed with
@ to silence errors? I'm telling PHPSadness!)

This is certainly some people's concern, but Anatol has raised a subtly
different consistency-related point, which is this:

Since we have no policy for what kinds of Throwable should be emitted in
what circumstance, throwing anything in this function sets a precedent
which will have to be incorporated in any future plan.

Assuming nobody is fundamentally against ever adding Throwables to core
functions, there are a minimum number of questions that need to be
agreed before adding the first one:

when should we inherit from Error and when from Exception?
is it ever OK to throw a plain Error or Exception (thus forcing users
into the otherwise bad practice of catching those base classes)?
if not throwing the base class, how specific should sub-classes be?
(i.e. a framework for defining the hierarchy, not necessarily the
hierarchy itself)

If we can get agreement on those points in time for 7.0, fine, but time
is very tight, and the window for such discussions has theoretically
closed...

Regards,

Rowan Collins
[IMSoP]

10 years ago by Anthony Ferrara — view source — reply

unread

Rowan,

This is certainly some people's concern, but Anatol has raised a subtly
different consistency-related point, which is this:

Since we have no policy for what kinds of Throwable should be emitted in
what circumstance, throwing anything in this function sets a precedent which
will have to be incorporated in any future plan.

Assuming nobody is fundamentally against ever adding Throwables to core
functions, there are a minimum number of questions that need to be agreed
before adding the first one:

when should we inherit from Error and when from Exception?

IMHO, Errors signify programmer error, where Exceptions signify
unknown or runtime errors. Meaning that an Error should always be a
problem with your code, but an Exception could be a systems problem, a
user problem or a problem in your code.

While that's slightly off-topic to this discussion, it frames which
type random_* would throw pretty clearly (Exception).

is it ever OK to throw a plain Error or Exception (thus forcing users into
the otherwise bad practice of catching those base classes)?

For now, I think that's a good practice. It doesn't constrain us from
sub-typing down the road (7.1, etc), but it also lets us build the
support in today.

For example, if we throw Exception, in 7.1 we could make it
php\RandomException in 7.1 without issue (all we need to get right is
the hierarchy parent).

if not throwing the base class, how specific should sub-classes be? (i.e.
a framework for defining the hierarchy, not necessarily the hierarchy
itself)

I think this is something that should be RFC'd for 7.1. I don't think
that limits us here though.

If we can get agreement on those points in time for 7.0, fine, but time is
very tight, and the window for such discussions has theoretically closed...

I think the only real agreement we need is Error vs Exception. If we
can agree on one of those, we can do the rest in 7.1 without worrying
about BC...

Anthony

10 years ago by Scott Arciszewski — view source — reply

unread

Rowan,

This is certainly some people's concern, but Anatol has raised a subtly
different consistency-related point, which is this:

Since we have no policy for what kinds of Throwable should be emitted in
what circumstance, throwing anything in this function sets a precedent which
will have to be incorporated in any future plan.

Assuming nobody is fundamentally against ever adding Throwables to core
functions, there are a minimum number of questions that need to be agreed
before adding the first one:

when should we inherit from Error and when from Exception?

IMHO, Errors signify programmer error, where Exceptions signify
unknown or runtime errors. Meaning that an Error should always be a
problem with your code, but an Exception could be a systems problem, a
user problem or a problem in your code.

While that's slightly off-topic to this discussion, it frames which
type random_* would throw pretty clearly (Exception).

is it ever OK to throw a plain Error or Exception (thus forcing users into
the otherwise bad practice of catching those base classes)?

For now, I think that's a good practice. It doesn't constrain us from
sub-typing down the road (7.1, etc), but it also lets us build the
support in today.

For example, if we throw Exception, in 7.1 we could make it
php\RandomException in 7.1 without issue (all we need to get right is
the hierarchy parent).

if not throwing the base class, how specific should sub-classes be? (i.e.
a framework for defining the hierarchy, not necessarily the hierarchy
itself)

I think this is something that should be RFC'd for 7.1. I don't think
that limits us here though.

If we can get agreement on those points in time for 7.0, fine, but time is
very tight, and the window for such discussions has theoretically closed...

I think the only real agreement we need is Error vs Exception. If we
can agree on one of those, we can do the rest in 7.1 without worrying
about BC...

Anthony

I'm fine with either Error or Exception. I'd prefer Exception (easier
to write a sane backport for PHP 5.6) but I leave this decision in the
hands of others.

/**
 * Slightly insane PHP 5 backport but it works
 */
class Error extends Exception { } // Done!

Does anybody feel particularly strong about one or the other?

If so, should we set up a vote somewhere? (I don't vote karma on RFCs
etc. so I don't know if the existing infrastructure would work.)

If not, can we get PR 1397 & 1398 merged? :)

Regards,

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Niklas Keller — view source — reply

unread

2015-07-30 19:12 GMT+02:00 Scott Arciszewski scott@paragonie.com:

On Mon, Jul 27, 2015 at 2:03 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:

Rowan,

This is certainly some people's concern, but Anatol has raised a subtly
different consistency-related point, which is this:

Since we have no policy for what kinds of Throwable should be emitted in
what circumstance, throwing anything in this function sets a precedent
which
will have to be incorporated in any future plan.

Assuming nobody is fundamentally against ever adding Throwables to core
functions, there are a minimum number of questions that need to be
agreed
before adding the first one:

when should we inherit from Error and when from Exception?

IMHO, Errors signify programmer error, where Exceptions signify
unknown or runtime errors. Meaning that an Error should always be a
problem with your code, but an Exception could be a systems problem, a
user problem or a problem in your code.

While that's slightly off-topic to this discussion, it frames which
type random_* would throw pretty clearly (Exception).

is it ever OK to throw a plain Error or Exception (thus forcing users
into
the otherwise bad practice of catching those base classes)?

For now, I think that's a good practice. It doesn't constrain us from
sub-typing down the road (7.1, etc), but it also lets us build the
support in today.

For example, if we throw Exception, in 7.1 we could make it
php\RandomException in 7.1 without issue (all we need to get right is
the hierarchy parent).

if not throwing the base class, how specific should sub-classes be?
(i.e.
a framework for defining the hierarchy, not necessarily the hierarchy
itself)

I think this is something that should be RFC'd for 7.1. I don't think
that limits us here though.

If we can get agreement on those points in time for 7.0, fine, but time
is
very tight, and the window for such discussions has theoretically
closed...

I think the only real agreement we need is Error vs Exception. If we
can agree on one of those, we can do the rest in 7.1 without worrying
about BC...

Anthony

I'm fine with either Error or Exception. I'd prefer Exception (easier
to write a sane backport for PHP 5.6) but I leave this decision in the
hands of others.
/**
 * Slightly insane PHP 5 backport but it works
 */
class Error extends Exception { } // Done!
Does anybody feel particularly strong about one or the other?

If so, should we set up a vote somewhere? (I don't vote karma on RFCs
etc. so I don't know if the existing infrastructure would work.)

If not, can we get PR 1397 & 1398 merged? :)

Regards,

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises https://paragonie.com

--

I prefer Exception, too, because it's I/O related.

@Scott: You can open votes on everything, doesn't matter, just create a
page with a vote.
I just don't know where to put it in the wiki, because it's not a RFC.

Regards, Niklas

10 years ago by Scott Arciszewski — view source — reply

unread

I prefer Exception, too, because it's I/O related.

@Scott: You can open votes on everything, doesn't matter, just create a
page with a vote.
I just don't know where to put it in the wiki, because it's not a RFC.

Regards, Niklas

I'm not sure how to do that.

I have a noncritical security patch I intend to write for random_bytes()
that I would like to submit as a PR but first I'd like to see what the
resolution will be re: Exceptions.

Also, merge conflicts aren't fun. ;)

If there's anything I can do to get those two merged faster, please let me
know.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Scott,

On Fri, Jul 31, 2015 at 6:33 AM, Scott Arciszewski scott@paragonie.com
wrote:

I prefer Exception, too, because it's I/O related.

@Scott: You can open votes on everything, doesn't matter, just create a
page with a vote.
I just don't know where to put it in the wiki, because it's not a RFC.

Regards, Niklas

I'm not sure how to do that.

I have a noncritical security patch I intend to write for random_bytes()
that I would like to submit as a PR but first I'd like to see what the
resolution will be re: Exceptions.

Also, merge conflicts aren't fun. ;)

If there's anything I can do to get those two merged faster, please let me
know.

You can have RFC vote options like

Raise Error/Exception when CSPRNG is not available
(Choice: Yes/No)
Use error(E_RECOVERABLE_ERROR) or exception(Whatever exception prefered)
(Choice: Error/Exception)

BTW, I prefer exception, but consistency is important also.
My vote will be Yes and Error.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Niklas Keller — view source — reply

unread

2015-07-31 9:29 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Scott,

On Fri, Jul 31, 2015 at 6:33 AM, Scott Arciszewski scott@paragonie.com
wrote:

I prefer Exception, too, because it's I/O related.

@Scott: You can open votes on everything, doesn't matter, just create a
page with a vote.
I just don't know where to put it in the wiki, because it's not a RFC.

Regards, Niklas

I'm not sure how to do that.

I have a noncritical security patch I intend to write for random_bytes()
that I would like to submit as a PR but first I'd like to see what the
resolution will be re: Exceptions.

Also, merge conflicts aren't fun. ;)

If there's anything I can do to get those two merged faster, please let me
know.

You can have RFC vote options like

Raise Error/Exception when CSPRNG is not available
(Choice: Yes/No)

Use error(E_RECOVERABLE_ERROR) or exception(Whatever exception
prefered)
(Choice: Error/Exception)

BTW, I prefer exception, but consistency is important also.
My vote will be Yes and Error.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Hi Yasuo,

I think the question is more whether Exception or Error (class, not
E_RECOVERABLE_ERROR), to allow handling it gracefully.

Regards, Niklas

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

I think the question is more whether Exception or Error (class, not
E_RECOVERABLE_ERROR), to allow handling it gracefully.

E_WARNING is too weak for CSPRNG not available.
It's fatal error.

I agree fatal errors should be able to handle gracefully if it is possible.
That's the reason why I prepared a patch that changes session E_ERROR
to E_RECOVERABLE_ERROR.

https://github.com/php/php-src/compare/master...yohgaki:master-e_recoverable_error?expand=1

It's inspired by this thread :)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Niklas Keller — view source — reply

unread

Using set_error_handler isn't handling errors gracefully. Well, it's better
than E_ERROR, but then libraries can't handle those errors gracefully,
because the user might override its error handler by setting an own handler.

Regards, Niklas

2015-07-31 11:46 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

I think the question is more whether Exception or Error (class, not
E_RECOVERABLE_ERROR), to allow handling it gracefully.

E_WARNING is too weak for CSPRNG not available.
It's fatal error.

I agree fatal errors should be able to handle gracefully if it is possible.
That's the reason why I prepared a patch that changes session E_ERROR
to E_RECOVERABLE_ERROR.

https://github.com/php/php-src/compare/master...yohgaki:master-e_recoverable_error?expand=1

It's inspired by this thread :)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

Using set_error_handler isn't handling errors gracefully. Well, it's
better than E_ERROR, but then libraries can't handle those errors
gracefully, because the user might override its error handler by setting an
own handler.

Now I see what do you mean by "gracefully".

TL;DR;
It's app developer jobs to handle these fatal errors.

Most fatal errors shouldn't be recovered by library. e.g. Fallback to non
CSPRNG when CSPRNG is not
available.

IMO, library authors do not have to worry about errors like session's
E_RECOVERABLE_ERROR
or CSPRNG not available. These are fatal errors by its nature and app
developer or system administrator
job to handle them. We cannot blindly assume such fatal errors never
happen in production apps, so we
are better to give users a chance to return "Nice error page/message" when
it happened.

Anyway, we are better to move errors to exceptions at once and force "all"
modules including 3rd party
module to raise exceptions. i.e. Make php_error_docref() raise exceptions.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Niklas Keller — view source — reply

unread

2015-07-31 23:36 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

Using set_error_handler isn't handling errors gracefully. Well, it's
better than E_ERROR, but then libraries can't handle those errors
gracefully, because the user might override its error handler by setting an
own handler.

Now I see what do you mean by "gracefully".

TL;DR;
It's app developer jobs to handle these fatal errors.

Nope.

Most fatal errors shouldn't be recovered by library. e.g. Fallback to non
CSPRNG when CSPRNG is not
available.

They should totally be handled. You need to catch the error and throw a
defined exception, otherwise your public API will break if you choose to
use another internal implementation.
Additionally, you seem to assume that the library doesn't have to do things
like cleanups in such a case.

Regards, Niklas

IMO, library authors do not have to worry about errors like session's
E_RECOVERABLE_ERROR
or CSPRNG not available. These are fatal errors by its nature and app
developer or system administrator
job to handle them. We cannot blindly assume such fatal errors never
happen in production apps, so we
are better to give users a chance to return "Nice error page/message" when
it happened.

Anyway, we are better to move errors to exceptions at once and force "all"
modules including 3rd party
module to raise exceptions. i.e. Make php_error_docref() raise exceptions.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

2015-07-31 23:36 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

Using set_error_handler isn't handling errors gracefully. Well, it's
better than E_ERROR, but then libraries can't handle those errors
gracefully, because the user might override its error handler by setting an
own handler.

Now I see what do you mean by "gracefully".

TL;DR;
It's app developer jobs to handle these fatal errors.

Nope.

Most fatal errors shouldn't be recovered by library. e.g. Fallback to non
CSPRNG when CSPRNG is not
available.

They should totally be handled. You need to catch the error and throw a
defined exception, otherwise your public API will break if you choose to
use another internal implementation.
Additionally, you seem to assume that the library doesn't have to do
things like cleanups in such a case.

My thought is based on Design by Contract (Contract programming).
When parameter or environment does not satisfy contract, contract error
should be
resulted in program/process termination.

Fixing inappropriate parameter or environment is not library/framework
author's
responsibility, but the developer's. i.e. Caller(function/programmer/system
admin)
has the responsibility that satisfies parameter/environment requirement. If
requirement is not met, it's perfectly OK for library/framework to raise
fatal
errors/exceptions. e.g. "You need PHP 5.6 or greater" error.

Handling these fatal errors in a library/framework make code complex
unnecessarily.
More complex code has more chances to have bugs including security related
bugs.
Library/framework may simply raise error/exception telling users "It's
impossible to work".

PHP is general programing language, so we have to consider long life
applications
such as standalone apps. I fully agree that exception is far easier for
handling errors
properly and keep app running. However, making randon_*() a special
function does
not worth it. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

They should totally be handled. You need to catch the error and throw a

defined exception, otherwise your public API will break if you choose to
use another internal implementation.
Additionally, you seem to assume that the library doesn't have to do
things like cleanups in such a case.

My thought is based on Design by Contract (Contract programming).
When parameter or environment does not satisfy contract, contract error
should be
resulted in program/process termination.

Fixing inappropriate parameter or environment is not library/framework
author's
responsibility, but the developer's. i.e.
Caller(function/programmer/system admin)
has the responsibility that satisfies parameter/environment requirement.
If
requirement is not met, it's perfectly OK for library/framework to raise
fatal
errors/exceptions. e.g. "You need PHP 5.6 or greater" error.

I'll be more specific for "CSPRNG not available" error.

If a author would like to handle the error and fallback to non crypt safe
RNG,
he/she should detect environment and execute alternative code for the
environment.

Catching exception and fallback to non crypt safe RNG is not optimal way
for handling unsatisfactory environment. IMHO. If we need function that
checks
environment, we are better to provide one rather than let users to use
exception.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Niklas Keller — view source — reply

unread

2015-08-01 1:43 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

They should totally be handled. You need to catch the error and throw a

defined exception, otherwise your public API will break if you choose to
use another internal implementation.
Additionally, you seem to assume that the library doesn't have to do
things like cleanups in such a case.

My thought is based on Design by Contract (Contract programming).
When parameter or environment does not satisfy contract, contract error
should be
resulted in program/process termination.

Fixing inappropriate parameter or environment is not library/framework
author's
responsibility, but the developer's. i.e.
Caller(function/programmer/system admin)
has the responsibility that satisfies parameter/environment requirement.
If
requirement is not met, it's perfectly OK for library/framework to raise
fatal
errors/exceptions. e.g. "You need PHP 5.6 or greater" error.

I'll be more specific for "CSPRNG not available" error.

If a author would like to handle the error and fallback to non crypt safe
RNG,
he/she should detect environment and execute alternative code for the
environment.

Hi Yasuo,

You always assume the developer just wants to fallback to something
different. You can't
detect the environment btw. because it could just fail because of too many
open file descriptors.

Catching exception and fallback to non crypt safe RNG is not optimal way
for handling unsatisfactory environment. IMHO. If we need function that
checks
environment, we are better to provide one rather than let users to use
exception.

This is damn insecure and far away from "not optimal". As said, checking
the environment
before executing the function isn't safe, and no, exceptions would always
be the better way here.

Regards, Niklas

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

You always assume the developer just wants to fallback to something
different. You can't
detect the environment btw. because it could just fail because of too many
open file descriptors.

This is not my assumption, but others. They argue that they would like to
fallback to alternative method when error happened. (which I think it's not
best
way to do) If they don't, E_RECOVERABLE_ERROR is enough.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Ferenc Kovacs — view source — reply

unread

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 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. :)

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 errors
like mcrypt_encrypt() getting an invalid key/iv
Nikita, Anthony what do you think?

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

10 years ago by Anthony Ferrara — view source — reply

unread

Ferenc,

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 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.
:)

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 errors
like mcrypt_encrypt() getting an invalid key/iv
Nikita, Anthony what do you think?

Strong -1 on this.

The consistency point is something to consider, but there are three major
differences between password_* + mcrypt_* and this case.

First, the majority of mcrypt and password functions are single use.
Meaning they are called once and not inside of loops. Random_int on the
other hand will be used in loops and in direct combination with other
functions.

Second, the target audience is different. Mcrypt targets those that know
something about what they are doing. The api is designed that way. Don't
get me wrong, a lot of users have no idea how to use the APIs properly. But
the api is designed such that you need to know it well to use it
effectively. This is a massive fundamental problem with how crypto code was
implemented in php. Things have been changing lately though. Apis are being
designed with end users in mind. Password_hash was one of these, but others
are on their way as well. In general, the difference is putting the onus
of correct usage on the designer rather than the implementor. Make it
harder to screw up than to do it safe.

Third and lastly, they were built in different times. Had exceptions been
acceptable when I wrote the password functions, believe me I would have
used them. But they weren't just not an option, they were strictly
verboten. Today things are very different with 7. IMHO an exception is the
right way to error here. The problems Scott raises are too big and have too
much potential impact to just leave to the implementors.

Instead, let's follow the trend of not handicapping what's possible. But
instead making it harder to do something wrong than to do it right.

After all, we are not discussing whether an error is appropriate here. We
are discussing whether to fail in a way that forces the user to admit there
was failure, or fail in a way they they can miss (leading to insecurity)...

My $0.02 at least

Anthony

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

10 years ago by Anatol Belski — view source — reply

unread

Hi Anthony,

-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Saturday, August 1, 2015 2:34 AM
To: Ferenc Kovacs tyra3l@gmail.com
Cc: Sammy Kaye Powers me@sammyk.me; internals@lists.php.net; Nikita
Popov nikic@php.net
Subject: Re: [PHP-DEV] Core functions throwing exceptions in PHP7

Ferenc,

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 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.
:)

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
errors
like mcrypt_encrypt() getting an invalid key/iv
Nikita, Anthony what do you think?

Strong -1 on this.

The consistency point is something to consider, but there are three major
differences between password_* + mcrypt_* and this case.

First, the majority of mcrypt and password functions are single use.
Meaning they are called once and not inside of loops. Random_int on the other
hand will be used in loops and in direct combination with other functions.

Second, the target audience is different. Mcrypt targets those that know
something about what they are doing. The api is designed that way. Don't get
me wrong, a lot of users have no idea how to use the APIs properly. But the api
is designed such that you need to know it well to use it effectively. This is a
massive fundamental problem with how crypto code was implemented in php.
Things have been changing lately though. Apis are being designed with end users
in mind. Password_hash was one of these, but others are on their way as well.
In general, the difference is putting the onus of correct usage on the designer
rather than the implementor. Make it harder to screw up than to do it safe.

Third and lastly, they were built in different times. Had exceptions been
acceptable when I wrote the password functions, believe me I would have used
them. But they weren't just not an option, they were strictly verboten. Today
things are very different with 7. IMHO an exception is the right way to error
here. The problems Scott raises are too big and have too much potential impact
to just leave to the implementors.

Instead, let's follow the trend of not handicapping what's possible. But instead
making it harder to do something wrong than to do it right.

After all, we are not discussing whether an error is appropriate here. We are
discussing whether to fail in a way that forces the user to admit there was
failure, or fail in a way they they can miss (leading to insecurity)...

If I'm allowed to notice, what is still not being discussed here is how exceptions are to be thrown in functions. Checking the wiki every day anticipating to find a RFC draft, but hopeless :)

There's still no overall understanding where and how Error vs. Exception are to be meant. There is still no point about how to proceed with the legacy code a way not producing a parallel universe of nice vs ugly code. Still no mention which cases are there, which cases would be acceptable and which wouldn’t. Et cetera. There's only an discussion of one case which doesn't build the whole picture. Having at least this in mind, the procedure of how this case is being thrusted appears to be running deficient, while not the main thrust.

If functions are supposed to be failing - so be. ATM it could be done PHP5 way, might it want to fail softly or rudely. But due to above, IMHO we're not ready to start throwing exceptions in functions today, so in 7.0.

Regards

Anatol

10 years ago by Nikita Popov — view source — reply

unread

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 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. :)

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 errors
like mcrypt_encrypt() getting an invalid key/iv
Nikita, Anthony what do you think?

Hey Ferenc,

I agree with the opinion of Anthony and Scott, in that this should throw
some kind of exception. I don't think it is realistic to expect users to
check the return value of random-number generation functions for "false".
Heck, I probably wouldn't bother with the check myself, given how this
error condition is relatively unlikely (malconfigured system or very high
load). This is exactly the kind of failure for which exceptions are ideal,
because it's not something the programmer can (or at least should) handle
locally -- this is something that needs to get handled at a high level,
likely just with your standard error page.

Regarding how this function differs from other existing crypto functions,
the main difference is of temporal nature. mcrypt_encrypt exists since the
very beginning of PHP 4 and I making the invalid key/IV checks that were
introduced in PHP 5.6 throw would have been a somewhat drastic change for
this function, in my opinion. On the other hand, these CRPRNG functions are
being introduces in PHP 7. Our attitude towards exceptions has changed
significantly in the meantime: Not only were they actually introduced in
the first place (PHP 4 had no exceptions), in PHP 7 a lot of core
functionality has been changed to throw. As of PHP 7 throwing an exception
is the default mode for handling a new engine failure.

Of course, this does not immediately apply to standard library functions.
But as Stas pointed out, given the aforementioned developments, continuing
to deny the use of exceptions in functions categorically is no longer
reasonable. It may be unclear as yet under which circumstances precisely a
function should throw an exception rather than a warning and of what type
that exception ought to be, but it is clear that library functions will
start throwing in the foreseeable future (and for that matter, they have
already started, see for example intdiv).

The only question I see here, and, as Anatol mentions, this is a general
question that needs to be discussed before we start wildly throwing
exceptions, is whether Error or Exception ought to be used. From what I
gather, there exist essentially two interpretations of this matter:

Error is to be used in cases where an error is attributable to
programmer mistake.
Error signifies a failure condition that the programmer is discouraged
(and unlikely to want) to handle. It should only be dealt with at the top
level.

Depending on which interpretation is used, a different type would be
appropriate in this particular case. Under the first interpretation we
should throw an Exception, as this is not a programmer mistake. Instead it
is either a malconfigured system or some sort of IO failure. Under the
second interpretation we should throw an Error, as this is not a condition
that can be reasonable handled.

The first interpretation matches our current
LogicException/RuntimeException split. It would essentially make Error the
new LogicException and Exception the new RuntimeException.

Looking at other error conditions, random_bytes() also errors if the length
is <= 0. This is clearly a programmer mistake and also not something you
should catch, so this should be an Error. random_int() has similar
restrictions on min/max, this is once again a clear Error, using either
interpretation.

Another interesting aspect are the zpp calls. Currently these will throw a
warning and return NULL in weak mode and throw a TypeError in strict mode.
I wonder whether we should be using zpp_throw for new functions, so a
TypeError is also thrown in weak mode. Continuing to use the old
warning+NULL behavior was a BC concern, which we don't have for new
functions. The reason why I think this is worth considering, is that if all
other error conditions of a function already throw, then ordinary zpp will
still add one case where a different type is returned on error. This makes
random_int from a function returning int, to a function returning int|null.

tl;dr: This should definitely throw, but I'm as yet unclear as to what it
should throw. My gut says zpp should throw Error, length/min/max errors
should throw Error and the randomness-not-available condition should throw
Exception.

Nikita

10 years ago by Scott Arciszewski — view source — reply

unread

tl;dr: This should definitely throw, but I'm as yet unclear as to what it
should throw. My gut says zpp should throw Error, length/min/max errors
should throw Error and the randomness-not-available condition should throw
Exception.

Hi Nikita,

This sounds reasonable to me. As I've said before, I don't have any
real preference for which is thrown. If Aaron (author of the Throwable
RFC that passed unanimously) wants to chime in, I'd almost certainly
go with whatever you, him, and Anthony could agree on.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

10 years ago by Trevor Suarez — view source — reply

unread

PHP 7.0 RC1 was just tagged.
Shouldn't this be a relatively high priority to fix/decide so we don't end
up with behavior that can't be fixed until PHP 8.0?

On Sat, Aug 1, 2015 at 6:16 PM Scott Arciszewski scott@paragonie.com
wrote:

tl;dr: This should definitely throw, but I'm as yet unclear as to what
it
should throw. My gut says zpp should throw Error, length/min/max errors
should throw Error and the randomness-not-available condition should
throw
Exception.

Hi Nikita,

This sounds reasonable to me. As I've said before, I don't have any
real preference for which is thrown. If Aaron (author of the Throwable
RFC that passed unanimously) wants to chime in, I'd almost certainly
go with whatever you, him, and Anthony could agree on.

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com

Core functions throwing exceptions in PHP7

Regards,

Regards,

Cheers,

Brian.

Regards,

Given PHP is a weakly-typed language, I don't thing you can rely on this kind of type checking. While in many cases we can tell people "just check for errors or accept the fallout", when we come to security I think the price is too high.

Regards,

Regards,

Regards,

Given PHP is a weakly-typed language, I don't thing you can rely on this
kind of type checking. While in many cases we can tell people "just
check for errors or accept the fallout", when we come to security I
think the price is too high.