Change CSPRNG warnings to exceptions #1397

Closed
wants to merge 8 commits into
from

Projects

None yet
@SammyK
Contributor
SammyK commented Jul 7, 2015

Inspired by this issue and this comment, this PR changes the behavior of the new CSPRNG's in PHP7; random_bytes() and random_int().

If a good source of random cannot be found an Exception will now be thrown instead of raising an E_WANRING.

This will allow the user to do something like this:

try {
    $bytes = random_bytes(42);
} catch (Exception $e) {
    $cryptoStrong = false;
    $bytes = openssl_random_pseudo_bytes(42, $cryptoStrong);
    // Or whatever
}

There is also discussion of tweaking the error conditions a bit which I'll submit as a separate PR.

CC: @weltling, @KalleZ

@SammyK
Contributor
SammyK commented Jul 13, 2015

Closing to re-trigger Travis CI when I reopen.

@SammyK SammyK closed this Jul 13, 2015
@SammyK
Contributor
SammyK commented Jul 13, 2015

Come on Travis CI!

@SammyK SammyK reopened this Jul 13, 2015
@paragonie-scott
Contributor

👍 tests pass, exceptions > errors

@weltling
Contributor

@SammyK we was discussing both of your PRs with Kalle. it's probably not something for 7.0. The main reason is - throwing expeptions is not defined yet regarding functions.

  • We had a RFC about exceptions in the engine, which was grown from a smaller RFC about catching fatal calling method on a non object to the Throwable RFC. That went in quite late in 7.0 alpha. But that also narrows to the engine area, not to internal functions.
  • Aaron posted an RFC which looks like should be targeting 7.1 as well #1388 exactly for the same reason
  • even if ignoring the above, an exception would make the behavior alternating from the other rand functions, fe mt_rand(). But also it would be different from the other core functions.

At the end - it would be probably more sensible to introduce a RFC like "exceptions in core functions". Currently internal functions don't throw exceptions, and it is the global API behavior. Probably many people would find your suggestion if spread over the whole code base useful. But IMHO, even then - converting fatals to Error is something else than touching warnings.

Thanks.

@KalleZ
Contributor
KalleZ commented Jul 13, 2015

CCing @trowski for this as it might be of interest

@nikic
Contributor
nikic commented Jul 13, 2015

@weltling I think these functions deserve special consideration, because they are security-critical. If a CSPRNG source cannot be acquired and you continue ordinary execution despite that, you'll effectively end up using either 0 or the empty string as your random data, which will likely completely undermine your security. We cannot reasonably expect users to do if (false === $rand = random_int($a, $b)) { ... } checks when they generate random numbers. The minimum guarantee we should provide is that if you provide correct input parameters, you'll get either CS-quality output or a hard failure (i.e. something not continuing on the same execution path). This guarantee is what this PR provides.

I agree that use of exceptions in the standard library in general is a topic that is still up to discussion and this is just a special case due to security concerns.

@weltling
Contributor

@nikic yeah, however converting warning to Excepiton seems like a very long jump. Having an exception would probably have the same effect as a warning when an exception handler is in use. At the end these are two sides of user WTF - at least, like why rand() and mt_rand() throw warning while random_int() throws exception.

IMHO it fully makes sense to consider special handling on case by case base, as you say, and that should be probably one of the key points of the RFC. However then - there were much more places like in ext/openssl, ext/hash or where ever to handle. It doesn't solve the issue saying we're secure in two functions while a huge amount is still how it was. And that was also not the principle in PHP5 as we don't see much fatal errors where they really should be. And imagining this functions were added in PHP5, for sure there were no fatal errors used. That's why IMHO the behavior consistency has preference here, and a consolidated solution should be still worked out. Bringing some changes into user land, we don't really can know what comes and whether we'll have to do a BC break afterwards because a solution was based on today's assumptions.

Thanks.

@nikic
Contributor
nikic commented Jul 13, 2015

yeah, however converting warning to Excepiton seems like a very long jump.

Yes, converting a warning into an Exception is problematic due to BC concerns. However these are newly introduced functions, so we're not really converting anything here. For that matter, changing this at a larger point in time will likely be impossible due to BC concerns, unless you have PHP 8 in mind.

At the end these are two sides of user WTF - at least, like why rand() and mt_rand() throw warning while random_int() throws exception.

These error conditions do not existing for rand() or mt_rand(). I have not a single time seen anyone write false === $rand = mt_rand() conditions, which would be required to safely use random_int() if this change is not implemented.

IMHO it fully makes sense to consider special handling on case by case base, as you say, and that should be probably one of the key points of the RFC. However then - there were much more places like in ext/openssl, ext/hash or where ever to handle. It doesn't solve the issue saying we're secure in two functions while a huge amount is still how it was.

Conversely, the fact that we have made bad choices (or were limited at the time the functions were introduced, PHP 4 didn't even have exceptions) for existing functions, doesn't mean that we have to continue doing so when introducing new ones.

@trowski
Contributor
trowski commented Jul 13, 2015

Since these functions are used for security purposes, I think it is imperative that an exception is thrown if a number with sufficient entropy cannot be produced. These functions are meant to be used for generating security dependent random numbers, whereas rand() and mt_rand() use a low entropy source and are not meant to be used for security purposes.

At the end these are two sides of user WTF - at least, like why rand() and mt_rand() throw warning while random_int() throws exception.

Since these functions are used for different purposes, I would not necessarily expect them to behave similarly upon failure. random_int() or random_bytes() failing could compromise the security of an application. Execution should not be allowed to continue with a result that could be interpreted as 0 or an empty string when application security may depend on that value.

There are no BC concerns here as @nikic pointed out, these are new functions in PHP 7. I agree the the behavior is somewhat apart from some other internal functions. However, I think this is a good place to start switching internal functions to throwing exceptions for critical errors.

@weltling
Contributor

Guys, the fact that bad choices was made in the past doesn't bear they should be done today. Today's bad choice would be - introducing a major behavior change without a good discussion on all the sides. These are new functions, but it is an old big code base. So it fact we would be introducing this behavior change to the whole.

The behavior in mt_rand imply also another Sammy's ticket, see https://github.com/php/php-src/blob/master/ext/standard/rand.c#L321 .

And yeah - it can create BC breaks, the future ones. Because strategically it's undefined today what we do with exceptions in the functions, if we start doing it randomly - it'll end up with an undefined result as well. Say, what if after research you decide it has to throw a SecurityError instead of Exception? Just as example, but it is the same concern I was expressing right at the start - any change to user land should not be done without a good strategy. IMHO even 6 guys on a bug ticket page is not what required to make decisions about this.

Thanks.

@bwoebi
Contributor
bwoebi commented Jul 14, 2015

I'm not sure if that's already acting unresponsibly in this case. If you need to use security critical functions, you really use them because you expect them to give the security you need.
If you're returning false, it definitely doesn't.

Seriously, I'd even prefer a fatal error over a warning for this. I don't really care whether we're throwing an Exception or fataling out here, but it should definitely bail out there. I'd just prefer an Exception here, because we removed the fatals where possible.

Also, this will always be some type of Exception (not Error as it's related to some sort of I/O), and using a subclass of an Exception is not a BC break.

@weltling
Contributor

Bob, but since when Error imply to be non IO only? You see, it is something we're talking about without having same base? It's actually exactly what I mean - it is a controversial topic and there's no good strategy behind it. It is something that must be hardly discussed to clear out such subtle issues. We shouldn't urge a major behavior change of this scale after the feature freeze. However with a good worked out strategy there is far less risk.

With the irresponsibility - one can say same about ext/openssl or many other places. Where it's just RETURN_FALSE instead of a real bailout, or even just silently pass through without any kind of warning. The fact the older functionality behaves same way were less concerning then? Also, the CSPRNG functions are already there for some time, people already play with them (and implement something, as we see).

This is the current situation. One even doesn't need to argue that many behaviors can be improved. At least they're broken in consistent ways, so should be fixed in consistent ways. The current project rule is to do as less bailouts as possible, don't remember it was ever changing. I would suggest to follow that, once we have a strategy to handle these cases, it is safe for the future to go on and continue on Aaron's PR.

Thanks.

@bwoebi
Contributor
bwoebi commented Jul 14, 2015

For the first point… what are Errors for, what are Exceptions for? I understood the RFC as if Errors were for programming errors, exceptions for anything which is influenced by the environment. (I meant I/O in the larger sense including syscalls etc.)
If we don't agree, then I'm now seriously wondering what else it means.

I wouldn't say it's broken in consistent ways. At least functions now added with PHP 7 properly respect the exceptions. See for example intdiv().

I can't speak for the old functions (like you say in ext/openssl) … I agree that there's room for improvement. BUT the new functions in PHP 7 definitely should be doing it right from the beginning on. Or do we need needless forward compatibility breaks? That's absurd.

And I very much think that changed with PHP 7. It's just that the most exception related changes now came though very late in PHP 7 and we didn't really want to change anything before deciding on Throwable.
I don't say we should bail out for every little thing, but we definitely shall for security related concerns. I don't argue for adding that to the old functions [at least not now. For 7.1, sure.], but the new ones should do it right from beginning on, as said.

@weltling
Contributor

Actually with the Error i was understanding exactly the opposite :) Also just reread the Throwable and Exceptions in the engine RFCs - the code examples, the texts, also fe converting E_ERROR is more about Error. With Exception it's now more complicated because they can represent any kind of issue as it was only the throw possibility in the past, and it's something to be very carefully to cleanup.

So it's not broken in consistent ways. It is sad. However intdiv regards to an even more contradictory topic than this one. Because intdiv is related to the operators handling in ZE, it's fresh as it was happening before short time. It is a very good example of what happens when there is no clear concept. Now, i wouldn't see any reason to continue this practice.

While talking about that "new functions should do the right behavior from the start on" - there's an argument that i have to repeat. We have no definition of the right behavior, we have no concept of the right behavior, and no strategy. The more cases like intdiv land "right at the start", the more risk we won't ever have a chance to define the right behavior at all.

Thanks.

@bwoebi
Contributor
bwoebi commented Jul 14, 2015

If that's your concern, feel free to officially launch a discussion on internals about that. Then we know what we'll head to and can adequately adapt the behavior of the new functions here. I thought it was rather clear to all of us what Exception and Error meant, but if it's not, go ahead to internals and discuss it.

I feel like here are five people all of one same opinion; so, let's better decide what's right sooner than later.

Let's not continue that discussion in that PR now as long as we're talking about such fundamental issues.

EDIT: And generally, I'd preferred having just Exception at the base, which would have made these choices between Error and Exception less problematic… But everyone had raised a very big red BC break shield then. Here we are now… [No, I'm not proposing to change it again; just saying.]

@weltling
Contributor

Bob, it's no me pushing these changes. And, the concerns about RFC was expressed earlier in the Aaron's PR about fatals->errors in the functions where you was in favor of it. So who is supposed to raise a discussion and work out the concept? :) What we as RM can do is to hint about possible issue, but it is obviously impossible to implement everyone's ideas. Imagine this concept were before you went for the ArithmeticError fix - it would have saved your and everyone's else time and mental power.

And I said right at the start that it's not to decide by a couple of a people in the room on stack overflow or on a github PR page, what you just say a very dangerous thing.

But where you right is that 5 people advocating a strong opinion can move giga stones. Just it needs to be handled appropriately.

Thanks.

@paragonie-scott
Contributor

If you're relying on these new functions for cryptographically secure pseudorandom data, and it fails to provide reliable randomness, your application should halt. The best way to do this is to throw an exception. Expecting your run-of-the-mill w3schools-educated PHP developer to know to check the return value isn't realistic. Forcing them to write code to handle the exception (or the script terminates and doesn't fail open) if they want to proceed puts less end-users at risk.

On the flipside, raising a fatal error is much more difficult for a PHP-land developer to mitigate and add fallback code.

try {
    $data = random_bytes(64);
} catch (Exception $e) {
    return $this->view('error', [
        'message' => 'Sorry, we are experiencing technical difficulties and cannot safely process your request.'
    ]);
}

Versus messing around with set_error_handler(), which is something that I do not find intuitive enough to provide an example of off the top of my head because it will be wrong and stupid.

@kelunik
Contributor
kelunik commented Jul 14, 2015

Having security related functions introduced to make generating secure random ints / strings easier, which then return false on failure and proceed with normal control flow by default, is ridiculous. Security related functions should be safe by default, which can be achieved here by throwing an Exception. For BC reasons, I'd go with an exception here, because it's I/O related and can safely be changed into a sub-type in 7.1.

@SammyK
Contributor
SammyK commented Jul 14, 2015

We should totes discuss this on the PHP Roundtable in a few hours. :)

@hakre
Contributor
hakre commented Jul 14, 2015

Silly question: How can I probe (find out) that if I would need X random bytes or X random integers when an Exception is thrown? Does it require me to open a side-channel (catching exceptions) to return true/false for the probe?

If such a function is used for secure programming, the person who writes the code should take the path that random was not sufficient into consideration from the ground on. So the check if it worked or not is mandatory in any case. At least with defensive programming it most certainly is.

How could that be used with defensive programming that is checking the precondition enough random is available w/o having to catch an exception?

@paragonie-scott
Contributor

If such a function is used for secure programming, the person who writes the code should take the path that random was not sufficient into consideration from the ground on.

See also Error-prone cryptographic designs (PDF).

Do you think we should put the burden on the users to perform steps if they don't want it to fail open? Would such a design lead to more or less security failures?

@ktomk
ktomk commented Jul 14, 2015

Do you think we should put the burden on the users to perform steps if they don't want it to fail open?

No, Exception is a good safety guard against unprotected use. Can be very useful to produce (more) portable PHP code. Otherwise an oversight can be really hurting.

Would such a design lead to more or less security failures?

In general? - I'd say it's more secure to create an Exception as the case is really exceptional (as I'm learning). Otherwise a user of that function must use much more code only for very specific cases. Like the one described. So in general, I'd say Exception is fine.

@paragonie-scott
Contributor

No, Exception is a good safety guard against unprotected use.

Agreed.

So in general, I'd say Exception is fine.

👍

@ktomk
ktomk commented Jul 14, 2015

@paragonie-scott

See also Error-prone cryptographic designs (PDF).

Would you see this here qualify as an implementation or as a protocol?

@paragonie-scott
Contributor

For the sake of analogy, it fits the role of a protocol.

We're providing a design that we expect other people to use in their code. If we give them more responsibilities than necessary, they will make avoidable mistakes.

@SammyK
Contributor
SammyK commented Jul 21, 2015

FYI: These changes have also been discussed on the internals mailing list.

@paragonie-scott
Contributor

The discussion, by the way, seems to have stalled.

@SammyK
Contributor
SammyK commented Aug 19, 2015

Hey guys! Just a heads up: the HHVM port of this will be throwing exceptions instead of warnings to favor security over compatibility. Don't know if that makes a difference on whether or not this get merged. But wanted to give this one last push before a stable release. :)

Ping: @weltling @KalleZ

@weltling
Contributor

Hy @SammyK,

thanks for the ping. The suggestion working out a strategy didn't make through as is to see. And now we're in the situation of the feature freeze.

IMHO PHP needs such a strategy to produce stable behaviors regarding exceptions (in the functions in general, but not only). Unfortunately no one of the supporters went for an RFC.

It is good for HHVM that such things can admittedly be accepted, that means HHVM obviously has a plan about it (even if it is to "deliberately differ from PHP"). Please note that PHP-7.0 will be branched in less than a month, so then master were unlocked to accept the pending RFCs. And there are some PR at least from Aaron regarding the topic. And there was some work moving E_ERROR to E_RECOVERABLE_ERRER (which looks like a clear preparation for exteptions) in the ext/session by Yasuo. And what Anthony was mentioning about ext/hash and "he would have done it with exteptions", etc.

I am not sure what is the reason for not to go for RFC. If it is a courage/experience lack and one would need support, sure one would find it. It is to see that there are quite some patterns that fit into presumable scheme, but the scheme is to be defined clearly. If someone (or more) is still up to go for this, i can take some time to support whatever is needed for paper/implementation work - it is eventually a fun.

Best regards

Anatol

@SammyK
Contributor
SammyK commented Aug 21, 2015

Thanks @weltling! For now I'll just update the docs with some details on how to handle the rare edge-case error. And then I'll try to get caught up on the discussions for an exception RFC. :)

@weltling
Contributor

@SammyK you are a cool man, thank you!

@SammyK
Contributor
SammyK commented Aug 24, 2015

@weltling Aw shucks, thanks. :)

@SammyK
Contributor
SammyK commented Sep 2, 2015

Updated the implementation to match the RFC: Random Functions Throwing Exceptions in PHP 7. :)

@paragonie-scott
Contributor

Awesome. The RFC passed 28-2. :)

@nikic nikic and 3 others commented on an outdated diff Sep 7, 2015
ext/standard/random.c
@@ -158,19 +159,19 @@ PHP_FUNCTION(random_int)
zend_ulong umax;
zend_ulong result;
- if (zend_parse_parameters(ZEND_NUM_ARGS(), "ll", &min, &max) == FAILURE) {
+ if (zend_parse_parameters_throw(ZEND_NUM_ARGS(), "ll", &min, &max) == FAILURE) {
return;
}
if (min >= max) {
@nikic
nikic Sep 7, 2015 Contributor

The RFC specs this as min > max. @ircmaxell diff intentional?

@ircmaxell
ircmaxell Sep 7, 2015 Contributor

That was an oversight in the RFC, it should be >=.

@bwoebi
bwoebi Sep 7, 2015 Contributor

@ircmaxell Hmm, I sometimes have cases like where I get an unsigned integer and then want a random number between 0 and that integer… surely that integer can be zero too... Yes, it's not a random value then, but actually the most trivial case. No (?) reason to exclude it, I think.

Would be a bit dumb to have to function better_random_int($a, $b) { return $a === (int) $b ? random_int() : $a; } just for that...

After all, the point is to pick a number on a range. [0, 0] is a valid range. Just like [0, 1] is and [0, -1] isn't.

@SammyK
SammyK Sep 7, 2015 Contributor

@bwoebi This PR adjusted the logic to accept random_int(42, 42).

@ircmaxell Since the RFC passed but with the min > max spec, does this not get adjusted?

@SammyK
Contributor
SammyK commented Sep 8, 2015

Updated to allow min & max to be samezies per this tweet.

@php-pulls php-pulls pushed a commit that referenced this pull request Sep 8, 2015
@bwoebi bwoebi Merged RFC Random Functions Throwing Exceptions in PHP 7
Squashes commits from PR #1397

commit cd5dcc8c9eb43603d908abcea69c9e18df0f2ed5
Author: SammyK <sammyk@sammykmedia.com>
Date:   Tue Sep 8 13:53:42 2015 -0500

    Add min max samezies

commit b719499218a4e84efecd4dc1d4235d16142c9793
Author: SammyK <sammyk@sammykmedia.com>
Date:   Wed Sep 2 07:00:25 2015 -0500

    Make random_bytes() throw Error when $length <= 0 and random_int() throw Error when $min > $max

commit 0cca557291c278716ec4b00b32fc2bdc1c1c8848
Author: SammyK <sammyk@sammykmedia.com>
Date:   Wed Sep 2 06:55:59 2015 -0500

    Make random_*() functions throw Error exception when random bytes cannot be obtained

commit 998c7f1e209123605b41139e8d9093075ce16bd6
Author: SammyK <sammyk@sammykmedia.com>
Date:   Wed Sep 2 06:41:20 2015 -0500

    Make random_*() functions throw TypeError when zend_parse_parameters fails

commit 99d305c18820ff55d82d952777cbcdf1cf0158be
Author: SammyK <sammyk@sammykmedia.com>
Date:   Mon Jul 6 19:50:47 2015 -0500

    Make exceptions less specific

commit b042dfab290713366741a663a420cf12bf802f39
Author: SammyK <sammyk@sammykmedia.com>
Date:   Mon Jul 6 17:20:13 2015 -0500

    Upgrade warnings to RuntimeExceptions
c12917a
@php-pulls

Comment on behalf of bwoebi at php.net:

Squashed (as Anthony requested) via c12917a

@php-pulls php-pulls closed this Sep 8, 2015
@bwoebi
Contributor
bwoebi commented Sep 8, 2015

Hmm, shouldn't have rebased before squashing… now all the commit ids are meaningless...

@SammyK
Contributor
SammyK commented Sep 8, 2015

One of these days I'm going to get a clean merge into php-src. Once I finally learn how to use git. And code properly. Thanks @bwoebi!

@SammyK SammyK deleted the SammyK:csprng-exception branch Sep 9, 2015
@hhvm-bot hhvm-bot added a commit to facebook/hhvm that referenced this pull request Dec 3, 2015
@SammyK @hhvm-bot SammyK + hhvm-bot Port PHP7 CSPRNG functions random_int() and random_bytes()
Summary:
Hello! This PR is a port of the [CSPRNG functions](http://php.net/csprng) `random_*()` which were [added in PHP7](https://wiki.php.net/rfc/easy_userland_csprng).

This PR is very much a work in progress and I'll need a lot of help from you smarter kids to get this implemented properly. But it works well on my machine thanks to some one-on-one help from jmikola.

The things that I know need to be addressed:

1. The PHP version [checks for sources of random at compile time](https://github.com/php/php-src/blob/97f159d70288ae94f9a05a370121bcbea760ed9a/Zend/Zend.m4#L406-L412). Not sure how to do this in HHVM.
2. PHP [supports Windows with `arc4random_buf()`](https://github.com/php/php-src/blob/cbcacbb2dad4c97ba5653f42729f7956193d9112/ext/standard/random.c#L86). Not sure if we want to support this as well as HHVM gets more Windows friendly... is that a thing?
3. There is [an open PR](php/php-src#1397) to change the behavior of handling the cases when a reliable source of random cann

Closes #5498
Closes #5925

Reviewed By: sgolemon

Differential Revision: D2342229

Pulled By: jwatzman

fb-gh-sync-id: 419d949e3dd06fb1c13ecf86223c1b28eb57bb84
df3688e
@jwatzman jwatzman added a commit to facebook/hhvm that referenced this pull request Dec 4, 2015
@SammyK @jwatzman SammyK + jwatzman Port PHP7 CSPRNG functions random_int() and random_bytes()
Summary:
Hello! This PR is a port of the [CSPRNG functions](http://php.net/csprng) `random_*()` which were [added in PHP7](https://wiki.php.net/rfc/easy_userland_csprng).

This PR is very much a work in progress and I'll need a lot of help from you smarter kids to get this implemented properly. But it works well on my machine thanks to some one-on-one help from jmikola.

The things that I know need to be addressed:

1. The PHP version [checks for sources of random at compile time](https://github.com/php/php-src/blob/97f159d70288ae94f9a05a370121bcbea760ed9a/Zend/Zend.m4#L406-L412). Not sure how to do this in HHVM.
2. PHP [supports Windows with `arc4random_buf()`](https://github.com/php/php-src/blob/cbcacbb2dad4c97ba5653f42729f7956193d9112/ext/standard/random.c#L86). Not sure if we want to support this as well as HHVM gets more Windows friendly... is that a thing?
3. There is [an open PR](php/php-src#1397) to change the behavior of handling the cases when a reliable source of random cann

Closes #5498
Closes #5925

Reviewed By: sgolemon

Differential Revision: D2342229

Pulled By: jwatzman

fb-gh-sync-id: 419d949e3dd06fb1c13ecf86223c1b28eb57bb84
c436f81
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment