On Wed, Jan 18, 2017 at 10:22 AM, Nikita Popov <nikita....@gmail.com> wrote:
> On Wed, Jan 18, 2017 at 1:44 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote: > >> Hi Lauri, >> >> On Tue, Jan 17, 2017 at 11:59 PM, Lauri Kenttä <lauri.ken...@gmail.com> >> wrote: >> >> > On 2017-01-17 16:18, Lauri Kenttä wrote: >> > >> >> On 2017-01-17 02:34, Yasuo Ohgaki wrote: >> >> >> >>> Set state somewhere between MT rand's 2^19937−1 cycle. >> >>> >> >> >> >> This is exactly what my patch does. >> >> >> > >> > Or, to be honest, my patch provides 2^19936 possible states, >> > which should be more than enough. >> > >> > To get all 2^19937−1, you would need to get one more bit of >> > entropy (2^19936 to 2^19937) and then check that the state is >> > not all zeros (which is the −1 in 2^19937−1). That's certainly >> > not worth the trouble, so I just set that one "extra" bit to 1. >> > (MT doesn't work if the state is all zeros.) >> >> >> Sorry for sloppy patch reading. >> Your patch initialize whole BG(state) buffer by php_random_bytes(). >> This should be good enough. >> I'll merge this patch. >> >> This better automatic initialization should be included 7.0 and up. >> mt_rand() will at a lot stronger against dictionary attacks. >> Any comments, RMs? >> > > The patch initializes the full MT state vector, approximately 2.5KB of > memory, from a CSPRNG. To put this into perspective, 16 bytes are generally > considered to be sufficient for cryptographic keying material. Does this > seem somewhat disproportionate? > It could be. I haven't read and research MT rand initialization code carefully yet. > > Additionally, the patch has the same concern that has already been > mentioned in the uniqid() thread more than once: If a CSPRNG is not > available, this will make mt_rand() throw. While this is an unusual > situation that should not occur in a well-configured system, this is still > not acceptable. > For the record, "uniqid()'s php_random_*() exception issue" is came from "Userland random_byte() compatibility lib issue which reads /dev/urandom from PHP script". Since PHP script could be affected by open_basedir, some systems cannot read PRNG and throw exception. Internal functions are not affected by open_basedir. If internal functions cannot read /dev/urandom (or like), then the system wouldn't work in fatal way. Therefore, it's irrelevant for internal functions. If you wish to introduce this change, please follow the usual procedure of > submitting a PR, getting it reviewed by the relevant people and only > merging it once it has been approved (or even better, just leave merging to > someone else). I recommend doing this for any non-trivial change. > > For the record, I am generally in favor of seeding MT rand from CSPRNG. It > doesn't really cost us anything and it will make it significantly harder to > exploit code that uses mt_rand() inappropriately, especially for cases > where mt_rand() is only called rarely within a single request. > Lauri, You wrote the patch. Could you make Pull Request to github's php-src repo? If you prefer not to, I'll make the PR. I think your patch should be applied from PHP-7.0 branch. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net
