Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:97565 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 88297 invoked from network); 7 Jan 2017 20:19:15 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 7 Jan 2017 20:19:15 -0000 Authentication-Results: pb1.pair.com smtp.mail=yohgaki@ohgaki.net; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=yohgaki@ohgaki.net; sender-id=pass Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender) X-PHP-List-Original-Sender: yohgaki@ohgaki.net X-Host-Fingerprint: 180.42.98.130 ns1.es-i.jp Received: from [180.42.98.130] ([180.42.98.130:51822] helo=es-i.jp) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id E4/67-37836-04D41785 for ; Sat, 07 Jan 2017 15:19:14 -0500 Received: (qmail 88372 invoked by uid 89); 7 Jan 2017 20:19:08 -0000 Received: from unknown (HELO mail-wm0-f42.google.com) (yohgaki@ohgaki.net@74.125.82.42) by 0 with ESMTPA; 7 Jan 2017 20:19:08 -0000 Received: by mail-wm0-f42.google.com with SMTP id t79so71920352wmt.0 for ; Sat, 07 Jan 2017 12:19:07 -0800 (PST) X-Gm-Message-State: AIkVDXJTdAoGYPr2QW3IvcGlv6EAKMFXB6rKHYlHlmLxGG+y2ORJ12K+n0T0sUVosCkfu/0EQFU8rqgX+emssg== X-Received: by 10.223.164.130 with SMTP id g2mr5872170wrb.84.1483820341050; Sat, 07 Jan 2017 12:19:01 -0800 (PST) MIME-Version: 1.0 Received: by 10.195.12.8 with HTTP; Sat, 7 Jan 2017 12:18:20 -0800 (PST) In-Reply-To: References: <7500b0c6a50baf49beac70ae01e8b50d@koti.fimnet.fi> Date: Sun, 8 Jan 2017 05:18:20 +0900 X-Gmail-Original-Message-ID: Message-ID: To: Niklas Keller Cc: =?UTF-8?Q?Lauri_Kentt=C3=A4?= , "internals@lists.php.net" Content-Type: multipart/alternative; boundary=f403045f24e056c970054586d8f0 Subject: Re: [PHP-DEV] Use decent entropy for uniqid($prefix, TRUE) From: yohgaki@ohgaki.net (Yasuo Ohgaki) --f403045f24e056c970054586d8f0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Niklas, On Sun, Jan 8, 2017 at 4:08 AM, Niklas Keller wrote: > 2017-01-07 2:15 GMT+01:00 Yasuo Ohgaki : > >> Hi Lauri, >> >> On Wed, Jan 4, 2017 at 4:56 AM, Lauri Kentt=C3=A4 >> wrote: >> >> > On 2016-12-31 01:20, Yasuo Ohgaki wrote: >> > >> >> + zend_long rand; >> >> + php_random_int(1000000000, 9999999999, &rand, 1); >> >> + uniqid =3D strpprintf(0, "%s%08x%05x%.8F", prefix, se= c, >> >> usec, (double)rand/10000000000); >> >> >> > >> > Your code is broken. It produces 0.10000000 - 0.99999999 when it shoul= d >> > produce 0.00000000 - 9.99999999. Also, you have integer overflow on >> 32-bit >> > systems. >> > >> > Why do you mess with oversized integers and doubles and at all? It wou= ld >> > be cleaner and simpler to use just regular 32-bit integers like this: >> > >> > + zend_long rand; >> > + php_random_int(0, 999999999, &rand, 1); >> > + uniqid =3D strpprintf(0, "%s%08x%05x%d.%08d", prefix, = sec, >> > usec, rand % 10, rand / 10); >> > >> > Also, your argument about PHPMailer has nothing to do with your main >> > complaint about lcg_value, since collisions of lcg_value are not the >> > problem there. >> > >> >> + php_random_int(1000000000, 9999999999, &rand, 1); >> >> This should be >> >> + php_random_int(0, 9999999999, &rand, 1); >> >> Anyway, I forgot about 32 bit systems. Thank you for catching this. >> >> My original patch is better, then, >> >> [yohgaki@dev PHP-master]$ git show 48f1a17886d874dc90867c669481804de90 >> commit 48f1a17886d874dc90867c669481804de90509e8 >> Author: Yasuo Ohgaki >> Date: Tue Oct 18 09:04:57 2016 +0900 >> >> Fix bug #47890 #73215 uniqid() should use better random source >> >> diff --git a/ext/standard/uniqid.c b/ext/standard/uniqid.c >> index f429e6d..207cf01 100644 >> --- a/ext/standard/uniqid.c >> +++ b/ext/standard/uniqid.c >> @@ -35,9 +35,11 @@ >> #include >> #endif >> >> -#include "php_lcg.h" >> +#include "php_random.h" >> #include "uniqid.h" >> >> +#define PHP_UNIQID_ENTROPY_LEN 10 >> + >> /* {{{ proto string uniqid([string prefix [, bool more_entropy]]) >> Generates a unique ID */ >> #ifdef HAVE_GETTIMEOFDAY >> @@ -77,7 +79,22 @@ PHP_FUNCTION(uniqid) >> * digits for usecs. >> */ >> if (more_entropy) { >> - uniqid =3D strpprintf(0, "%s%08x%05x%.8F", prefix, sec, >> usec, >> php_combined_lcg() * 10); >> + int i; >> + unsigned char c, entropy[PHP_UNIQID_ENTROPY_LEN+1]; >> + >> + for(i =3D 0; i < PHP_UNIQID_ENTROPY_LEN;) { >> + php_random_bytes_throw(&c, sizeof(c)); >> + /* Avoid modulo bias */ >> + if (c > 249) { >> + continue; >> + } >> + entropy[i] =3D c % 10 + '0'; >> + i++; >> + } >> + /* Set . for compatibility */ >> + entropy[1] =3D '.'; >> + entropy[PHP_UNIQID_ENTROPY_LEN] =3D '\0'; >> + uniqid =3D strpprintf(0, "%s%08x%05x%s", prefix, sec, us= ec, >> entropy); >> } else { >> uniqid =3D strpprintf(0, "%s%08x%05x", prefix, sec, usec= ); >> } >> >> >> I'll revert the revert commit to master. >> >> >> > Why don't you put your effort into a more useful solution such as >> > random_string or something? >> > random_string(PHP_STRING_HEX_LOWER, 32) would produce md5-style output= . >> > random_string(PHP_STRING_BASE64, 32) would produce a lot more entropy. >> > random_string("my_charset", 20) would cover the general case. >> > random_array([1,2,3], 20) could extend this to arbitrary arrays. >> > >> >> They are useful. I would like to have such functions, too. However, they >> don't fix uniqid() problem. i.e. They are out of scope of uniqid() >> improvement. As I mentioned, we should get rid of useless goccha like >> uniqid() is merely a system timestamp. i.e. more_entropy option should b= e >> enabled by default. Additionally, we may provide stronger entropy such a= s >> 128 bits random value. >> >> Do you think uniqid() is good function that we should keep as it is now, >> forever? I guess you do not. Why not improve it > > > Because it's a BC break. A new function with the deprecation of the old > one is way easier for developers to handle. They don't need version check= s. > Maybe it will work on older versions, but will behave differently. > Recent rand() BC is more severe. i.e. rand() is predictable PRNG and backend was replaced to MT rand by 7.1. BC is important, but I couldn't find code that will be broken by "more_entropy" by default. uniqid() is popular function yet misused a lot. I don't think this will change because removing uniqid() is more severe BC and RFC for it wouldn't pass. We knew slight change could break something, but question is "Does it worth it". Wordpress's PHPMailer patch explains it worths. Anyway, are there any objections for revert the revert patch that uses CSPRNG for uniqid() entropy? 48f1a17886d874dc90867c669481804de90509e8 I'll wait few days more. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --f403045f24e056c970054586d8f0--