User tests: Successful: 0 Unsuccessful: 0
Pull Request for Issue #13568
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.
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.
Document the added class and its use.
//cc @joomla/security
Status | New | ⇒ | Pending |
Category | ⇒ | Repository External Library Libraries |
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
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.
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.
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):
hash_equals()
to prevent timing side-channels.chr()
.There is still some attack surface that sodium_compat does not, and cannot, address:
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.
@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.
I have tested this item
Works for me with the given test Instructions. Thanks!
Category | Repository External Library Libraries | ⇒ | Repository External Library Composer Change Libraries |
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-26 05:15:38 |
Closed_By | ⇒ | rdeutz | |
Labels |
Added:
?
Removed: ? |
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?