Re: PHP 7.1.0 to 7.2.0beta2 mt_rand() modulo bias bug

On Wed, Aug 16, 2017 at 9:47 PM, Leigh <leight@gmail.com> wrote: > On Wed, 16 Aug 2017 at 20:13 Solar Designer <solar@openwall.com> wrote: > > > Also, why even bother to support ranges beyond 32-bit? Sounds like a > > misfeature to me, considering it won't(?) be universally available on > > all PHP builds anyway (not on 32-bit ones, right?) and thus shouldn't(?) > > be relied upon by applications (although it might become reasonable for > > application developers not to care about 32-bit soon). I also see few > > use cases for it, even if it were universally available. > > > > It was possible (on 64 bit builds) to specify min and max such that the > size of the output required from mt_rand was the full 64 bit range. > > Prior to 7.1 this full output was created by stretching a single 32 bit > output up to the required range using floating point arithmetic, which > caused other biases in the output. > > Unfortunately when fixing this bias, a new bias was introduced. I took > known working code from the CSPRNG and didn't account for the variable > length of the sample. > > My proposed fix would be to add a "limit_max" variable, initialise it to > UINT32_MAX, and in the first range check where we decide to add an extra > output or not, set it to ZEND_ULONG_MAX. Then the statement creating the > ceiling value can use limit_max instead of the constant value. > I'd suggest to split the 32-bit and 64-bit code codepaths entirely, as the interleaved #ifs are somewhat hard to follow. Something like https://gist.github.com/nikic/64e7ec58ebb6121d350fb80927a65082 (not thoroughly tested). Nikita