Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97500
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain giraffy.jp designates 49.212.134.110 as permitted sender)
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: "internals\@lists.php.net" <internals@lists.php.net>
In-Reply-To: <CAGa2bXZ9EzWHDTBg_ZHGW4KdjiFK4tpE7aMCVTZD5n+UZ2vEWA@mail.gmail.com>
	(Yasuo Ohgaki's message of "Sat, 31 Dec 2016 08:20:42 +0900")
References: <CAGa2bXZ9EzWHDTBg_ZHGW4KdjiFK4tpE7aMCVTZD5n+UZ2vEWA@mail.gmail.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
Date: Mon, 02 Jan 2017 03:03:52 +0900
Message-ID: <87y3yupshj.fsf@lil.giraffy.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-2022-jp
Subject: Re: [PHP-DEV] Use decent entropy for uniqid($prefix, TRUE)
From: kazuo@o-ishi.jp (Kazuo Oishi)

Hi,

> 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 = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
> usec, php_combined_lcg() * 10);
> +               zend_long rand;
> +               php_random_int(1000000000, 9999999999, &rand, 1);
> +               uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
> usec, (double)rand/10000000000);
>         } else {
>                 uniqid = strpprintf(0, "%s%08x%05x", prefix, sec, usec);
>         }
>

I think it would be

   php_random_int(0, 9999999999, &rand, 1);
   uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec, usec, (double)rand/1000000000);

   [Please note that "(double)rand/10000000000" is changed to
    "(double)rand/1000000000".]


I don't oppose this patch, but I cannot understand your argument.

> 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.
...
> 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    /**
> ……class PHPMailer
> 2034 2101    {
> 2035 2102        $body = '';
> 2036 2103        //Create unique IDs and preset boundaries
> 2037             $this->uniqueid = md5(uniqid(time()));
>      2104        $this->uniqueid = $this->generateId();
> 2038 2105        $this->boundary[1] = 'b1_' . $this->uniqueid;
> 2039 2106        $this->boundary[2] = 'b2_' . $this->uniqueid;
> 2040 2107        $this->boundary[3] = '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.

Obviously, this is not related to your patch.  "we must fix due to
proposed PHPMailer bug fix patch" is "FUD".  Behavior of uniqid without
$more_entropy=TRUE is not changed.

What's your intention?


BTW, I agree that uniqid() is good enough to use as MIME boundary key, if
collision to body content is checked.  But non-timestamp-based, simple
random value generator is more useful for this purpose.


> IMHO, we should enable more_entropy by default, with stronger entropy
> prefered.

This is BC-break, and I don't think such change is wanted.

(IMHO, what is wanted is new simple random string generator that uses
only specified characters.)

-- 
Kazuo Oishi