Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:96455 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 84776 invoked from network); 19 Oct 2016 00:36:05 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 19 Oct 2016 00:36:05 -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:42375] helo=es-i.jp) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 50/11-12428-1FFB6085 for ; Tue, 18 Oct 2016 20:36:04 -0400 Received: (qmail 79716 invoked by uid 89); 19 Oct 2016 00:35:58 -0000 Received: from unknown (HELO mail-qt0-f182.google.com) (yohgaki@ohgaki.net@209.85.216.182) by 0 with ESMTPA; 19 Oct 2016 00:35:58 -0000 Received: by mail-qt0-f182.google.com with SMTP id f6so6624741qtd.2 for ; Tue, 18 Oct 2016 17:35:57 -0700 (PDT) X-Gm-Message-State: AA6/9Rlq/FulKIQWQyddnN9sUFLG/E9U/FLcBvj149r8I264zvJnvWYOwcFuPtN/0Jz+hTP4h9zNfH9JwTO3mw== X-Received: by 10.200.50.157 with SMTP id z29mr3503949qta.11.1476837351557; Tue, 18 Oct 2016 17:35:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.22.38 with HTTP; Tue, 18 Oct 2016 17:35:10 -0700 (PDT) In-Reply-To: <075a01d22993$9efc6c80$dcf54580$@belski.net> References: <070001d2295e$76b7d730$64278590$@belski.net> <075a01d22993$9efc6c80$dcf54580$@belski.net> Date: Wed, 19 Oct 2016 09:35:10 +0900 X-Gmail-Original-Message-ID: Message-ID: To: Anatol Belski Cc: Joe Watkins , Niklas Keller , Leigh , PHP Internals Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PHP-DEV] Re: [RFC][DISCUSSION] Improve uniqid() uniqueness From: yohgaki@ohgaki.net (Yasuo Ohgaki) On Wed, Oct 19, 2016 at 8:01 AM, Anatol Belski wrot= e: >> -----Original Message----- >> From: Yasuo Ohgaki [mailto:yohgaki@ohgaki.net] >> Sent: Tuesday, October 18, 2016 9:53 PM >> To: Anatol Belski >> Cc: Joe Watkins ; Niklas Keller ; >> Leigh ; PHP Internals >> Subject: Re: [PHP-DEV] Re: [RFC][DISCUSSION] Improve uniqid() uniqueness >> >> Hi Anatol, >> >> On Wed, Oct 19, 2016 at 1:41 AM, Anatol Belski w= rote: >> > AFM the patch is not acceptable for 7.0. It is true that some place wa= s moved >> to the new random int functionality (in password AFAIR). But, it is done= at the >> place and the way that a BC breach is unlikely. Using the throwing varia= nt is for >> sure a BC breach, but also the way pushing while being explicitly asked = to go >> through an RFC, is inappropriate. As the new random_* functions are avai= lable >> and allow to implement the best possible uniqueness in user land, changi= ng the >> algorithm of the existing uniqid() doesn't look to have a valid base. >> > >> >> Any additional error could be BC. It's the fact. >> >> However, your sentence does not make sense at all. >> Do we revert any error emitting bug fix? No, not at all. >> > As far as I remember, uniqid() was never meant to be cryptographically sa= fe. It is documented. Indeed systems might be too fast for microseconds bas= ed function nowadays. In 7.0, my simple exercise - substr(md5(random_bytes= (8)), 0, 13) which does same in the way you want it. We are talking about a= oneliner of new code vs. an old function that is guaranteed in use at any = possible places. The original draft RFC proposal aimed to be cryptographically safe unique ID as much as it can, but the pushed patch is not. > >> We do add errors as normal bug fix process. Many of them are w/o RFC, e= ven >> w/o discussion. >> >> Example: https://bugs.php.net/bug.php?id=3D73238 >> This bug fix caused WordPress caused 3 additional E_WARNING displayed th= at >> can be remove by php.ini or code fix. >> > As a reminder - there's no global rule about functions throwing exception= s, so it is not done by default. Except a couple of new places, no function= throws an exception. The place in password salt code, that was migrated to= the new randomness, did already depend on /dev/urandom and others. However= , even it's based on the new functionality, the old behavior is kept and it= is done intentionally. I agree that this apply to cases such as rand(). We do had to keep rand() behavior even if it produces very bad random on Windows, as you know well. Replacing bad entropy that should be "really random" is different story. Current uniqid('', true)'s result is: unique id (time stamp) + entropy (timestamp based entropy) Isn't this a shame of us providing the result as "uniqid()" call? (I'm not saying original design is bad. Original design was inevitable due to technical limitation, historical reason, just like Windows rand()) Entropy is entropy. As long as format is kept, it does not matter if we use better entropy. > >> Which is important? >> - uniqid() is not unique >> - Really broken system that shouldn't be used may emit error >> >> "/dev/urandom cannot read discussion" is FUD and irrelevant to this disc= ussion. >> Issues with user script random_bytes() implementation or like does not a= pply to >> uniqid() fix. >> > But your implementation indeed uses another API that has other impacts. P= hp_random_bytes is crossplatform, there can be various errors on various pl= atforms. That's the concern as I'd understand it. > >> >> Anyway, are we going to revert anything emit new errors from now on beca= use >> it's BC? >> Are we going to require RFC for this kind of very simple and reasonable = fix? >> I hope not. >> >> IMHO my discussion is logical. Please consider revert the revert. >> Otherwise, we cannot fix even simple bugs. >> > No, IMHO you overdo it a bit. Of course it is acceptable with errors, war= ning, etc. where it makes sense. But it needs a base and a balance also in = other areas for usability, performance, BC, language consistency, etc. If o= ne were telling, it's impossible to do it in PHP - but there are functions = in PHP 7, that provide the functionality aimed. Yes, there is also some leg= acy functionality, so should everything be moved to cryptographically safe?= The answer is obviously - no. For crypto there are dedicated functions and= extensions there. Besides that, you see many other people opposing this ch= ange. An RFC were the way to target the PHP version you want, even 7.0. As = for me, I'd likely vote yes for master, if the throwing part were replaced. I think you and Joe could not follow the discussion. It's okay, reading them all is waste of your time. I read all, but I'm not sure if I understand/remember all of them well. IMHO Oppositions for the patch is based on _wrong_ assumption that "new uniqid() causes common enough errors to be an issue". This wrong assumption is the reason why my commit became an issue, I presume. Could you reconsider decision based on _wrong_ assumption? Thank you. P.S. I'm a bit tired of uniqid() discussion because I expected this is easy= one. This - unique id (time stamp) + entropy (timestamp based entropy) - is obviously wrong for today's PHP. I won't have time to write RFC for this, probably. I have many other things that I would like to improve, like session error status handling improvement that I recently proposed. -- Yasuo Ohgaki yohgaki@ohgaki.net