uniqid() 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.
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). Yet
uniqid() 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:
In a perfect world, this provides, at most, 61 bits of entropy (32 from
mt_rand(), and only 29 bits from
uniqid()). Given that an attacker can perform an exhaustive 56-bit brute force in 24 hours, and each bit doubles the time required, this means that an attacker only needs 32 days to calculate every possible output of this function, which they could store and search at their convenience.
Such a database would require several petabytes of storage space, but would allow a sophisticated attacker to recover the mt_rand seed and predict the next RNG output.
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 of
SettingsPiwik::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.)
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 like
Common::getSecureRandom(). Which could fall back to the
Common::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:
If an appropriate source of randomness cannot be found, an Exception will be thrown.
random_int()and seeing if it throws an exception.
As part of this issue we also adjust
Common::getRandomInt https://github.com/matomo-org/matomo/blob/4.5.0-b1/core/Common.php#L591-L622 to not fallback to any other method and always use
Since we've been using
random_bytes without any issue it's safe to use
random_int() directly without fallback.