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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
