On Sat, Aug 1, 2015 at 12:34 AM, Ferenc Kovacs <tyr...@gmail.com> wrote:
> > > > On Tue, Jul 14, 2015 at 11:04 PM, Sammy Kaye Powers <m...@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? > 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: 1) Error is to be used in cases where an error is attributable to programmer mistake. 2) 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
