Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97848
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAF+90c9jC0r22NEPbeYXMgL8F_qm0rtJ-nXTeqj_PgjhcY6RRw@mail.gmail.com>
References: <CAGa2bXbFST=5DkhiJ6zJ1hJ5vZYUo7CnCDp+VXcyvSU-bWcyMg@mail.gmail.com>
 <CAGa2bXak3tZ+kC6EeiA=ZCeynGSZbO-4Hpd1v_1oZVDHpSAvsA@mail.gmail.com>
 <71c26cd6df6f59e76dafd31647852c2e@koti.fimnet.fi> <CAGa2bXY0djrgpbC_wNGeRyciNygS59hmuaUaZG-=vAt0dZoxxA@mail.gmail.com>
 <142a3537a99809cf23d78e0eaadc3aef@gmail.com> <7a359bb08b0ad8b046534c15492cec91@gmail.com>
 <CAGa2bXZgbsHM-PZcSUoyA8VZ22rmMmjy41tGPUszC2Acr7Zjyw@mail.gmail.com> <CAF+90c9jC0r22NEPbeYXMgL8F_qm0rtJ-nXTeqj_PgjhcY6RRw@mail.gmail.com>
Date: Wed, 18 Jan 2017 15:04:02 +0900
Message-ID: <CAGa2bXaSux=Pq8+9N4wT9gaqjY--N=D9WwH0560=JEUjutd5LA@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: =?UTF-8?Q?Lauri_Kentt=C3=A4?= <lauri.kentta@gmail.com>, 
	"internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114a48265b9c6505465831e8
Subject: Re: [PHP-DEV] Re: Improving mt_rand() seed
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--001a114a48265b9c6505465831e8
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 18, 2017 at 10:22 AM, Nikita Popov <nikita.ppv@gmail.com> wrote=
:

> On Wed, Jan 18, 2017 at 1:44 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> Hi Lauri,
>>
>> On Tue, Jan 17, 2017 at 11:59 PM, Lauri Kentt=C3=A4 <lauri.kentta@gmail.=
com>
>> wrote:
>>
>> > On 2017-01-17 16:18, Lauri Kentt=C3=A4 wrote:
>> >
>> >> On 2017-01-17 02:34, Yasuo Ohgaki wrote:
>> >>
>> >>> Set state somewhere between MT rand's 2^19937=E2=88=921 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=E2=88=921, 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 =E2=88=921 in 2^19937=E2=88=921). That's c=
ertainly
>> > 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 general=
ly
> 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 stil=
l
> 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. I=
t
> 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
yohgaki@ohgaki.net

--001a114a48265b9c6505465831e8--