Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97495
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
Date: Sat, 31 Dec 2016 08:20:42 +0900
Message-ID: <CAGa2bXZ9EzWHDTBg_ZHGW4KdjiFK4tpE7aMCVTZD5n+UZ2vEWA@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=94eb2c0d4582d020ac0544e87562
Subject: Use decent entropy for uniqid($prefix, TRUE)
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--94eb2c0d4582d020ac0544e87562
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi all,

I thought we must fix due to proposed PHPMailer bug fix patch. (See below
for detail) Previous discussion went wrong because of compatibility
misunderstanding. There is _no_ additional BC issue. Please keep in mind
this.

This is simple change proposal replacing weak entropy to string one.


Background:

uniqid() returns timestamp or timestamp+random value which is based on
current timestamp. It  is obvious that current uniqid() has fatal problems,
uniqid() does not produce acceptable unique ID unlike the name implies.
https://php.net/uniqid


What are the fatal problems:

1) uniqid() (w/o more_entropy option) returns timestamp simply,  tries
to guarantee uniqueness by sleeping short amount of time for a
process. This behavior may create duplicates among processes and by
system clock adjustment. i.e. NTP or like. This issue is not addressed
by proposed patch.

2) uniqid('', TRUE) (w/ more_entropy option) should return acceptable
unique ID, but it's not. This is due to more_entropy is generated by
timestamp, i.e. combined_lcg(), and issue like 1) exists. Proposed
patch fixes this issue.

Use of combined LCG for uniqid() entropy does not make sense now.


Solution:

Current PHP has php_random_int/bytes() which are much better
entropy than combined_lcg(). php_random_int/bytes() use CSPRNG.


Note:

There is no additional compatibility issue at all with
php_random_int/bytes(). Previously merged patch discussion regarding
uniqid() improvement was ruined compatibility FUD. open_basedir
restriction is irrelevant to internal functions. PRNG failure is
unlikely, and the system is unusable anyway if it fails.  e.g. random
functions, session, crypt related functions do not work at all.

Although stronger entropy is needed, uniqid('', TRUE) will have
acceptable uniqueness similar to UUID by replacing CSPRNG entropy.
https://en.wikipedia.org/wiki/Universally_unique_identifier

This kind of change is simple enough change to be merged as "Simple
improvement for new release" or even "Simple bug fix for released
versions", IMO. I'm posting this mail to ask comments because of
previous discussion for this issue.

Following patch is basically the same as patch I committed and
reverted before. I suppose nobody insists RFC for this change now.

I'll merge the patch to master (7.2) if there is no comment.


Patch:

$ git diff
diff --git a/ext/standard/uniqid.c b/ext/standard/uniqid.c
index f429e6d..80dacdb 100644
--- a/ext/standard/uniqid.c
+++ b/ext/standard/uniqid.c
@@ -35,7 +35,7 @@
 #include <sys/time.h>
 #endif

-#include "php_lcg.h"
+#include "php_random.h"
 #include "uniqid.h"

 /* {{{ proto string uniqid([string prefix [, bool more_entropy]])
@@ -77,7 +77,9 @@ PHP_FUNCTION(uniqid)
         * digits for usecs.
         */
        if (more_entropy) {
-               uniqid =3D strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
usec, php_combined_lcg() * 10);
+               zend_long rand;
+               php_random_int(1000000000, 9999999999, &rand, 1);
+               uniqid =3D strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
usec, (double)rand/10000000000);
        } else {
                uniqid =3D strpprintf(0, "%s%08x%05x", prefix, sec, usec);
        }



Proposed patch for PHPMailer command injection issue:

I found following code(patch) for PHPMailer security issue.
https://core.trac.wordpress.org/attachment/ticket/37210/0001
-Upgrade-PHPMailer-from-5.2.14-to-5.2.19.patch

     2086     * Create unique ID
     2087     * @return string
     2088     */
     2089    protected function generateId() {
     2090        return md5(uniqid(time()));
2024 2091    }
2025 2092
2026 2093    /**
=E2=80=A6=E2=80=A6class PHPMailer
2034 2101    {
2035 2102        $body =3D '';
2036 2103        //Create unique IDs and preset boundaries
2037             $this->uniqueid =3D md5(uniqid(time()));
     2104        $this->uniqueid =3D $this->generateId();
2038 2105        $this->boundary[1] =3D 'b1_' . $this->uniqueid;
2039 2106        $this->boundary[2] =3D 'b2_' . $this->uniqueid;
2040 2107        $this->boundary[3] =3D 'b3_' . $this->uniqueid;

Although I never recommend such code, the ID is good enough for this
specific usage. I think we should remove the goccha, "uniqid() is not
unique". This code explains why.


Related RFC:

Improve uniqid() uniqueness
https://wiki.php.net/rfc/uniqid

IMHO, we should enable more_entropy by default, with stronger entropy
prefered.


Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--94eb2c0d4582d020ac0544e87562--