Use decent entropy for uniqid($prefix, TRUE) View as Markdown

Hi Kazuo,

You misunderstand the mail.
PHPMailer and uniqid() fix is unrelated, but uniqid() is misused proposed
patch in obvious way.

What's your intention?

The point we should learn from the code is, it is clear that users
misunderstand how uniqid() works. You'll find number of such usages if you
search net.

There is uniqid() improvement RFC
https://wiki.php.net/rfc/uniqid

The proposed patch for PHPmailer proves we should improve 'more_entropy'
option and enable it by default. That's my point. Enabling 'more_entropy'
option by default will be handled by RFC process, but we can simply improve
randomness of 'more_entropy' w/o BC for now. This is what I proposed.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2017-01-07 2:15 GMT+01:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Lauri,

On Wed, Jan 4, 2017 at 4:56 AM, Lauri Kenttä lauri.kentta@gmail.com
wrote:

•           zend_long rand;
  

•           php_random_int(1000000000, 9999999999, &rand, 1);
  

•           uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
  

usec, (double)rand/10000000000);

Your code is broken. It produces 0.10000000 - 0.99999999 when it should
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 would
be cleaner and simpler to use just regular 32-bit integers like this:

•           zend_long rand;
  

•           php_random_int(0, 999999999, &rand, 1);
  

•           uniqid = 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 yohgaki@php.net
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 <sys/time.h>
#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 = 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 = 0; i < PHP_UNIQID_ENTROPY_LEN;) {
  

•                   php_random_bytes_throw(&c, sizeof(c));
  

•                   /* Avoid modulo bias */
  

•                   if (c > 249) {
  

•                           continue;
  

•                   }
  

•                   entropy[i] = c % 10 + '0';
  

•                   i++;
  

•           }
  

•           /* Set . for compatibility */
  

•           entropy[1] = '.';
  

•           entropy[PHP_UNIQID_ENTROPY_LEN] = '\0';
  

•           uniqid = strpprintf(0, "%s%08x%05x%s", prefix, sec, usec,
  

entropy);
} else {
uniqid = 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 be
enabled by default. Additionally, we may provide stronger entropy such as
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 checks.
Maybe it will work on older versions, but will behave differently.

Regards, Niklas

•           php_random_int(1000000000, 9999999999, &rand, 1);
  

This should be

•           php_random_int(0, 9999999999, &rand, 1);
  

No, it shouldn't. That fixes none of the reported problems. You still
have too many numbers (integer overflow) and still produce 0.abcdefgh
instead of a.bcdefghi.

If you can't fix it, maybe you shouldn't be doing it in the first
place...

--
Lauri Kenttä

Hi,

•           zend_long rand;
  

•           php_random_int(1000000000, 9999999999, &rand, 1);
  

•           uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
  

usec, (double)rand/10000000000);

Your code is broken. It produces 0.10000000 - 0.99999999 when it should
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 would
be cleaner and simpler to use just regular 32-bit integers like this:

•           zend_long rand;
  

•           php_random_int(0, 999999999, &rand, 1);
  

•           uniqid = strpprintf(0, "%s%08x%05x%d.%08d", prefix, sec,
  

usec, rand % 10, rand / 10);
...
My original patch is better, then,

[yohgaki@dev PHP-master]$ git show 48f1a17886d874dc90867c669481804de90
commit 48f1a17886d874dc90867c669481804de90509e8
Author: Yasuo Ohgaki yohgaki@php.net
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 <sys/time.h>
#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 = 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 = 0; i < PHP_UNIQID_ENTROPY_LEN;) {
  

•                   php_random_bytes_throw(&c, sizeof(c));
  

•                   /* Avoid modulo bias */
  

•                   if (c > 249) {
  

•                           continue;
  

•                   }
  

•                   entropy[i] = c % 10 + '0';
  

•                   i++;
  

•           }
  

•           /* Set . for compatibility */
  

•           entropy[1] = '.';
  

•           entropy[PHP_UNIQID_ENTROPY_LEN] = '\0';
  

•           uniqid = strpprintf(0, "%s%08x%05x%s", prefix, sec, usec,
  

entropy);
} else {
uniqid = strpprintf(0, "%s%08x%05x", prefix, sec, usec);
}

I'll revert the revert commit to master.

No. Lauri's version is better.

Your php_random_bytes_throw() version is significantly slow. Lauri's
version is faster and cleaner.

[original uniqid() using php_combined_lcg]
$ time ./php_uniqid_orig -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real 0m0.366s
user 0m0.350s
sys 0m0.010s

[your php_random_bytes_throw version (commit 48f1a17886d874dc90867c669481804de90509e8)]
$ time ./php_uniqid_yohgaki -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real 0m4.509s
user 0m0.430s
sys 0m4.070s

[Lauri's php_random_int version]
$ time ./php_uniqid_lauri -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real 0m0.664s
user 0m0.260s
sys 0m0.400s

--
Kazuo Oishi

I really don't see any pros for caring about failing CSPRNG and fallback
to weak behavior.

1. BC is extremely unlikely. Basically, no BC on healthy hardware/OS.
2. Then things failed, programs should fail properly. i.e. Shouldn't
   fallback to weaker/problematic code.

Failing closed on a missing CSPRNG isn't really important for uniqid().
There's no guarantee that uniqid() produces ungessable output. It tries to
guarantee uniqueness and fails at the single one job it has for distributed
systems. I still think it's better to just leave it as is and deprecate it,
maybe while moving a UUID ext to core.

Regards, Niklas

Broken CSPRNG is like BUS error, i.e. hardware error, why should we care
so much about it?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net