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? 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. 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. Nikita
