Using secure random numbers #9473
Labels
c: Security
For issues that make Matomo more secure. Please report issues through HackerOne and not in Github.
Help wanted
Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
not-in-changelog
For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone
Currently, PHP's
rand()
,mt_rand()
anduniqid()
are used in following security-related context. Yet using them in such context is not advisable (see, e.g., https://paragonie.com/blog/2015/07/how-safely-generate-random-strings-and-integers-in-php) - they are not sufficiently unpredictable.SMS verification code in the MobileMessaging plugin
MobileMessaging plugin uses PHP's
mt_rand()
for generation of the SMS verification code in https://github.com/piwik/piwik/blob/2.16.0-b1/plugins/MobileMessaging/API.php#L104.Login Form Nonce
Common::generateUniqId()
is used to generate the nonce (https://github.com/piwik/piwik/blob/2.16.0-b1/core/Nonce.php#L46). YetCommon::generateUniqId()
usesrand()
,mt_rand()
anduniqid()
internally (https://github.com/piwik/piwik/blob/2.16.0-b1/core/Common.php#L538), in the same way as described in https://paragonie.com/blog/2015/10/coming-wordpress-4-4-csprng. I.e., it has only 61 bits of entropy, not the 128 bit of the MD5 length as it may seem - because hashing does not add entropy:Salt generation on Piwik installation
Common::generateUniqId()
(see point 2 above) is also used to generate salt on installation (https://github.com/piwik/piwik/blob/2.16.0-b1/plugins/Installation/Controller.php#L529). This salt (of only 61 bits of entropy) is later used as a secret in various places (see usage ofSettingsPiwik::getSalt()
- e.g. on generation of the password reset token (https://github.com/piwik/piwik/blob/2.16.0-b1/plugins/Login/PasswordResetter.php#L292) and cookies (https://github.com/piwik/piwik/blob/2.16.0-b1/core/Cookie.php#L276 as well as the ignore cookie in https://github.com/piwik/piwik/blob/2.16.0-b1/plugins/UsersManager/Controller.php#L302)).Please consider moving the above cases to a more secure random number generator (a cryptographically secure PRNG). E.g., one of the following libraries:
$factory->getMediumStrengthGenerator()
(Endorsed in the top blog post, but apparently before development of the own random_compat library above.)Notes:
PHP 7 has built-in CSPRNG functions (https://secure.php.net/manual/en/ref.csprng.php), of which the above random_compat library is an emulation.
To keep
Common::generateUniqId()
for non-security related cases (as it is probably faster then a CSPRNG and/or to not lower the system's entropy pool unnecessarily), the new generation functions could be wrapped in something likeCommon::getSecureRandom()
. Which could fall back to theCommon::generateUniqId()
if the library throws an exception due to an inability to use a CSPRNG on some systems (https://github.com/paragonie/random_compat/blob/1.1.4/lib/random_bytes_dev_urandom.php#L139).Note that, similarly, PHP 7 built-in functions can throw, too. From https://secure.php.net/manual/en/function.random-bytes.php:
See also: https://github.com/phpseclib/phpseclib/blob/29b1bb7a58e171cdfa599579d02b6314855b4915/phpseclib/Crypt/Random.php#L57-L68.
To let the Piwik user know if a secure PRNG is available on the system, a respective check could be done among the other Piwik installation checks, with the level of a "warning", to not block the installation. The check could be done by calling
random_bytes()
orrandom_int()
and seeing if it throws an exception.The text was updated successfully, but these errors were encountered: