Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Removed the OpenSSL usage #16

Closed
wants to merge 3 commits into from
Closed

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Mar 23, 2016

This PR removes the usage of the openssl_random_pseudo_bytes() function of OpenSSL due to the bug #70014 and the discussion reported here.

functions [random_bytes](http://php.net/manual/en/function.random-bytes.php) and
[random_int](http://php.net/manual/en/function.random-int.php), otherwise we use the
[Mcrypt](http://it.php.net/manual/en/book.mcrypt.php) extension or /dev/urandom source with a mixer
function provided by . If you don't have a secure random source in your environment the component
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"provided by "... what?

@ezimuel
Copy link
Contributor Author

ezimuel commented Mar 23, 2016

@Ocramius I used function_exists() as suggested, thanks!

@ezimuel
Copy link
Contributor Author

ezimuel commented Mar 25, 2016

@weierophinney I think the PR is ready to be merged.

@@ -56,7 +54,7 @@ public static function getBytes($length, $strong = false)
if (true === $strong && false === $checkAlternatives) {
throw new Exception\RuntimeException(
'This PHP environment doesn\'t support secure random number generation. ' .
'Please consider installing the OpenSSL and/or Mcrypt extensions'
'Please consider installing Mcrypt extension or use PHP 7'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase this to "Please consider either installing ext/mcrypt or upgrading to PHP 7".

@weierophinney
Copy link
Member

@ezimuel Just a few notes about exceptions to address, and then I can merge. I'm assuming this would be for a new minor release?

@weierophinney weierophinney self-assigned this Apr 7, 2016
@weierophinney weierophinney added this to the 2.7.0 milestone Apr 7, 2016
weierophinney added a commit that referenced this pull request Apr 7, 2016
weierophinney added a commit that referenced this pull request Apr 7, 2016
- What you did
weierophinney added a commit that referenced this pull request Apr 7, 2016
@weierophinney
Copy link
Member

Merged to develop for release with 2.7.0.

@j-schumann
Copy link

Isn't this considered backwards incompatible because developer action is required when you happen to have neither mcrypt nor the RandomLib installed?

After updating to 2.7.0 my application now throws the "The RandomLib fallback pseudorandom generator is not installed" exception when using Zend\Validator\Csrf (which calls Zend\Math\Rand::getBytes).

Shouldn't have the openSSL usage been marked as deprecated before in 2.6.0 so developers could prepare? I understand that there are security implications but are they important enough to force the BC break?

@Ocramius
Copy link
Member

Ocramius commented Apr 8, 2016

If this is considered a security issue, it takes priority over BC compat. Depends on @ezimuel's decision tho.

@gianarb
Copy link

gianarb commented Apr 8, 2016

Depends on @ezimuel's decision tho.

Is it a security issue @ezimuel ? :) Thanks

@j-schumann
Copy link

I see, thanks for clearing that up!
One could argue that, since #70014 is patched for all PHP versions from 5.4.44 on, the security risk is not sincere.
I just would opt that this could have been fixed without a BC break, e.g. if Zend\Math\Rand would implement the functionality of RandomLib\Source\URandom and RandomLib\Source\CAPICOM itself as it already checks for the availability of this sources already. A new major version (or after deprecation) could then remove the functionality and require RandomLib or ext/mcrypt.

@ezimuel
Copy link
Contributor Author

ezimuel commented Apr 8, 2016

@j-schumann yes, this is considered as a potential security issue for some specific OpenSSL installation, see #70014.

@gianarb
Copy link

gianarb commented Apr 8, 2016

Thanks! :)

@ezimuel
Copy link
Contributor Author

ezimuel commented Apr 8, 2016

@j-schumann there are also other issues around the usage of OpenSSL, see comments reported here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants