# Core functions throwing exceptions in PHP7 - Thread: https://externals.io/message/87172 - Original archive: https://news-web.php.net/php.internals/87172 - Started by Sammy Kaye Powers on 2015-07-14T21:04:55+00:00 - Messages: 57 - Page 1 of 3: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/87172.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #87172 — Sammy Kaye Powers — 2015-07-14T21:04:55+00:00 URL: https://externals.io/message/87172#87172 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 --- ## #87173 — Dean Eigenmann — 2015-07-14T21:10:41+00:00 URL: https://externals.io/message/87172#87173 · In reply to: #87172 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 --- ## #87174 — Scott Arciszewski — 2015-07-14T21:18:38+00:00 URL: https://externals.io/message/87172#87174 · In reply to: #87173 On Tue, Jul 14, 2015 at 5:10 PM, Dean Eigenmann wrote: > Would a PHP Error not work in this case? Or would the error then be interpreted as the result? > > > > […] > > > -- > > > 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 --- ## #87175 — Yasuo Ohgaki — 2015-07-15T08:27:28+00:00 URL: https://externals.io/message/87172#87175 · In reply to: #87172 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 --- ## #87176 — Scott Arciszewski — 2015-07-15T10:19:13+00:00 URL: https://externals.io/message/87172#87176 · In reply to: #87175 > Hi Sammy, > > > > > […] > > 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: 1. `E_WARNING` - fail open, possibly cause security problems for the user 2. `E_ERROR` - fail closed, but make graceful handling a pain in the neck 3. Exception - fail closed by default, developer can write their own graceful failure code if they so choose, would currently be an exception to the rule 1 is bad, 2 is less bad, I think 3 is ideal. Security > continuity. Scott Arciszewski Chief Development Officer Paragon Initiative Enterprises wrote: > > > […] > false if > > […] > hole > > […] > in a > > […] > when > > […] > on > > […] > cryptography. If > > […] > error > > […] > :) > > […] > > -1 I emphatically disagree. > > > […] > > Recommended reading: > http://cr.yp.to/talks/2015.01.07/slides-djb-20150107-a4.pdf > > ------------------- > > Instead of placing all the burden on the user to add code to their > application to check for errors, lest their application fail open and > operate with zero entropy, which is what you've proposed, I say we > throw an exception if the random number generator fails. This means > that, in the default case, their application will fail closed (i.e. > not continue processing with zero entropy), but the developer can > still intuitively handle the exception if they want to add code to > fail gracefully. Try-catch blocks are far less messy than overriding > the error handler. > > Failing open is a terrible idea. You're going to have people who write > code like this: > > $max = strlen($alphabet) - 1; > for ($i = 0; $i < 32; ++$i) { > $password .= $alphabet[random_int(0, $max)]; > } > > How do I know this? Because people already write code like that with > `mt_rand()`. > > If a random number generator failure occurs, instead of generating a > predictable password, we ought to abort. If the developer wants to > handle this situation, they ought to do this. > > try { > $max = strlen($alphabet) - 1; > for ($i = 0; $i < 32; ++$i) { > $password .= $alphabet[random_int(0, $max)]; > } > } catch (Exception $e) { > $this->template('error', 'Could not safely generate a passphrase! > :('); > } > > In the case of a negligent developer, the uncaught exception is > indistinguishable from raising a fatal error. Either way prevents the > script from proceeding with zero entropy. > > TL;DR we have three options: > > 1. `E_WARNING` - fail open, possibly cause security problems for the user > 2. `E_ERROR` - fail closed, but make graceful handling a pain in the neck > 3. Exception - fail closed by default, developer can write their own > graceful failure code if they so choose, would currently be an > exception to the rule > > 1 is bad, 2 is less bad, I think 3 is ideal. Security > continuity. > The basis of my thought is user must write code in a way that would never raise errors under normal conditions and user must stop execution by any errors because they are unexpected. To do that, user should have custom error handler always to achieve graceful exit just like handling uncaught exceptions. i.e. User should have something like following always. 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 --- ## #87177 — Rowan Collins — 2015-07-15T10:57:20+00:00 URL: https://externals.io/message/87172#87177 · In reply to: #87172 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] --- ## #87179 — Yasuo Ohgaki — 2015-07-15T11:20:08+00:00 URL: https://externals.io/message/87172#87179 · In reply to: #87177 Hi Rowan, On Wed, Jul 15, 2015 at 7:57 PM, Rowan Collins wrote: > Sammy Kaye Powers wrote on 14/07/2015 22:04: > > > […] > > 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 --- ## #87180 — Scott Arciszewski — 2015-07-15T11:25:44+00:00 URL: https://externals.io/message/87172#87180 · In reply to: #87178 > Hi Scott, > > On Wed, Jul 15, 2015 at 7:19 PM, Scott Arciszewski > wrote: > > […] > > > 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. > 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: * http://swiftonsecurity.tumblr.com/post/98675308034/a-story-about-jessica * http://www.gaudior.net/alma/johnny.pdf Scott Arciszewski Chief Development Officer Paragon Initiative Enterprises Hi Scott, > > On Wed, Jul 15, 2015 at 8:20 PM, Scott Arciszewski > wrote: > > […] > > > I fully agree with this. > Isn't `E_RECOVERABLE_ERROR` enough for random_*()? > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #87185 — Rowan Collins — 2015-07-15T12:00:00+00:00 URL: https://externals.io/message/87172#87185 · In reply to: #87179 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] --- ## #87191 — Yasuo Ohgaki — 2015-07-15T21:58:19+00:00 URL: https://externals.io/message/87172#87191 · In reply to: #87185 Hi Rowan, On Wed, Jul 15, 2015 at 9:00 PM, Rowan Collins wrote: > If we are going to adopt exception for "functions", it would be better to > > […] > > 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 --- ## #87192 — Pierre Joye — 2015-07-16T00:34:15+00:00 URL: https://externals.io/message/87172#87192 · In reply to: #87191 > 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 --- ## #87203 — Brian Moon — 2015-07-16T16:19:31+00:00 URL: https://externals.io/message/87172#87203 · In reply to: #87172 > 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/ --- ## #87204 — Rowan Collins — 2015-07-16T16:32:15+00:00 URL: https://externals.io/message/87172#87204 · In reply to: #87203 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] --- ## #87250 — Aaron Piotrowski — 2015-07-23T17:26:31+00:00 URL: https://externals.io/message/87172#87250 · In reply to: #87172 > > > 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 --- ## #87274 — Scott Arciszewski — 2015-07-24T16:36:02+00:00 URL: https://externals.io/message/87172#87274 · In reply to: #87250 > > > […] > > 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 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 --- ## #87280 — Stanislav Malyshev — 2015-07-24T23:58:58+00:00 URL: https://externals.io/message/87172#87280 · In reply to: #87203 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 --- ## #87284 — Stanislav Malyshev — 2015-07-25T00:43:36+00:00 URL: https://externals.io/message/87172#87284 · In reply to: #87176 Hi! > 1. `E_WARNING` - fail open, possibly cause security problems for the user > 2. `E_ERROR` - fail closed, but make graceful handling a pain in the neck 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 --- ## #87296 — Larry Garfield — 2015-07-25T23:29:48+00:00 URL: https://externals.io/message/87172#87296 · In reply to: #87280 > Hi! > > > […] > 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 --- ## #87300 — kelerest123@gmail.com — 2015-07-26T18:36:19+00:00 URL: https://externals.io/message/87172#87300 · In reply to: #87296 Hi Larry! 2015-07-26 1:29 GMT+02:00 Larry Garfield : > 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: Rather it is most frequently being done like below: 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 --- ## #87303 — Larry Garfield — 2015-07-26T19:38:25+00:00 URL: https://externals.io/message/87172#87303 · In reply to: #87300 > Hi Larry! > > 2015-07-26 1:29 GMT+02:00 Larry Garfield : > > […] > What "boolean check" are you talkin' about? I've never seen a code > using e.g. `strpos()` like follows: > > > 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: > > > 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 --- ## #87304 — Anatol Belski — 2015-07-26T20:05:02+00:00 URL: https://externals.io/message/87172#87304 · In reply to: #87303 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 > > > > […] > 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 --- ## #87308 — Aaron Piotrowski — 2015-07-27T03:32:25+00:00 URL: https://externals.io/message/87172#87308 · In reply to: #87274 > 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 > > -- > > > 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 --- Next page: https://externals.io/message/87172.md?page=2