New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Improve openssl_random_pseudo_bytes() #3649

Closed
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@SammyK
Copy link
Contributor

SammyK commented Nov 3, 2018

PR for Improve openssl_random_pseudo_bytes() RFC. :)

@petk petk added the RFC label Nov 3, 2018

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Nov 3, 2018

Quite disgusting BC break for minor version (7.4)...

@SammyK

This comment has been minimized.

Copy link
Contributor Author

SammyK commented Nov 4, 2018

@Majkl578 The fail-closed BC should rarely occur in the wild, but is an important implementation detail for CSPRNG's. The second param deprecation is a separate vote in the RFC. :)

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Nov 4, 2018

Well, throwing an exception instead of returning false is a huge BC break in a minor version. This type of stuff is what makes people (and hosting providers) to not upgrade to newer minor versions.

should rarely occur

Can you provide some metrics for this statement?

@KalleZ

This comment has been minimized.

Copy link
Contributor

KalleZ commented Nov 4, 2018

@Majkl578 Those issues should be raised on internals, even more so now that the RFC is in voting

}

if (buffer_length <= 0
#ifndef PHP_WIN32
|| ZEND_LONG_INT_OVFL(buffer_length)
#endif
) {
RETURN_FALSE;
zend_throw_exception(zend_ce_error, "Length must be greater than 0", 0);

This comment has been minimized.

@bukka

bukka Nov 4, 2018

Member

I think this can potentially break stuff. I'm fine with throwing on rand error which happens only if there is something really wrong with the environment but this case can happen more likely. Maybe we should reconsider this case and just return empty string or keep it as false.

This comment has been minimized.

@SammyK

SammyK Nov 11, 2018

Author Contributor

Thanks for the review @bukka! This is to mimic the behavior of random_bytes() like the RFC suggests. This will ensure that openssl_random_pseudo_bytes() can't be used as an attack vector in the event an attacker has access to change $length.

// Let's hope this doesn't exist in the wild! :)
$bytes = openssl_random_pseudo_bytes($_GET['length']);

This comment has been minimized.

@nikic

nikic Nov 20, 2018

Member

I'm also somewhat concerned about this one. Requesting zero bytes from an RNG is not wrong per-se -- it's an operation with a well-defined return value, namely the empty string. The current return value (false) happens to coerce to that.

I feel like there's not much value in throwing an exception for this one specific case (throwing for negative length or overflowing length is fine), and I strongly suspect that this will lead to BC breakage somewhere, particularly in testing code. E.g. I remember that when we merged in ext/sodium and switched from using a sodium-specific random bytes implementation (which allows 0 length) to PHP's that's one of the issues that came up in testing code that was testing random strings of random length (including zero).

That's why I would prefer to continue allowing this particular case. As the RFC does not spell this out either way, I think we could still do so, if we wanted :)

Show resolved Hide resolved ext/openssl/openssl.c Outdated
Improve openssl_random_pseudo_bytes()
CSPRNG implementations should always fail closed. Now
openssl_random_pseudo_bytes() will fail closed by throwing an
`\Exception` in fail conditions.

@SammyK SammyK force-pushed the SammyK:rfc-improve-openssl-random-pseudo-bytes branch from c53ea27 to db44309 Nov 19, 2018

@SammyK

This comment has been minimized.

Copy link
Contributor Author

SammyK commented Nov 19, 2018

Just adjusted this PR to match what was voted in via the RFC. :)

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jan 11, 2019

Merged as 74c0e58 into master :)

@nikic nikic closed this Jan 11, 2019

@SammyK SammyK deleted the SammyK:rfc-improve-openssl-random-pseudo-bytes branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment