@Joey3000 opened this Issue on January 5th 2016 Contributor

Currently, PHP's rand(), mt_rand() and 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.

  1. 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.
  2. 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 Common::generateUniqId() uses rand(), mt_rand() and 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:

    md5(uniqid(mt_rand(), true));
    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.

  3. 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 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:

Notes:

@tsteur commented on September 19th 2021 Member

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 random_int().

Since we've been using random_bytes without any issue it's safe to use random_int() directly without fallback.

This Issue was closed on September 19th 2021
Powered by GitHub Issue Mirror