sodium_memzero()/sodium_increment() polyfills behave incorrectly when libsodium-php 1.x is available

[LeSuisse]

sodium_memzero() and sodium_increment() both work with the reference of their parameter, when using libsodium-php 1.x the parameter is not passed as reference. This is due to the usage of call_user_func() to call the "real" functions of php-libsodium from the polyfill.

For sodium_memzero() that means the content of the string is not wiped and no error, warning or exception is thrown.

<?php
$str = 'ABCDEFGH';
sodium_memzero($str);
var_dump($str === 'ABCDEFGH'); // true, should be false or SodiumException should have been thrown by sodium_memzero()

For sodium_increment() that means the value is not incremented.

<?php
$str = random_bytes(SODIUM_CRYPTO_SECRETBOX_NONCEBYTES);
var_dump(sodium_bin2hex($str)); // f4a85361c2e563252ea077abc9e0b9ce38fb201808444c1b
sodium_increment($str);
var_dump(sodium_bin2hex($str)); // f4a85361c2e563252ea077abc9e0b9ce38fb201808444c1b should be f5a85361c2e563252ea077abc9e0b9ce38fb201808444c1b

I initially thought that sodium_increment() was generating a fatal error instead of silently doing nothing but my initial test case was too simple and was hitting this error of libsodium: https://github.com/jedisct1/libsodium-php/blob/1.0.7/libsodium.c#L480

Both cases have been reproduced with the following environment:
Linux
PHP 5.6.37
libsodium-php 1.0.7
sodium_compat 1.6.3

[paragonie-scott]

Thanks for the excellent report and the fix. I'm going to be tagging/releasing a new version tonight. 👍

[LeSuisse]

Hi,

Thanks for your work!

I still think there is an issue with sodium_increment(). I mentioned it in #74, the same trick does not seem to work, the string is not incremented. It works as expected if I call \Sodium\increment() directly or if I force sodium_compat to use the pure PHP polyfill.

I did not took the time to do further investigations yet, either I miss something related to my environment or there is something subtle at play here.

[LeSuisse]

Hi,

So it seems there is really an issue not related to my environnement, the following crappy Dockerfile can be used to reproduce:

FROM php:5.6.37

RUN apt-get -y update \
    && apt-get -y install libsodium-dev \
    && pecl install libsodium-1.0.7 \
    && docker-php-ext-enable libsodium

WORKDIR /root/sodium_compat

RUN apt-get -y install unzip \
    && php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" \
    && php -r "if (hash_file('SHA384', 'composer-setup.php') === '544e09ee996cdf60ece3804abc52599c22b1f40f4323403c44d44fdfdd586475ca9813a858088ffbc1f233e9b180f061') { echo 'Installer verified'; } else { echo 'Installer corrupt'; unlink('composer-setup.php'); } echo PHP_EOL;" \
    && php composer-setup.php \
    && php -r "unlink('composer-setup.php');" \
    && php composer.phar require paragonie/sodium_compat:1.6.4 \
    && { \
        echo '<?php'; \
        echo 'require_once __DIR__ . "/vendor/autoload.php";'; \
        echo '$str = random_bytes(SODIUM_CRYPTO_SECRETBOX_NONCEBYTES);'; \
        echo 'var_dump(sodium_bin2hex($str));'; \
        echo 'sodium_increment($str);'; \
        echo 'var_dump(sodium_bin2hex($str));'; \
    } > compat_increment.php \
    && { \
        echo '<?php'; \
        echo 'require_once __DIR__ . "/vendor/autoload.php";'; \
        echo '$str = random_bytes(SODIUM_CRYPTO_SECRETBOX_NONCEBYTES);'; \
        echo 'var_dump(sodium_bin2hex($str));'; \
        echo 'Sodium\increment($str);'; \
        echo 'var_dump(sodium_bin2hex($str));'; \
    } > increment.php \
    && { \
        echo '#!/bin/sh'; \
        echo 'set -ex'; \
        echo 'php ./compat_increment.php'; \
        echo 'php -n ./compat_increment.php'; \
        echo 'php ./increment.php'; \
    } > run.sh \
    && chmod +x run.sh

CMD /root/sodium_compat/run.sh

 $ docker build -t sodium_compat_73 .
 $ docker run --rm sodium_compat_73
+ php ./compat_increment.php
string(48) "e8d9de99e92e434bdc569ed1bcab8dbc4c9eb4831fcc9f16"
string(48) "e8d9de99e92e434bdc569ed1bcab8dbc4c9eb4831fcc9f16"
+ php -n ./compat_increment.php
string(48) "cabe321d95412bd0c4719d7c96a3f79e5c74927933d36823"
string(48) "cbbe321d95412bd0c4719d7c96a3f79e5c74927933d36823"
+ php ./increment.php
string(48) "7248b751f048f86c952afd4433f642e4b08e0fca73d386ee"
string(48) "7348b751f048f86c952afd4433f642e4b08e0fca73d386ee"

The first script executed uses sodium_increment() and the string is not incremented.
The second execution is the same than the first but with PHP extensions disabled to force the use of the pure PHP polyfill, the string is correctly incremented.
The third execution uses \Sodium\increment() directly, the string is correctly incremented.

So it seems there is an issue with sodium_compat but I'm not sure to understand why you have done in bbb7fac does not work.