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

Add JCryptCipherSodium to support libsodium #16754

Merged
merged 1 commit into from Jul 26, 2017

Conversation

Projects
8 participants
@mbabker
Member

mbabker commented Jun 18, 2017

Pull Request for Issue #13568

Summary of Changes

PHP 7.2 will be removing ext/mcrypt from the core distribution and ext/sodium will be added to core as a new encryption library. As the JCryptCipher classes are primarily built around mcrypt (and inherently what is present in PHP core), we should add support for the new library as well.

ext/sodium isn't restricted to PHP 7.2 installations only. There is also a PECL extension providing support for PHP 5.4 through 7.1 and the sodium_compat polyfill providing support down to PHP 5.2.4. The polyfill is included in this PR to make things usable for all of our supported environments, and the polyfill already has logic in place to support working with either the native PHP API or the PECL extension, so we can just arbitrarily call into the polyfill API without doing extra work on our own.

Testing Instructions

The unit test coverage for the class best demonstrates its use:

$cipher = new JCryptCipherSodium;
$key    = $cipher->generateKey();
$data   = 'My encrypted data.';

$cipher->setNonce(\Sodium\randombytes_buf(\Sodium\CRYPTO_BOX_NONCEBYTES));

$encrypted = $cipher->encrypt($data, $key);
$decrypted = $cipher->decrypt($encrypted, $key);

if ($decrypted !== $data)
{
	throw new RuntimeException('The data was not decrypted correctly.');
}

One thing to note here. Unlike our other ciphers, a nonce must be set and used for both the encryption and decryption of data, and must be the same value on both ends of the process. Otherwise, this is no different than the existing API.

Documentation Changes Required

Document the added class and its use.

//cc @joomla/security

@yvesh

yvesh approved these changes Jun 19, 2017

@SniperSister

This comment has been minimized.

Show comment
Hide comment
@SniperSister

SniperSister Jun 19, 2017

Contributor

As far as I know, sodium_compat still did not had the cryptographic review that the author was strongly asking for, right? If yes, is it wise to build our crypto stuff around it?

Contributor

SniperSister commented Jun 19, 2017

As far as I know, sodium_compat still did not had the cryptographic review that the author was strongly asking for, right? If yes, is it wise to build our crypto stuff around it?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Jun 19, 2017

Contributor

It has not (it's in the readme Michael linked to). I'm also unsure (but not strongly against) on whether we just require min php 7.2 in the is_supported.

We do have to be very careful about documenting that clearly as a minimum in php doc blocks etc

Contributor

wilsonge commented Jun 19, 2017

It has not (it's in the readme Michael linked to). I'm also unsure (but not strongly against) on whether we just require min php 7.2 in the is_supported.

We do have to be very careful about documenting that clearly as a minimum in php doc blocks etc

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jun 19, 2017

Member

I would not lock it to only PHP 7.2 since as noted there is a PECL extension offering support down to PHP 5.4 and is the predecessor for the core API that will be in 7.2. So depending on your server config, the polyfill is the only way to get support for PHP 5.3 and covers the other PHP branches if you can't install the PECL extension. We do have other system components which require PECL extensions that aren't part of the core PHP distro (Memcached and Redis for cache/session support) or depend on PEAR packages (Cache_Lite), so it's not unprecedented for us to have API components with extra dependencies.

Not using the polyfill though complicates this implementation quite a bit. Two main issues come to mind. First, there would be no way to support PHP 5.3 at all (in many ways that's fair, it's a dead branch anyway, but it is part of our supported version range and thus far we don't have features in core that are only available on newer PHP versions). Second, the API between PECL and core PHP 7.2 is different (PECL uses a namespaced API whereas to put the code in core everything had to move to core PHP's naming convention), so we'd have a lot of conditionals in the class for each of the functions we're using.

Member

mbabker commented Jun 19, 2017

I would not lock it to only PHP 7.2 since as noted there is a PECL extension offering support down to PHP 5.4 and is the predecessor for the core API that will be in 7.2. So depending on your server config, the polyfill is the only way to get support for PHP 5.3 and covers the other PHP branches if you can't install the PECL extension. We do have other system components which require PECL extensions that aren't part of the core PHP distro (Memcached and Redis for cache/session support) or depend on PEAR packages (Cache_Lite), so it's not unprecedented for us to have API components with extra dependencies.

Not using the polyfill though complicates this implementation quite a bit. Two main issues come to mind. First, there would be no way to support PHP 5.3 at all (in many ways that's fair, it's a dead branch anyway, but it is part of our supported version range and thus far we don't have features in core that are only available on newer PHP versions). Second, the API between PECL and core PHP 7.2 is different (PECL uses a namespaced API whereas to put the code in core everything had to move to core PHP's naming convention), so we'd have a lot of conditionals in the class for each of the functions we're using.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jul 12, 2017

Contributor

Second, the API between PECL and core PHP 7.2 is different (PECL uses a namespaced API whereas to put the code in core everything had to move to core PHP's naming convention), so we'd have a lot of conditionals in the class for each of the functions we're using.

Just adding sodium_compat actually nullifies that. If you're on < 7.2, it maps sodium_* to \Sodium\*. If you're on 7.2+, it will do the converse. If you want to simplify debugging paths, just use \ParagonIE_Sodium_Compat::* and it will automatically use the C extension where it can, and polyfill the rest.

Contributor

paragonie-scott commented Jul 12, 2017

Second, the API between PECL and core PHP 7.2 is different (PECL uses a namespaced API whereas to put the code in core everything had to move to core PHP's naming convention), so we'd have a lot of conditionals in the class for each of the functions we're using.

Just adding sodium_compat actually nullifies that. If you're on < 7.2, it maps sodium_* to \Sodium\*. If you're on 7.2+, it will do the converse. If you want to simplify debugging paths, just use \ParagonIE_Sodium_Compat::* and it will automatically use the C extension where it can, and polyfill the rest.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jul 24, 2017

Contributor

@SniperSister

If yes, is it wise to build our crypto stuff around it?

First, let's assume there's some vulnerability in sodium_compat. What might it look like?

Let's begin with the attacks that have been eliminated by its design (which is enforced via unit tests and static analysis):

  • It won't be a straightforward cryptanalysis break, since:
    • For shared-key encryption, we're implementing one of the eSTREAM finalists which has been hit hard by cryptographers for years and still has a higher security margin than AES.
    • For shared-key authentication, we're using PHP's built-in HMAC.
    • For public-key cryptography, we're using Curve25519.
    • If any of the above turns out to be false, the problem isn't with sodium_compat, but with all modern cryptography.
  • It won't be a protocol construction flaw, since:
    • For shared-key encryption, we always Encrypt Then Authenticate.
    • For shared-key authentication, we're using hash_equals() to prevent timing side-channels.
    • For public-key encryption (ECDH over Curve25519 then shared-key encryption), we're using a Montgomery ladder construction which only sends the X coordinate. This eliminates invalid curve attacks and timing side-channels.
    • For public-key digital signatures, Ed25519 is a deterministic signature scheme, which prevents a private-key leaking security flaw.
  • It won't be any of the obvious cache-timing side channels in PHP, including chr().
  • It won't be any of the less-obvious (and maybe not exploitable) side-channels, including:

There is still some attack surface that sodium_compat does not, and cannot, address:

  • Zeroing memory buffers is impossible from PHP.
  • Some extensions and features, such as OpCache, might now or in the future introduce premature optimizations that include an accidental side-channel
  • If there is a vulnerability in PHP itself, that causes a side-channel leak, we probably cannot prevent it

That's the crux of what the audit scope was meant to address: Is it possible to implement completely constant-time cryptography in PHP without just writing it in C as an extension?

Even if the answer is "No", in practical terms, you really only need to worry about local attackers (e.g. neighbors in cloud hosting) that can do attacks on the processor's caches (e.g. FLUSH+RELOAD). If you have a high security requirement where this exposure to an unproven hypothetical risk is not acceptable, it's easily mitigated by the following command:

pecl install libsodium

Whether or not it's wise to use sodium_compat is really your decision, but with this knowledge in hand, I hope whatever answer you go with is a lot clearer.

Contributor

paragonie-scott commented Jul 24, 2017

@SniperSister

If yes, is it wise to build our crypto stuff around it?

First, let's assume there's some vulnerability in sodium_compat. What might it look like?

Let's begin with the attacks that have been eliminated by its design (which is enforced via unit tests and static analysis):

  • It won't be a straightforward cryptanalysis break, since:
    • For shared-key encryption, we're implementing one of the eSTREAM finalists which has been hit hard by cryptographers for years and still has a higher security margin than AES.
    • For shared-key authentication, we're using PHP's built-in HMAC.
    • For public-key cryptography, we're using Curve25519.
    • If any of the above turns out to be false, the problem isn't with sodium_compat, but with all modern cryptography.
  • It won't be a protocol construction flaw, since:
    • For shared-key encryption, we always Encrypt Then Authenticate.
    • For shared-key authentication, we're using hash_equals() to prevent timing side-channels.
    • For public-key encryption (ECDH over Curve25519 then shared-key encryption), we're using a Montgomery ladder construction which only sends the X coordinate. This eliminates invalid curve attacks and timing side-channels.
    • For public-key digital signatures, Ed25519 is a deterministic signature scheme, which prevents a private-key leaking security flaw.
  • It won't be any of the obvious cache-timing side channels in PHP, including chr().
  • It won't be any of the less-obvious (and maybe not exploitable) side-channels, including:

There is still some attack surface that sodium_compat does not, and cannot, address:

  • Zeroing memory buffers is impossible from PHP.
  • Some extensions and features, such as OpCache, might now or in the future introduce premature optimizations that include an accidental side-channel
  • If there is a vulnerability in PHP itself, that causes a side-channel leak, we probably cannot prevent it

That's the crux of what the audit scope was meant to address: Is it possible to implement completely constant-time cryptography in PHP without just writing it in C as an extension?

Even if the answer is "No", in practical terms, you really only need to worry about local attackers (e.g. neighbors in cloud hosting) that can do attacks on the processor's caches (e.g. FLUSH+RELOAD). If you have a high security requirement where this exposure to an unproven hypothetical risk is not acceptable, it's easily mitigated by the following command:

pecl install libsodium

Whether or not it's wise to use sodium_compat is really your decision, but with this knowledge in hand, I hope whatever answer you go with is a lot clearer.

@SniperSister

This comment has been minimized.

Show comment
Hide comment
@SniperSister

SniperSister Jul 24, 2017

Contributor

@paragonie-scott thanks a lot for the detailed explanation, that was really helpful!

@mbabker based on the feedback, I'm fine with using the polyfill.

Contributor

SniperSister commented Jul 24, 2017

@paragonie-scott thanks a lot for the detailed explanation, that was really helpful!

@mbabker based on the feedback, I'm fine with using the polyfill.

@zero-24 zero-24 added this to the Joomla 3.8.0 milestone Jul 24, 2017

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Jul 24, 2017

Contributor

I have tested this item successfully on b11ee6f

Works for me with the given test Instructions. Thanks!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16754.

Contributor

zero-24 commented Jul 24, 2017

I have tested this item successfully on b11ee6f

Works for me with the given test Instructions. Thanks!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16754.

@mbabker mbabker changed the base branch from 3.8-dev to staging Jul 25, 2017

@mbabker mbabker removed the PR-3.8-dev label Jul 25, 2017

@rdeutz rdeutz merged commit a2a0d53 into joomla:staging Jul 26, 2017

4 of 5 checks passed

continuous-integration/jenkins/pr-merge This commit is being built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

roland-d added a commit to roland-d/joomla-cms that referenced this pull request Jul 26, 2017

Merge branch 'staging' into redo-7259-modal-for-quicker-edit
* staging: (274 commits)
  Add JCryptCipherSodium to support libsodium (#16754)
  Performance 2 (libraries/legacy) (#12220)
  Performance 6 (templates) (#12233)
  Fixed typehint (#16425)
  Fix for: Repeatable field is no longer rendered with Chosen layout (#16471)
  Fix the path for the ajax-loader.gif (#16701)
  Menu items list parent filter (#17060)
  Text Filters layout (#17113)
  mod_login showon option (#17153)
  com_banners incorret tooltip (#17157)
  fix joomla.content.options_default (#17123)
  remove the never working limitstart call (#17184)
  Update phpDocumentor build
  set 3.8.0 Dev State
  Prepare 3.7.4 Stable Release
  fixed a logic change in #12294, thanks @Hoffi1
  Update sv-SE.ini
  Update pt-BR.ini
  Update lv-LV.ini
  Update fa-IR.ini
  ...

@mbabker mbabker deleted the mbabker:sodium-crypt branch Jul 26, 2017

@mbabker mbabker moved this from Testing/Review to Completed in [3.8] General Jul 27, 2017

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