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 = 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); } 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. 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 yohg...@ohgaki.net
