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

What to do about OpenSSL? #96

Closed
paragonie-scott opened this Issue Mar 16, 2016 · 25 comments

Comments

Projects
None yet
8 participants
@paragonie-scott
Member

paragonie-scott commented Mar 16, 2016

I'm stuck between several possible avenues:

  • Release a new version (v1.3.0 or most likely v2.0.0) that doesn't rely on OpenSSL at all
  • Create an OpenSSL-free fork, called secure_random
  • Possibly in either case allow LibreSSL/BoringSSL but definitely not OpenSSL

I'm interested in everyone's opinions here. The status quo is simply unacceptable.

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Mar 16, 2016

Kill it.

narfbg commented Mar 16, 2016

Kill it.

@ivantcholakov

This comment has been minimized.

Show comment
Hide comment
@ivantcholakov

ivantcholakov Mar 16, 2016

v2.0.0 that doesn't rely on OpenSSL at all seems to be the best choice.

ivantcholakov commented Mar 16, 2016

v2.0.0 that doesn't rely on OpenSSL at all seems to be the best choice.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Mar 16, 2016

👍 For killing it in a 2.0.0 release.

GrahamCampbell commented Mar 16, 2016

👍 For killing it in a 2.0.0 release.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Mar 16, 2016

Though, actually, given the seriousness, I'm inclined to say that actually a 1.3.0 is a better bet to ensure this gets widely deployed, quickly.

GrahamCampbell commented Mar 16, 2016

Though, actually, given the seriousness, I'm inclined to say that actually a 1.3.0 is a better bet to ensure this gets widely deployed, quickly.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 16, 2016

Member

http://semver.org/ dictates a major version increment with an incompatible API change, which removing support for OpenSSL almost certainly is

Member

paragonie-scott commented Mar 16, 2016

http://semver.org/ dictates a major version increment with an incompatible API change, which removing support for OpenSSL almost certainly is

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Mar 16, 2016

This is not an API change.

In fact, the public API (and semver.org does always say "public API") is tied to PHP 7.0.0's, so you almost by definition can't break SemVer compatibility.

narfbg commented Mar 16, 2016

This is not an API change.

In fact, the public API (and semver.org does always say "public API") is tied to PHP 7.0.0's, so you almost by definition can't break SemVer compatibility.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Mar 16, 2016

That's true (ish), which is why my first comment was for 2.0.0, but on reflection, I think 1.3.0 is better, despite semver for the reasons I said already. Most applications are restricting the version to 1.x for this package.

I agree with @narfbg in terms of saying this isn't actually a break. Only the internal API has changed.

GrahamCampbell commented Mar 16, 2016

That's true (ish), which is why my first comment was for 2.0.0, but on reflection, I think 1.3.0 is better, despite semver for the reasons I said already. Most applications are restricting the version to 1.x for this package.

I agree with @narfbg in terms of saying this isn't actually a break. Only the internal API has changed.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 16, 2016

Member

Right. And the area where the issue is most likely going to occur is in WordPress, which already catches the Exception and falls back to what they used previously.

Unless anyone objects I'm going to roll out 1.3.0 without OpenSSL.

Member

paragonie-scott commented Mar 16, 2016

Right. And the area where the issue is most likely going to occur is in WordPress, which already catches the Exception and falls back to what they used previously.

Unless anyone objects I'm going to roll out 1.3.0 without OpenSSL.

@ivantcholakov

This comment has been minimized.

Show comment
Hide comment
@ivantcholakov

ivantcholakov Mar 16, 2016

No objection about 1.3.0 from me.

ivantcholakov commented Mar 16, 2016

No objection about 1.3.0 from me.

@ramsey

This comment has been minimized.

Show comment
Hide comment
@ramsey

ramsey Mar 16, 2016

@paragonie-scott, I'm curious about your thoughts on a better CSPRNG to use in PHP, given your RFC (currently in voting phase) to kill mcrypt and our concerns with OpenSSL. Is there a better library we should eventually move into the PHP core that provides the same (yet more secure) functionality (i.e. LibreSSL, libsodium, etc.)?

ramsey commented Mar 16, 2016

@paragonie-scott, I'm curious about your thoughts on a better CSPRNG to use in PHP, given your RFC (currently in voting phase) to kill mcrypt and our concerns with OpenSSL. Is there a better library we should eventually move into the PHP core that provides the same (yet more secure) functionality (i.e. LibreSSL, libsodium, etc.)?

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 16, 2016

Member

@ramsey Yes, for a CSPRNG, it's called random_bytes() and it's part of the PHP 7 core. The current random_bytes() implementation in 7.0 is almost a verbatim copy of libsodium's randombytes_buf(). They're morally equivalent for the sake of most discussions.

The water is muddy here: mcrypt_create_iv() is the only ext/mcrypt function that's sanely implemented. On the flipside, openssl_random_pseudo_bytes() is the only ext/openssl function that's insanely implemented.

CSPRNG aside, libsodium (which random_compat prefers if it's available) is highly regarded by cryptography/security experts. I have another RFC to add libsodium to PHP in 7.1, pending some changes after 1.0.3 of the PHP extension (which will sync up with 1.0.9 of the library) is finished.

Member

paragonie-scott commented Mar 16, 2016

@ramsey Yes, for a CSPRNG, it's called random_bytes() and it's part of the PHP 7 core. The current random_bytes() implementation in 7.0 is almost a verbatim copy of libsodium's randombytes_buf(). They're morally equivalent for the sake of most discussions.

The water is muddy here: mcrypt_create_iv() is the only ext/mcrypt function that's sanely implemented. On the flipside, openssl_random_pseudo_bytes() is the only ext/openssl function that's insanely implemented.

CSPRNG aside, libsodium (which random_compat prefers if it's available) is highly regarded by cryptography/security experts. I have another RFC to add libsodium to PHP in 7.1, pending some changes after 1.0.3 of the PHP extension (which will sync up with 1.0.9 of the library) is finished.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 16, 2016

Member

To clarify, the post-1.0.3 changes are API related but functionally zero: Instead of \Sodium\function_here() it will be accessed via sodium_function_here(). A congruent change will be made to the constants.

Member

paragonie-scott commented Mar 16, 2016

To clarify, the post-1.0.3 changes are API related but functionally zero: Instead of \Sodium\function_here() it will be accessed via sodium_function_here(). A congruent change will be made to the constants.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Mar 16, 2016

Right. And the area where the issue is most likely going to occur is in WordPress

Can't say I'm that bothered :trollface:

GrahamCampbell commented Mar 16, 2016

Right. And the area where the issue is most likely going to occur is in WordPress

Can't say I'm that bothered :trollface:

@ramsey

This comment has been minimized.

Show comment
Hide comment
@ramsey

ramsey Mar 16, 2016

Yes, for a CSPRNG, it's called random_bytes() and it's part of the PHP 7 core.

Well, obviously! ;-)

ramsey commented Mar 16, 2016

Yes, for a CSPRNG, it's called random_bytes() and it's part of the PHP 7 core.

Well, obviously! ;-)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 17, 2016

Member

1c435ec removes OpenSSL. I'll tag/sign a release on Friday.

Member

paragonie-scott commented Mar 17, 2016

1c435ec removes OpenSSL. I'll tag/sign a release on Friday.

@xabbuh xabbuh referenced this issue Mar 17, 2016

Closed

1.3 release #97

@AshleyPinner

This comment has been minimized.

Show comment
Hide comment
@AshleyPinner

AshleyPinner Mar 17, 2016

Just to weigh in here, while I agree that the other reasons are good ones to remove it, OpenSSL doesn't actually use MD5 for the RAND calls. It's not much better, but by default in 1.0.1 it seems to be using SHA1 (Unless you explicitly build it to use MD5... or less...).

A quick check of the compile options (Use openssl version -a to see them all) should tell you if you're using a version compiled with explicit USE_MD5_RAND or not. CentOS 7 & 6, Ubuntu 14.04 seem not to define it, and I'd love to know if any OpenSSL 1.0.x builds define it.

AshleyPinner commented Mar 17, 2016

Just to weigh in here, while I agree that the other reasons are good ones to remove it, OpenSSL doesn't actually use MD5 for the RAND calls. It's not much better, but by default in 1.0.1 it seems to be using SHA1 (Unless you explicitly build it to use MD5... or less...).

A quick check of the compile options (Use openssl version -a to see them all) should tell you if you're using a version compiled with explicit USE_MD5_RAND or not. CentOS 7 & 6, Ubuntu 14.04 seem not to define it, and I'd love to know if any OpenSSL 1.0.x builds define it.

narfbg referenced this issue Mar 18, 2016

Prepare for 1.3
* Remove OpenSSL from available RNGs, due to overwhelming security concerns

jorissteyn pushed a commit to OpenConext/OpenConext-engineblock that referenced this issue Feb 26, 2018

Joris Steyn
Apply security update to paragonie/random_compat
See: paragonie/random_compat#96

This vulnerability is actually two years old, but has recently been
added to the sensio security checker database. EngineBlock was not
affected because the insecure openssl function is only used when all
other providers (like mcrypt) are not installed.
@kaystrobach

This comment has been minimized.

Show comment
Hide comment
@kaystrobach

kaystrobach Feb 28, 2018

will there be an 1.4.x tag which is not listed on sensio labs (e.g. with openssl disabled?)

kaystrobach commented Feb 28, 2018

will there be an 1.4.x tag which is not listed on sensio labs (e.g. with openssl disabled?)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Feb 28, 2018

Member

That's what v2.x is.

Member

paragonie-scott commented Feb 28, 2018

That's what v2.x is.

kaystrobach added a commit to kaystrobach/flow-development-collection that referenced this issue Mar 1, 2018

@andrerom

This comment has been minimized.

Show comment
Hide comment
@andrerom

andrerom Mar 1, 2018

@paragonie-scott Is it correct to say this is not affecting PHP 5.6.24 and higher?

Asking in regards to the listing on security-advisories

andrerom commented Mar 1, 2018

@paragonie-scott Is it correct to say this is not affecting PHP 5.6.24 and higher?

Asking in regards to the listing on security-advisories

@kaystrobach

This comment has been minimized.

Show comment
Hide comment
@kaystrobach

kaystrobach Mar 1, 2018

@paragonie-scott - thanks just created an pull request in neos/flow 3.3 to switch to v2 ... thanks a lot

kaystrobach commented Mar 1, 2018

@paragonie-scott - thanks just created an pull request in neos/flow 3.3 to switch to v2 ... thanks a lot

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 1, 2018

Member

@andrerom I believe so. However, the decision to trust a userspace PRNG rather than the kernel's CSPRNG was a mistake that we will never repeat.

Member

paragonie-scott commented Mar 1, 2018

@andrerom I believe so. However, the decision to trust a userspace PRNG rather than the kernel's CSPRNG was a mistake that we will never repeat.

@andrerom

This comment has been minimized.

Show comment
Hide comment
@andrerom

andrerom Mar 1, 2018

Clear, I was more thinking a 1.5 release bumping php requirements would allow security advisories to avoid blindly marking whole 1.x branch as insecure, when in fact it depends.

andrerom commented Mar 1, 2018

Clear, I was more thinking a 1.5 release bumping php requirements would allow security advisories to avoid blindly marking whole 1.x branch as insecure, when in fact it depends.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 1, 2018

Member

Honestly, just upgrade to v2.x. Other than the OpenSSL drop, there are no other significant changes.

Member

paragonie-scott commented Mar 1, 2018

Honestly, just upgrade to v2.x. Other than the OpenSSL drop, there are no other significant changes.

@kaystrobach

This comment has been minimized.

Show comment
Hide comment
@kaystrobach

kaystrobach Mar 1, 2018

I just did it for 2 projects, works like a charm - thanks a lot.

kaystrobach commented Mar 1, 2018

I just did it for 2 projects, works like a charm - thanks a lot.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Mar 1, 2018

Member

Awesome! If anyone else stumbles over this, I've just updated the readme to use headers instead of bold text so you can link folks directly to the relevant section: https://github.com/paragonie/random_compat/blob/master/README.md#version-conflict-with-other-php-project

Member

paragonie-scott commented Mar 1, 2018

Awesome! If anyone else stumbles over this, I've just updated the readme to use headers instead of bold text so you can link folks directly to the relevant section: https://github.com/paragonie/random_compat/blob/master/README.md#version-conflict-with-other-php-project

neos-bot pushed a commit to neos/flow that referenced this issue Mar 1, 2018

jorissteyn pushed a commit to OpenConext/OpenConext-engineblock that referenced this issue May 24, 2018

Joris Steyn
Apply security update to paragonie/random_compat
See: paragonie/random_compat#96

This vulnerability is actually two years old, but has recently been
added to the sensio security checker database. EngineBlock was not
affected because the insecure openssl function is only used when all
other providers (like mcrypt) are not installed.

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