[RFC][DISCUSSION] Improve uniqid() uniqueness

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

This is RFC for improving uniqid() uniqueness.
https://wiki.php.net/rfc/uniqid

PR
https://github.com/php/php-src/pull/2123

If there is anything left to discuss, please comment.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

This is RFC for improving uniqid() uniqueness.
https://wiki.php.net/rfc/uniqid

PR
https://github.com/php/php-src/pull/2123

If there is anything left to discuss, please comment.

I think uniqid() should not be changed in BC break way, it should be
left as is.

You said,

Almost all uniqid() usages do not care about return value chars nor
length. Therefore, BC will be minimum.

https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24

but you may be underestimating.

I found that some code saved output of uniqid() without more_entropy to
DB, in the search results. Output length change may cause problem in
such case. And you are not supposed to forget that most of php codes
are not open source and not opened to the public.

In addition, you shoud hear "I expect the numbers to grow" about output
of uniqid(), as reply to you.

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

This is RFC for improving uniqid() uniqueness.
https://wiki.php.net/rfc/uniqid

PR
https://github.com/php/php-src/pull/2123

If there is anything left to discuss, please comment.

I think uniqid() should not be changed in BC break way, it should be
left as is.

You said,

Almost all uniqid() usages do not care about return value chars nor
length. Therefore, BC will be minimum.

https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24

but you may be underestimating.

I found that some code saved output of uniqid() without more_entropy to
DB, in the search results. Output length change may cause problem in
such case. And you are not supposed to forget that most of php codes
are not open source and not opened to the public.

In addition, you shoud hear "I expect the numbers to grow" about output
of uniqid(), as reply to you.

I know some code breaks, but it's not many. It's not fatal BC also.

IMHO, uniqid() should try to generate uniqid() possible. uniqid() does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.

Which is important?

Fix known issues and generate unique ID (as much as possible)
Let it generate non unique ID and ignore for some code may complain.

Fixing is my priority.

Smart developers uses mt_rand() to improve uniqueness, but such tweak
shouldn't be needed in the first place as uniqid() should generate unique ID.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

I know some code breaks, but it's not many. It's not fatal BC also.

IMHO, uniqid() should try to generate uniqid() possible. uniqid() does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.

Which is important?

Fix known issues and generate unique ID (as much as possible)

Let it generate non unique ID and ignore for some code may complain.

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

If good unique ID generator is needed in core, please create new
function with another name like "unique_id".

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

I know some code breaks, but it's not many. It's not fatal BC also.

IMHO, uniqid() should try to generate uniqid() possible. uniqid() does produce
non unique ID because it is system time based. This change mitigates impact of
misuse also which is common in both open and close codes.

Which is important?

Fix known issues and generate unique ID (as much as possible)

Let it generate non unique ID and ignore for some code may complain.

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID as name "uniqid()" implies by default.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

Since we have to change "more entropy" to TRUE by default, why not use
much better entropy? php_combined_lcg() is legacy entropy generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.

If good unique ID generator is needed in core, please create new
function with another name like "unique_id".

Already is, session_create_id() could be used.
Or UUID if 3rd party module can be used.

Although I would not like to leave legacy functions as much as
possible, this could be a vote option. Any more request for new
function, anyone?

I don't like the name because "unique_id()" implies "absolute unique ID",
but we can only provide "very close to unique ID". I'm not sure if it's good to
have "uniqid()" and "unique_id()" that have the same note:

"This function does not guarantee 100% uniqueness".

If we are going to have unique_id(), I would like to keep timestamp
value, since this improves uniqueness and handy for non crypt usage.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

It cannot not produce unique ID as name "uniqid()" implies by default.

It cannot not produce unique ID by default as name "uniqid()" implies.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Rowan Collins — view source — reply

unread

Hi Yasuo,

uniqid() has never been, and is not claimed to be, guaranteed unique to
any particular standard.

Since we have to change "more entropy" to TRUE by default

Is your intention that the version without "more entropy" be deprecated,
and at some point the option removed? Or do you just want to increase
the visibility of this option by enabling it by default?

In other words, do you consider the function to be broken / useless if
this option is not set to true? Or do you think users don't understand
when to use it and when not?

why not use much better entropy? php_combined_lcg() is legacy entropy generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.

What costs and benefits will users see of changing the entropy
generator? Does it make uniqid() collisions less likely, and if so what
kind of probability are we talking about? Does it have a speed or memory
cost (over the existing more_entropy version, i.e. ignoring sleep)?

Even if we accept a) that the default parameters should be changed, and
b) that the source for "more entropy" should be changed, I'm not clear
why the output format also needs to change. Is there some reason the
output of php_random_bytes() can't be encoded into decimal digits,
rather than [0-v]?

Regards,

Rowan Collins
[IMSoP]

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rowan,

uniqid() has never been, and is not claimed to be, guaranteed unique to any
particular standard.

Right. We need to improve documentation. It only has crypt related
usage warning now.

Since we have to change "more entropy" to TRUE by default

Is your intention that the version without "more entropy" be deprecated, and
at some point the option removed? Or do you just want to increase the
visibility of this option by enabling it by default?

I don't think we should remove "more entropy" option now, but it may
be preferred.

Currently, I'm proposing "more entropy" by default and use of better
entropy source.

In other words, do you consider the function to be broken / useless if this
option is not set to true? Or do you think users don't understand when to
use it and when not?

It's useful. It's works as serial ID in most cases.

Apparently, some users don't understand what is does. Some of them are
fatal misusages.

why not use much better entropy? php_combined_lcg() is legacy entropy
generator
must not be used now. New code's entropy is more than a million
times better for the same length. 50 bits entropy is far less enough for
crypt safety, though.

What costs and benefits will users see of changing the entropy generator?
Does it make uniqid() collisions less likely, and if so what kind of
probability are we talking about? Does it have a speed or memory cost (over
the existing more_entropy version, i.e. ignoring sleep)?

I pasted simple benchmark to the PR.
New code uses about 2x cpu time on my Fedora 24. CSPRNG uses more complex
code than php_combined_lcg(), so this is expected.

Even if we accept a) that the default parameters should be changed, and b)
that the source for "more entropy" should be changed, I'm not clear why the
output format also needs to change. Is there some reason the output of
php_random_bytes() can't be encoded into decimal digits, rather than [0-v]?

If I encode php_random_bytes() to the same length of digits, it does
not increase entropy space. It remains about a million (a little less
than 10 bits). It's too small for current baseline.

Proposed code has 50 bits entropy. Besides php_combined_lcg() is based
on system time. Therefore, it is extremely poor entropy source for
uniqid() which generates timestamp string. It's more than a million
times better entropy than now. Since uniqid() has timestamp string
prefix, collision is unlikely and very close to 0, it's much more
reliable than now.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Rowan Collins — view source — reply

unread

I pasted simple benchmark to the PR.
New code uses about 2x cpu time on my Fedora 24. CSPRNG uses more complex
code than php_combined_lcg(), so this is expected.

To me, this is at least as important as changing the length and
character range of the output.

If I encode php_random_bytes() to the same length of digits, it does
not increase entropy space. It remains about a million (a little less
than 10 bits). It's too small for current baseline.

Not enough entropy for what? Can you give some concrete scenarios where
you see this being a problem?

To me, uniqid() is useful because it is a quick way of getting a short
string that's likely to be fairly unique. If that is its purpose, then
making it slower, and its output longer, are not helping anybody.

If it's purpose is to be truly random, and have controllable entropy,
etc, then we might as well deprecate it in favour of random_bytes().

Regards,
Rowan Collins
[IMSoP]

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rowan,

To me, uniqid() is useful because it is a quick way of getting a short
string that's likely to be fairly unique. If that is its purpose, then
making it slower, and its output longer, are not helping anybody.

If it's purpose is to be truly random, and have controllable entropy, etc,
then we might as well deprecate it in favour of random_bytes().

Reasonable.
Then enable more entropy and use php_random_byte() for it.

Question is what format:

the same as now (digits and .)
convert alphanumeric [0-v]
convert to hex [0-f]

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

I had read it, of course. But I could not understand why you chose BC
break way.

Now, I understand your intention to change default value of
"more_entropy" despite of BC break. You do want to change the default
behavior of uniqid.

But I cannot agree.

--
Kazuo Oishi

9 years ago by Marco Pivetta — view source — reply

unread

Hi,

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

I had read it, of course. But I could not understand why you chose BC
break way.

Now, I understand your intention to change default value of
"more_entropy" despite of BC break. You do want to change the default
behavior of uniqid.

But I cannot agree.

--
Kazuo Oishi

--

Full ACK on what Kazuo said.

I use uniqid() daily in my test suites, even new ones (not much else),
and introducing the . everywhere will just cause issues.
Changing string length may also break everything.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Marco,

Full ACK on what Kazuo said.

I use uniqid() daily in my test suites, even new ones (not much else), and
introducing the . everywhere will just cause issues.
Changing string length may also break everything.

It seems you have code that will be broken.
Could you share the code and what it does?

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

I use uniqid() daily in my test suites, even new ones (not much else), and
introducing the . everywhere will just cause issues.
Changing string length may also break everything.

It seems you have code that will be broken.
Could you share the code and what it does?

BTW, my proposed code does not use "." at all.
More entropy is [0-v]{10} from php_random_bytes().

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

I had read it, of course. But I could not understand why you chose BC
break way.

IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?

How serious BC is? It's much less impact than using mt_rand() all over
the code. i.e. rand() and mt_rand() is predictable random generator.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

I had read it, of course. But I could not understand why you chose BC
break way.

IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?

Do you say about extra part which is added by "more_entropy" option?

Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)

I think it is enough to avoid collision in the same usec, for
non-security purpose.

How serious BC is?

You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)

BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.

It's much less impact than using mt_rand() all over
the code. i.e. rand() and mt_rand() is predictable random generator.

Sorry, I cannot understand what you say... (Why mt_rand?)

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

It cannot not produce unique ID by default as name "uniqid()" implies.
Reason is described in the RFC. Please read RFC because it's the
official proposal.

I had read it, of course. But I could not understand why you chose BC
break way.

IMHO, 10 bits (about a million) entropy is not considered enough
entropy, do you?

Do you say about extra part which is added by "more_entropy" option?

Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)

I think it is enough to avoid collision in the same usec, for
non-security purpose.

Oops. Thank you for the correction :) I'll fix the RFC.

Actually not with current implementation. If time is rewined, the same ID
could be generated by chance. Because, both ID part and entropy part
is generated based on the current system time. Collision would be rare,
but it's not rare as it should be at all.

I think minimum would be 64 bits, 128 at least, 256 bits recommended.

How about use 2nd parameter for entropy length? 0 for none, 1 for
default, 10 to 255 chars.

Then user can have choice for prefered number of entropy chars.

How serious BC is?

You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)

BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.

I do think this is needed.

Let's not please security audit companies. Use of current uniqid() in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.

It's much less impact than using mt_rand() all over
the code. i.e. rand() and mt_rand() is predictable random generator.

Sorry, I cannot understand what you say... (Why mt_rand?)

The change have been done.
https://wiki.php.net/rfc/rng_fixes

IMHO, this RFC's BC is nothing compare to this.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

Current "more_entropy" part (10 bytes) pattern is "n.nnnnnnnn" and its
variation is 10^9 (1 billion) as written in your RFC. (about 30bits?)

I think it is enough to avoid collision in the same usec, for
non-security purpose.

Oops. Thank you for the correction :) I'll fix the RFC.

Oops again. I wrote correctly in RFC. Typo was in mail.

How serious BC is?

You should already know that this BC-breack breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)

BC-breack may be acceptable if the change is clearly greate improvement
or obviously necessary. But this change is not, I think.

I do think this is needed.

Let's not please security audit companies. Use of current uniqid() in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.

In short, making PHP be more secure platform (tolerant even for
mistakes) matter to me.
This BC is nothing compared to mt_rand() everywhere.

Anyway, let's talk BC with real code.

I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24
I could only find one code that tests uniqid() return value to test uniqid() (?)

Other than that, almost all code does not care about uniqid() return
value at all.
Who cares about uniqid() return value? for what purpose? other than testing
uniqid() itself?

Even if some test code breaks, does it worth than making PHP be more
secure platform?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

Actually not with current implementation. If time is rewined, the same ID
could be generated by chance. Because, both ID part and entropy part
is generated based on the current system time. Collision would be rare,
but it's not rare as it should be at all.

I think minimum would be 64 bits, 128 at least, 256 bits recommended.

How about use 2nd parameter for entropy length? 0 for none, 1 for
default, 10 to 255 chars.

Then user can have choice for prefered number of entropy chars.

I think, uniqid should be left as is.

How serious BC is?

You should already know that this BC-break breaks existing
valid PHP codes in some situation. (DB error, test failure, etc.)

BC-break may be acceptable if the change is clearly great improvement
or obviously necessary. But this change is not, I think.

I do think this is needed.

Let's not please security audit companies. Use of current uniqid() in
security sensitive context is fatal because it is too easy to predict
generated ID even with "more_entropy". Letting make such mistake
moderate is worth the change.

Misuse is just a misuse. The function uniqid() will not be a secure
function to be able to use in security sensitive context even if this
RFC is passed, isn't it?

In short, making PHP be more secure platform (tolerant even for
mistakes) matter to me.

IMO, THIS change is not important enough and not effective enough to do
in BC break way.

This BC is nothing compared to mt_rand() everywhere.

It would be worth enough.
(cryptographically secure random > predictive by seed)

Anyway, let's talk BC with real code.

I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24

Why didn't you...
You said "BC (BC break) will be minimum"...

I could only find one code that tests uniqid() return value to test uniqid() (?)

Example:
https://github.com/BrianPrz/worklist/blob/master/classes/Login.class.php

In this class, output of uniqid() is saved to DB (maybe).
If 'token' field has only 13 length, output length change cause problem.

Other than that, almost all code does not care about uniqid() return
value at all.
Who cares about uniqid() return value? for what purpose? other than testing
uniqid() itself?

The uniqid() manual explicitly say,

default value of more_entropy is false
the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters.
if more_entropy is set to TRUE, uniqid() will add additional entropy
(using the combined linear congruential generator) at the end of the
return value

http://php.net/manual/en/function.uniqid.php

So, it is fairly valid to design

field length limit to 13 chars in validation code or DB column.
acceptable character type limit to "[0-9A-Za-z]+".
expect increasing value
in their PHP applications.

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

The uniqid() manual explicitly say,

default value of more_entropy is false

the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters.

if more_entropy is set to TRUE, uniqid() will add additional entropy
(using the combined linear congruential generator) at the end of the
return value

http://php.net/manual/en/function.uniqid.php

So, it is fairly valid to design

field length limit to 13 chars in validation code or DB column.

acceptable character type limit to "[0-9A-Za-z]+".

expect increasing value
in their PHP applications.

It's legacy design.

php_combined_lcg() must not be used, especially functions like
uniqid(). i.e. It's supposed to generate unique ID based on time, but
php_combined_lcg() generates pseudo random from current time.

It's more than obvious it's legacy and obsolete today.

Anyway, let's talk BC with real code.

I didn't look into all, but only briefly.
https://searchcode.com/?q=uniqid&loc=0&loc2=10000&lan=24

Why didn't you...
You said "BC (BC break) will be minimum"...

Why should I look into all?
Show me the real code that breaks if you insist this minor BC matters.

I can think of number of way to be broken, but I cannot imagine real
production codes that are broken by change.

BTW, the current manual states uniqid() return unique identifier.
This is false.

I updated the manual, but we should do better job to generate almost unique ID.

<refsect1 role="returnvalues"> &reftitle.returnvalues; <para> - Returns the unique identifier, as a string. + Returns timestamp based unique identifier as a string. </para> + <warning> + <para> + This function tries to create unique identifier, but it does not + guarantee 100% uniqueness of return value. + </para> + </warning> </refsect1>

Current implementation is good enough for most cases, but it can be better.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

The uniqid() manual explicitly say,

default value of more_entropy is false

the returned string will be 13 characters long. If more_entropy is
TRUE, it will be 23 characters.

if more_entropy is set to TRUE, uniqid() will add additional entropy
(using the combined linear congruential generator) at the end of the
return value

http://php.net/manual/en/function.uniqid.php
....
It's legacy design.

php_combined_lcg() must not be used, especially functions like
uniqid(). i.e. It's supposed to generate unique ID based on time, but
php_combined_lcg() generates pseudo random from current time.

It's more than obvious it's legacy and obsolete today.

I agree that uniqid() is legacy design API.

And,

Current implementation is good enough for most cases, but it can be better.

I agree this legacy design API works good enough for most cases.

So, I think it should not be changed in BC break way.

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kauzo,

Current implementation is good enough for most cases, but it can be better.

I agree this legacy design API works good enough for most cases.

So, I think it should not be changed in BC break way.

I updated the RFC.
2nd parameter (more_entropy) is int now.

0 for disable more entropy.
(Compatible with current $more_entropy=FALSE)
1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT
13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.

More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.

Default could be more secure by using [0-v]+.
Marco does not like "." in default output.

I would like to choose default from discussion (or make some vote choices)

Current behavior is not an option because whole point of this RFC is
to make uniqid() to return unique ID almost always even when system
clock is adjusted.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kazuo Oishi — view source — reply

unread

Hi,

I updated the RFC.
2nd parameter (more_entropy) is int now.

0 for disable more entropy.
(Compatible with current $more_entropy=FALSE)

1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT

13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.

More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.

Default could be more secure by using [0-v]+.
Marco does not like "." in default output.

I would like to choose default from discussion (or make some vote choices)

Basically, I will not oppose if backward compatibility is kept (default
$number_of_entropy_chars = 0). I have no opinion about specifying
length of entropy chars.

However, I don't think this new 2nd parameter design is good.

It is not natural (or straightforward) to specify 1 as
parameter named $number_of_entropy_chars, to use 10 digits
entropy ($more_entropy=TRUE compatible output).
Why number of new style entropy ([0-9a-v]+) starts with 13?
(Why not 2 or 11?)
Why max number of entropy is 255? (32^255 = 1275 bits)
(Ease of implementation?)

And, what will happen when 2-12, greater than 255, or negative
value is specified?

--
Kazuo Oishi

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kazuo,

I updated the RFC.
2nd parameter (more_entropy) is int now.

0 for disable more entropy.
(Compatible with current $more_entropy=FALSE)

1 for 10 digits entropy. e.g. 1.23456789
(Compatible with current $more_entropy=TRUE) DEFAULT

13 to 255 to number of entropy [0-v]{13,255} chars.
e.g. 1234abcdefghi (13 = 65 bits)
65 bits entropy + timestamp will provide good enough uniqueness for
most usage.

More secure default may be future scope, but attack against misused
code will be much harder by default as a bonus.

Default could be more secure by using [0-v]+.
Marco does not like "." in default output.

I would like to choose default from discussion (or make some vote choices)

Basically, I will not oppose if backward compatibility is kept (default
$number_of_entropy_chars = 0). I have no opinion about specifying
length of entropy chars.

However, I don't think this new 2nd parameter design is good.

It is not natural (or straightforward) to specify 1 as
parameter named $number_of_entropy_chars, to use 10 digits
entropy ($more_entropy=TRUE compatible output).

Why number of new style entropy ([0-9a-v]+) starts with 13?
(Why not 2 or 11?)

Why max number of entropy is 255? (32^255 = 1275 bits)
(Ease of implementation?)

And, what will happen when 2-12, greater than 255, or negative
value is specified?

I'm going to propose more compatible fix that does not change
length of entropy chars nor parameter type.
Anyway, I was thinking to raise error for any invalid numbers.

I'm going to propose

A. Current format (digits and . e.g. 1.23456788)
B. HEX format ([0-9a-f]{10})

I'll make these vote options.

A is compatible with when $more_entropy=TRUE.
B uses compatible chars without $more_entropy (=FALSE).

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

This is RFC for improving uniqid() uniqueness.
https://wiki.php.net/rfc/uniqid

PR
https://github.com/php/php-src/pull/2123

If there is anything left to discuss, please comment.

Regards,

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Marco Pivetta — view source — reply

unread

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

If you need comments on a patch, send a PR?

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

9 years ago by Kalle Sommer Nielsen — view source — reply

unread

Hi Yasuo

2016-10-02 20:56 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

If anything this should be considered from 7.1+, I don't think we
should change uniqid() mid life time of a branch, ccing Anatol and
Davey

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Updated patch a little

https://gist.github.com/yohgaki/cbe5431f9d072b57af2883a2b5745195

Exception should not be ignored, but added few lines for this.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Leigh — view source — reply

unread

Hi all,

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Updated patch a little

https://gist.github.com/yohgaki/cbe5431f9d072b57af2883a2b5745195

Exception should not be ignored, but added few lines for this.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--

I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Leigh,

I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.

Yes. It seemed it might be slower due to number of retries at first,
but I realized that it isn't later.

It could be something like

$entropy = random_int(10000000000, 9999999999);
$entropy[1] = '.';
$uniqid = timestamp . $entropy;

I don't have particular preference.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Leigh — view source — reply

unread

Hi Leigh,

I'm curious, did you consider using random_int? It already handles
biasing, and you can reduce the repeated calls to random_bytes.

Yes. It seemed it might be slower due to number of retries at first,
but I realized that it isn't later.

It could be something like

$entropy = random_int(10000000000, 9999999999);
$entropy[1] = '.';
$uniqid = timestamp . $entropy;

I don't have particular preference.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Since we want to preserve BC

entropy = random_int(0, 99999999);
uniqid = strpprintf(0, "%s%08x%05x.%08d", prefix, sec, usec, entropy);

9 years ago by Davey Shafik — view source — reply

unread

Hi all,

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Yasuo,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths when
we can. A history of doing this is WHY php is still going strong after so
long.

Thanks,

Davey

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Davey,

Hi all,

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Yasuo,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths when
we can. A history of doing this is WHY php is still going strong after so
long.

Thanks,

I agree fully.

The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.

The main motivation is to simply this RFC discussion. I'll commit this
patch master only.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Pierre Joye — view source — reply

unread

Hi Davey,

Hi all,

On Mon, Oct 3, 2016 at 3:56 AM, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss
what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Yasuo,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug
fixes
(that don't change functionality or break BC) do not.

I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths
when
we can. A history of doing this is WHY php is still going strong after
so
long.

Thanks,

I agree fully.

The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.

The main motivation is to simply this RFC discussion. I'll commit this
patch master only.

Did you solve any of the issues we discussed here? Some of them are BC
breaks.

Thanks
Pierre

9 years ago by Niklas Keller — view source — reply

unread

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 4. Okt. 2016, 03:54:

Hi Davey,

Hi all,

On Mon, Oct 3, 2016 at 3:56 AM, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:

Besides improving "more entropy" the default and data, I prepared
fully compatible patch to simplify discussion.

https://gist.github.com/anonymous/fb615df325d559fa806a265031a06ede

I would like to apply this patch from PHP 7.0 branch, then discuss
what
the default should be.

Any comments?
If there is no objections, I'll apply this few days later.

Yasuo,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

I understand your desire to fix these things, especially the security
related type stuff, but as a group we have a responsibility to create
predictable, sane, and safe (as in, don't break stuff) migration paths
when
we can. A history of doing this is WHY php is still going strong after so
long.

Thanks,

I agree fully.

The only case this patch could break code is caused by broken PRNG in
the system which is fatal anyway. i.e. If PRNG is broken, session
module/randon_*() cannot produce secure session ID/values. We don't
have to worry about changed behavior/BC.

The main motivation is to simply this RFC discussion. I'll commit this
patch master only.

It still needs a RFC.

Regards, Niklas

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Pierre Joye — view source — reply

unread

Hi,

Hi,

I know some code breaks, but it's not many. It's not fatal BC also.

IMHO, uniqid() should try to generate uniqid() possible. uniqid() does
produce
non unique ID because it is system time based. This change mitigates
impact of
misuse also which is common in both open and close codes.

Which is important?

Fix known issues and generate unique ID (as much as possible)

Let it generate non unique ID and ignore for some code may complain.

IMO, improving it (generate better semi-unique ID) is not important
enoungh to introduce unnecessary BC break. (Why returning string length
is changed?)

I fully agree.

If good unique ID generator is needed in core, please create new
function with another name like "unique_id".

I would prefer to document the limitations and point to uuid or the Peng
instead of trying to fix something that was not designed to achieve that
(or was but at a time where the results were acceptable).

--
Kazuo Oishi

9 years ago by Leigh — view source — reply

unread

The list was missed off of Yasuo's replies to me, replying including the
list

Hi Leigh,

Since we want to preserve BC

entropy = random_int(0, 99999999);
uniqid = strpprintf(0, "%s%08x%05x.%08d", prefix, sec, usec, entropy);

Current entropy is double from php_combined_lcg() and has 10 chars
length,
has [0-9].[0-9]{8} format.

"F"->"d" does not work. It should be something like

entropy = (double) random_int(0, 9999999999);

No it shouldn't. Don't do this. It is an unnecessary conversion. The fact
the lcg returns a double is irrelevant. What is relevant is the 8 digits in
order to maintain BC. The 8 digits you receive from random_int will still
be higher quality than the 10 you get from the lcg rounded to 8 places.

uniqid = strpprintf(0, "%s%08x%05x.%08F", prefix, sec, usec,
entropy/100000000);

Current entropy is double from php_combined_lcg() and has 10 chars
length,
has [0-9].[0-9]{8} format.

"F"->"d" does not work. It should be something like

entropy = (double) random_int(0, 9999999999);
uniqid = strpprintf(0, "%s%08x%05x.%08F", prefix, sec, usec,
entropy/100000000);

Forgot to mention, this code leak more information about PRNG state
than my patch because php_random_int() copies random binary data into
long. It's still part of it and exposure of random data shouldn't
matter, so this is minor issue.

I think there is a misunderstanding here. You're using the CSPRNG which is
designed such that the entire output can be made public without you being
able to predict the next result. That is the definition of a CSPRNG. Also
remember this is "output" not "state".

While researching how to implement these CSPRNG functions, I spoke with
real security experts on the subject, they all said the same thing: Use the
system CSPRNG, and yes, it is fine to expose the output directly.

Also if you really are worried (which you shouldn't be), requesting 8
digits from random_int will effectively discard 5 or 37 bits of output
depending on whether you're on a 32 or 64 bit platform. You cannot know the
value of sequential outputs.

I'll update gist.
Any more comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Leigh,

The list was missed off of Yasuo's replies to me, replying including the
list

Me too :)

Hi Leigh,

Since we want to preserve BC

entropy = random_int(0, 99999999);
uniqid = strpprintf(0, "%s%08x%05x.%08d", prefix, sec, usec, entropy);

Current entropy is double from php_combined_lcg() and has 10 chars
length,
has [0-9].[0-9]{8} format.

"F"->"d" does not work. It should be something like

entropy = (double) random_int(0, 9999999999);

No it shouldn't. Don't do this. It is an unnecessary conversion. The fact
the lcg returns a double is irrelevant. What is relevant is the 8 digits in
order to maintain BC. The 8 digits you receive from random_int will still be
higher quality than the 10 you get from the lcg rounded to 8 places.

uniqid = strpprintf(0, "%s%08x%05x.%08F", prefix, sec, usec,
entropy/100000000);

There is misunderstanding for the format.
The patch is made to be fully compatible with current output.

php_combined_lcg() produces value between 1 and 0. It is multiplied
by 10, and 8 decimal numbers are used, so additional entropy is
something like

1.23456789 (10 chars)

[yohgaki@dev ~]$ php -v
PHP 5.6.26 (cli) (built: Sep 16 2016 04:36:41)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

[yohgaki@dev ~]$ php -r 'var_dump(uniqid(), uniqid("", true));'
string(13) "57f4ce3df2ea5"
string(23) "57f4ce3df2ea81.98781982"

Current uniqid('', true) adds 1 int char + '.' + 8 decimal char.
Tricky format string, but this is what it does.

If we would like to avoid int to double conversion, we may call
php_random_int() twice. Not sure if it's worth or not, though.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8

I thought there is php_random_int(), but it's not.
So this is one of the best patch for this purpose.

There is bug reports that request stronger uniqueness by default.
I may address this issue, but I would like to fix other things for the
time being.

Regards,

--
Yasuo Ohgaki

9 years ago by Niklas Keller — view source — reply

unread

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 02:21:

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

Doesn't this throw now in some environments where /dev/urandom isn't
readable?

Regards, Niklas

http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8

I thought there is php_random_int(), but it's not.
So this is one of the best patch for this purpose.

There is bug reports that request stronger uniqueness by default.
I may address this issue, but I would like to fix other things for the
time being.

Regards,

--
Yasuo Ohgaki

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 02:21:

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

Doesn't this throw now in some environments where /dev/urandom isn't
readable?

It could happen, but such system should not be used now a days.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Niklas Keller — view source — reply

unread

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 08:47:

Hi Niklas,

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 02:21:

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

Doesn't this throw now in some environments where /dev/urandom isn't
readable?

It could happen, but such system should not be used now a days.

Sure, but it did happen that shared hosts block it, noticed during
random_compat adoption.

You claimed there isn't any BC break.

Regards, Niklas

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 08:47:

Hi Niklas,

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 02:21:

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

Doesn't this throw now in some environments where /dev/urandom isn't
readable?

It could happen, but such system should not be used now a days.

Sure, but it did happen that shared hosts block it, noticed during
random_compat adoption.

You claimed there isn't any BC break.

The line should be

"There is no BC for usable systems"

Any file permission could disturb PHP script execution, couldn't it?

I think it's nothing special for /dev/urandom. User should set up system
correctly to use PHP. Then there is no BC at all.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Lester Caine — view source — reply

unread

Sure, but it did happen that shared hosts block it, noticed during

random_compat adoption.

You claimed there isn't any BC break.
The line should be

"There is no BC for usable systems"

Any file permission could disturb PHP script execution, couldn't it?

I think it's nothing special for /dev/urandom. User should set up system
correctly to use PHP. Then there is no BC at all.

'Suppliers' should perhaps be helped to configure their systems so the
users can use things, but things like /dev/urandom may need some
additional notes to help identify problems when frameworks like owncloud
start throwing errors. As Niklas says it's shared environments where
this one may bite.

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

9 years ago by Niklas Keller — view source — reply

unread

Lester Caine lester@lsces.co.uk schrieb am Di., 18. Okt. 2016, 11:42:

Sure, but it did happen that shared hosts block it, noticed during

random_compat adoption.

You claimed there isn't any BC break.
The line should be

"There is no BC for usable systems"

Any file permission could disturb PHP script execution, couldn't it?

I think it's nothing special for /dev/urandom. User should set up system
correctly to use PHP. Then there is no BC at all.

'Suppliers' should perhaps be helped to configure their systems so the
users can use things, but things like /dev/urandom may need some
additional notes to help identify problems when frameworks like owncloud
start throwing errors. As Niklas says it's shared environments where
this one may bite.

Just to be clear: I don't argue that those systems are broken, I just say
that there is a BC break for those systems and that this has to be
documented.

Regards, Niklas

--

Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

9 years ago by Lester Caine — view source — reply

unread

'Suppliers' should perhaps be helped to configure their systems so the

users can use things, but things like /dev/urandom may need some
additional notes to help identify problems when frameworks like owncloud
start throwing errors. As Niklas says it's shared environments where
this one may bite.

Just to be clear: I don't argue that those systems are broken, I just say
that there is a BC break for those systems and that this has to be
documented.

Yes ... and the RFC process is at least part of the documentation.

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

9 years ago by Yasuo Ohgaki — view source — reply

unread

'Suppliers' should perhaps be helped to configure their systems so the

users can use things, but things like /dev/urandom may need some
additional notes to help identify problems when frameworks like owncloud
start throwing errors. As Niklas says it's shared environments where
this one may bite.

Just to be clear: I don't argue that those systems are broken, I just say
that there is a BC break for those systems and that this has to be
documented.

Yes ... and the RFC process is at least part of the documentation.

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Lester Caine — view source — reply

unread

The patch committed is pure bug fix.
https://www.google.co.uk/search?q=%2Fdev%2Furandom+is+not+readable+by+php

Even bug fixes need proper documentation to avoid the WTF !

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

9 years ago by Yasuo Ohgaki — view source — reply

unread

The patch committed is pure bug fix.
https://www.google.co.uk/search?q=%2Fdev%2Furandom+is+not+readable+by+php

Even bug fixes need proper documentation to avoid the WTF !

I'm about to add the doc.

Anyway, this is due to "open_basedir" restriction.
"open_basedir" does not affect php_random_bytes() at all.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Joe Watkins — view source — reply

unread

Morning,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

You were told very specifically that the kinds of changes you proposed here
require an RFC.

You chose to ignore that, and merge an implementation into frozen branches
of PHP.

I have reverted this change.

Do not do that again.

Cheers
Joe

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016, 08:47:

Hi Niklas,

Yasuo Ohgaki yohgaki@ohgaki.net schrieb am Di., 18. Okt. 2016,
02:21:

Hi all,

I committed this patch that simply use php_random_bytes() w/o any BC.

Doesn't this throw now in some environments where /dev/urandom isn't
readable?

It could happen, but such system should not be used now a days.

Sure, but it did happen that shared hosts block it, noticed during
random_compat adoption.

You claimed there isn't any BC break.

The line should be

"There is no BC for usable systems"

Any file permission could disturb PHP script execution, couldn't it?

I think it's nothing special for /dev/urandom. User should set up system
correctly to use PHP. Then there is no BC at all.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Joe,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

You were told very specifically that the kinds of changes you proposed here
require an RFC.

This comment is for original proposal that changes output format, isn't it?

It simply switches entropy source which we already relied on.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Joe,

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug fixes
(that don't change functionality or break BC) do not.

You were told very specifically that the kinds of changes you proposed here
require an RFC.

This comment is for original proposal that changes output format, isn't it?

It simply switches entropy source which we already relied on.

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

Thanks.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Niklas Keller — view source — reply

unread

2016-10-18 14:02 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Joe,

On Tue, Oct 18, 2016 at 7:32 PM, Joe Watkins pthreads@pthreads.org
wrote:

This change should go through the standard RFC process and should be
targeted at 7.2+ (master) only.

Please check with the RMs before merging functionality changes into
release
branches. All functionality changes need consent and consensus. Bug
fixes
(that don't change functionality or break BC) do not.

You were told very specifically that the kinds of changes you proposed
here
require an RFC.

This comment is for original proposal that changes output format,
isn't it?

It simply switches entropy source which we already relied on.

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

This discussion shows there should be a RFC and a vote. I'd not consider
this a simple bug fix, after all it doesn't really fix it.

If we want to fix it in core, we'd better include an UUID generation
mechanism than fixing uniq_id.

Regards, Niklas

Thanks.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

This discussion shows there should be a RFC and a vote. I'd not consider
this a simple bug fix, after all it doesn't really fix it.

If we want to fix it in core, we'd better include an UUID generation
mechanism than fixing uniq_id.

UUID like uniqueness is not the subject of uniqid(), isn't it?

As I wrote, it's simple bug fix.

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Niklas Keller — view source — reply

unread

2016-10-18 14:12 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

This discussion shows there should be a RFC and a vote. I'd not consider
this a simple bug fix, after all it doesn't really fix it.

If we want to fix it in core, we'd better include an UUID generation
mechanism than fixing uniq_id.

UUID like uniqueness is not the subject of uniqid(), isn't it?

UUID = Universally Unique Identifier
uniqid = Generate a unique ID

Where is uniqueness not the subject of uniqid()?

As I wrote, it's simple bug fix.

The issue is that it doesn't fix it. Maybe it band aids. But it doesn't fix
uniqid.

It's exactly why I proposed to better deprecate uniqid. We can do that in
7.2 and provide UUIDs as a standardized and superior alternative.

Regards, Niklas

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected
uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Niklas,

2016-10-18 14:12 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

This discussion shows there should be a RFC and a vote. I'd not consider
this a simple bug fix, after all it doesn't really fix it.

If we want to fix it in core, we'd better include an UUID generation
mechanism than fixing uniq_id.

UUID like uniqueness is not the subject of uniqid(), isn't it?

UUID = Universally Unique Identifier
uniqid = Generate a unique ID

Where is uniqueness not the subject of uniqid()?

As I wrote, it's simple bug fix.

The issue is that it doesn't fix it. Maybe it band aids. But it doesn't fix
uniqid.

It's exactly why I proposed to better deprecate uniqid. We can do that in
7.2 and provide UUIDs as a standardized and superior alternative.

OK, I understand you prefer to deprecate uniqid(), but I guess
uniqid() deprecation is less likely to be passed than improving
uniqid() uniqueness with a little BC.

If you search uniqid() usage, you'll see UUID is too much for many
usages. uniqid() has it own use cases.

Current uniqid() is not unique at all. The patch simply fixes it by
using proper entropy, no BC basically.

What's wrong with this?

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Niklas Keller — view source — reply

unread

2016-10-18 14:41 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

2016-10-18 14:12 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

As you can see from last minutes discussion.

"/dev/urandom cannot be read" is FUD.
It's pure bug fix. (I intentionally made patch easy to extend used
chars, though)

Would you consider revert the revert?

This discussion shows there should be a RFC and a vote. I'd not
consider
this a simple bug fix, after all it doesn't really fix it.

If we want to fix it in core, we'd better include an UUID generation
mechanism than fixing uniq_id.

UUID like uniqueness is not the subject of uniqid(), isn't it?

UUID = Universally Unique Identifier
uniqid = Generate a unique ID

Where is uniqueness not the subject of uniqid()?

As I wrote, it's simple bug fix.

The issue is that it doesn't fix it. Maybe it band aids. But it doesn't
fix
uniqid.

It's exactly why I proposed to better deprecate uniqid. We can do that in
7.2 and provide UUIDs as a standardized and superior alternative.

OK, I understand you prefer to deprecate uniqid(), but I guess
uniqid() deprecation is less likely to be passed than improving
uniqid() uniqueness with a little BC.

If you search uniqid() usage, you'll see UUID is too much for many
usages. uniqid() has it own use cases.

Current uniqid() is not unique at all.

Right, and it's impossible to fix it without breaking BC, because really
fixing it would require more output.

The patch simply fixes it by
using proper entropy, no BC basically.

It might be fine committing this to master. But as you say, uniqid is
broken and I'd not consider it fixed with just changing the source of
entropy but leaving the output as is.

What's wrong with this?

Committing it directly to a frozen branch is.

Regards, Niklas

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected
uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Kalle Sommer Nielsen — view source — reply

unread

2016-10-18 18:41 GMT+02:00 Anatol Belski anatol.php@belski.net:

AFM the patch is not acceptable for 7.0. It is true that some place was moved to the new random int functionality (in password AFAIR). But, it is done at the place and the way that a BC breach is unlikely. Using the throwing variant is for sure a BC breach, but also the way pushing while being explicitly asked to go through an RFC, is inappropriate. As the new random_* functions are available and allow to implement the best possible uniqueness in user land, changing the algorithm of the existing uniqid() doesn't look to have a valid base.

I must add, despite not following the discussion entirely, that it
should also be approved by the two 7.1 RMs to be committed,
considering we are in RC4 stage at this point and I don't think we
should just commit things this late without the RM consent to it.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kalle and all,

2016-10-18 18:41 GMT+02:00 Anatol Belski anatol.php@belski.net:

AFM the patch is not acceptable for 7.0. It is true that some place was moved to the new random int functionality (in password AFAIR). But, it is done at the place and the way that a BC breach is unlikely. Using the throwing variant is for sure a BC breach, but also the way pushing while being explicitly asked to go through an RFC, is inappropriate. As the new random_* functions are available and allow to implement the best possible uniqueness in user land, changing the algorithm of the existing uniqid() doesn't look to have a valid base.

I must add, despite not following the discussion entirely, that it
should also be approved by the two 7.1 RMs to be committed,
considering we are in RC4 stage at this point and I don't think we
should just commit things this late without the RM consent to it.

This is usually I do. You'll see my mails discussing which branches to
merge that is not simple. For almost all bug fixes, I do not see
discussion for merging released branchs.

(Following questions are not for Kalle)

Most bug fixes are not discussed at all here.
What is making this simple bug special?

What's wrong with this simple fix?
What makes this a special requires RFC?

The patch committed is pure bug fix.

uniqid() is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg(). (I added large warning to the manual
recently, though)

unique id (time stamp) + entropy (timestamp based entropy)

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

If any new errors cannot be tolerated with bug fix, are we going to
revert any bug fixes with new error?
Besides, "uniqid() will emit error because it uses /dev/urandom" is
FUD, isn't it?

If there is no reasonable / logical answers for these,
The patch should be included PHP 7.0 and up.

BTW, who really think the patch is offending patch to be merged to
released branches?
Please raise your hand now. I don't think there are many.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Anatol,

AFM the patch is not acceptable for 7.0. It is true that some place was moved to the new random int functionality (in password AFAIR). But, it is done at the place and the way that a BC breach is unlikely. Using the throwing variant is for sure a BC breach, but also the way pushing while being explicitly asked to go through an RFC, is inappropriate. As the new random_* functions are available and allow to implement the best possible uniqueness in user land, changing the algorithm of the existing uniqid() doesn't look to have a valid base.

Any additional error could be BC. It's the fact.

However, your sentence does not make sense at all.
Do we revert any error emitting bug fix? No, not at all.

We do add errors as normal bug fix process. Many of them are w/o RFC,
even w/o discussion.

Example: https://bugs.php.net/bug.php?id=73238
This bug fix caused WordPress caused 3 additional E_WARNING displayed
that can be remove by php.ini or code fix.

Which is important?

uniqid() is not unique
Really broken system that shouldn't be used may emit error

"/dev/urandom cannot read discussion" is FUD and irrelevant to this
discussion. Issues with user script random_bytes() implementation or
like does not apply to uniqid() fix.

Anyway, are we going to revert anything emit new errors from now on
because it's BC?
Are we going to require RFC for this kind of very simple and reasonable fix?
I hope not.

IMHO my discussion is logical. Please consider revert the revert.
Otherwise, we cannot fix even simple bugs.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Rowan Collins — view source — reply

unread

Which is important?

uniqid() is not unique

Really broken system that shouldn't be used may emit error

Frankly, both are pretty rare cases. From the way you talk about it,
everybody who uses uniqid() will get duplicate values all the time, when
in fact, it's incredibly unlikely that anyone will even notice.

I know when I've used it, it's for things like avoiding duplicate id
attributes on an HTML page, or varying the URL of an asset by adding a
token to the URL. It's perfectly usable as is for those situations, and
I use it with full knowledge that it has a small chance of generating
colliding values.

I'm happy to see it improved, but I don't see any hurry, unless I'm
completely misunderstanding the chances of seeing collisions.

Regards,

--
Rowan Collins
[IMSoP]

9 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Rowan,

Which is important?

uniqid() is not unique

Really broken system that shouldn't be used may emit error

Frankly, both are pretty rare cases. From the way you talk about it,
everybody who uses uniqid() will get duplicate values all the time, when in
fact, it's incredibly unlikely that anyone will even notice.

I know when I've used it, it's for things like avoiding duplicate id
attributes on an HTML page, or varying the URL of an asset by adding a token
to the URL. It's perfectly usable as is for those situations, and I use it
with full knowledge that it has a small chance of generating colliding
values.

I'm happy to see it improved, but I don't see any hurry, unless I'm
completely misunderstanding the chances of seeing collisions.

This is reasonable discussion.

I do use uniqid() for HTML id attributes. It is difficult to detect id
collisions
since it's not server side. (Technically it can, but it requires more resources)
While it works almost always, but it should be better than now. Otherwise,
I'm very uncomfortable with uniqid().

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

9 years ago by Yasuo Ohgaki — view source — reply

unread

-----Original Message-----
From: Yasuo Ohgaki [mailto:yohgaki@ohgaki.net]
Sent: Tuesday, October 18, 2016 9:53 PM
To: Anatol Belski anatol.php@belski.net
Cc: Joe Watkins pthreads@pthreads.org; Niklas Keller me@kelunik.com;
Leigh leight@gmail.com; PHP Internals internals@lists.php.net
Subject: Re: [PHP-DEV] Re: [RFC][DISCUSSION] Improve uniqid() uniqueness

Hi Anatol,

AFM the patch is not acceptable for 7.0. It is true that some place was moved
to the new random int functionality (in password AFAIR). But, it is done at the
place and the way that a BC breach is unlikely. Using the throwing variant is for
sure a BC breach, but also the way pushing while being explicitly asked to go
through an RFC, is inappropriate. As the new random_* functions are available
and allow to implement the best possible uniqueness in user land, changing the
algorithm of the existing uniqid() doesn't look to have a valid base.

Any additional error could be BC. It's the fact.

However, your sentence does not make sense at all.
Do we revert any error emitting bug fix? No, not at all.

As far as I remember, uniqid() was never meant to be cryptographically safe. It is documented. Indeed systems might be too fast for microseconds based function nowadays. In 7.0, my simple exercise - substr(md5(random_bytes(8)), 0, 13) which does same in the way you want it. We are talking about a oneliner of new code vs. an old function that is guaranteed in use at any possible places.

The original draft RFC proposal aimed to be cryptographically safe
unique ID as much as it can, but the pushed patch is not.

We do add errors as normal bug fix process. Many of them are w/o RFC, even
w/o discussion.

Example: https://bugs.php.net/bug.php?id=73238
This bug fix caused WordPress caused 3 additional E_WARNING displayed that
can be remove by php.ini or code fix.

As a reminder - there's no global rule about functions throwing exceptions, so it is not done by default. Except a couple of new places, no function throws an exception. The place in password salt code, that was migrated to the new randomness, did already depend on /dev/urandom and others. However, even it's based on the new functionality, the old behavior is kept and it is done intentionally.

I agree that this apply to cases such as rand(). We do had to keep
rand() behavior even if it produces very bad random on Windows, as you
know well.
Replacing bad entropy that should be "really random" is different story.

Current uniqid('', true)'s result is:

unique id (time stamp) + entropy (timestamp based entropy)

Isn't this a shame of us providing the result as "uniqid()" call?

(I'm not saying original design is bad. Original design was inevitable
due to technical limitation, historical reason, just like Windows
rand()) Entropy is entropy. As long as format is kept, it does not
matter if we use better entropy.

Which is important?

uniqid() is not unique

Really broken system that shouldn't be used may emit error

"/dev/urandom cannot read discussion" is FUD and irrelevant to this discussion.
Issues with user script random_bytes() implementation or like does not apply to
uniqid() fix.

But your implementation indeed uses another API that has other impacts. Php_random_bytes is crossplatform, there can be various errors on various platforms. That's the concern as I'd understand it.

Anyway, are we going to revert anything emit new errors from now on because
it's BC?
Are we going to require RFC for this kind of very simple and reasonable fix?
I hope not.

IMHO my discussion is logical. Please consider revert the revert.
Otherwise, we cannot fix even simple bugs.

No, IMHO you overdo it a bit. Of course it is acceptable with errors, warning, etc. where it makes sense. But it needs a base and a balance also in other areas for usability, performance, BC, language consistency, etc. If one were telling, it's impossible to do it in PHP - but there are functions in PHP 7, that provide the functionality aimed. Yes, there is also some legacy functionality, so should everything be moved to cryptographically safe? The answer is obviously - no. For crypto there are dedicated functions and extensions there. Besides that, you see many other people opposing this change. An RFC were the way to target the PHP version you want, even 7.0. As for me, I'd likely vote yes for master, if the throwing part were replaced.

I think you and Joe could not follow the discussion. It's okay,
reading them all is waste of your time. I read all, but I'm not sure
if I understand/remember all of them well.

IMHO Oppositions for the patch is based on wrong assumption that
"new uniqid() causes common enough errors to be an issue". This wrong
assumption is the reason why my commit became an issue, I presume.

Could you reconsider decision based on wrong assumption?

Thank you.

P.S. I'm a bit tired of uniqid() discussion because I expected this is easy one.
This - unique id (time stamp) + entropy (timestamp based entropy) - is
obviously wrong for today's PHP.
I won't have time to write RFC for this, probably. I have many other
things that I would like to improve, like
session error status handling improvement that I recently proposed.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Anatol,

I won't have time to write RFC for this, probably. I have many other things that I
would like to improve, like session error status handling improvement that I
recently proposed.

I see. It's a pity you won't have time to write an RFC. I see one already in place on the wiki though. I see also your several other patches hanging on gihtub. IMHO it is a real waste of time to abandon the work you've done, without really pulling it through. With uniqid(), maybe it'd be even the right decision to return to your original RFC, or just to reduce it to comply with the simple patch variant. I'm sure, that no one wants to lose the good contributions, even though it might take some effort to reach the common ground sometimes.

I meant I wouldn't have time for RFC only replace php_combined_lcg().
I'll address uniqid() improvement sometime in the future.

To all,

We constantly get "uniqid() is not unique" bug reports.
In the meantime, any objection for adding following note to uniqid() manual.

"Do not make assumption for uniqid() output format, entropy
especially. uniqid() output format may be changed to provide
reasonably unique ID in future versions."

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Kalle Sommer Nielsen — view source — reply

unread

Hi Yasuo

2016-10-20 9:18 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

"Do not make assumption for uniqid() output format, entropy
especially. uniqid() output format may be changed to provide
reasonably unique ID in future versions."

Sounds reasonable to me; although I would phrase it a little
differently, something along the lines of:

The <function>uniquid</function> cannot be relied on to be unique and
there can occur collisions, even with the
<parameter>more_entrophy</parameter> set to &true;.

As for the in future version, although we may do that, I don't think
we should document something that is not in the core yet. What do you
think?

--
regards,

Kalle Sommer Nielsen
kalle@php.net

8 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kalle,

2016-10-20 9:18 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

"Do not make assumption for uniqid() output format, entropy
especially. uniqid() output format may be changed to provide
reasonably unique ID in future versions."

Sounds reasonable to me; although I would phrase it a little
differently, something along the lines of:

The <function>uniquid</function> cannot be relied on to be unique and
there can occur collisions, even with the
<parameter>more_entrophy</parameter> set to &true;.

I added warnings to uniqid() manual recently. It's visible now, could
you check this?

http://php.net/manual/en/function.uniqid.php

Warnings are based on following facts.

uniqid(); // without entropy

usleep(1) is called to get unique timestamp, but NTP can disturb and
uniqid() can result in the same ID.

uniqid('', TRUE); // with entropy

It's better, but entropy is based on system timestamp and there is no
usleep(1), so uniqid() is more sensitive to system clock adjustment by
NTP, and uniqid() can result in the same ID.

Collision is unlikely, but it not that unlikely with true CSPRNG based
entropy. Therefore, I made warning a little strong. With CSPRNG, we
may use more gentle warning. IMO.

As for the in future version, although we may do that, I don't think
we should document something that is not in the core yet. What do you
think?

Entropy is some random value by definition, so we may tell users "Make
no assumption for entropy" at least. IMO.
Is this reasonable to you?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Kalle,

I forgot to mention one more thing.

Warnings are based on following facts.

uniqid(); // without entropy

usleep(1) is called to get unique timestamp, but NTP can disturb and
uniqid() can result in the same ID.

uniqid('', TRUE); // with entropy

It's better, but entropy is based on system timestamp and there is no
usleep(1), so uniqid() is more sensitive to system clock adjustment by
NTP, and uniqid() can result in the same ID.

Collision is unlikely, but it not that unlikely with true CSPRNG based
entropy. Therefore, I made warning a little strong. With CSPRNG, we
may use more gentle warning. IMO.

Application requires unique ID under across multi process/thread
tasks, it will have more chance to have collided unique ID.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Pierre Joye — view source — reply

unread

Application requires unique ID under across multi process/thread
tasks, it will have more chance to have collided unique ID.

uniqid fill(s|ed) some needs or maybe still fits for some.

However for modern application with many concurrent requests or nodes,
it does not fit anymore, since long. Do we need to fix a broken hammer
to fix the screw? I do not think so.

I suggested already to simply improve it if we can do it without
breaking BC and recommend to use something designed for such tasks,
UUID. ramsey/uuid is one of them, other are available.

Cheers,

Pierre

@pierrejoye | http://www.libgd.org

8 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Pierre,

Application requires unique ID under across multi process/thread
tasks, it will have more chance to have collided unique ID.

uniqid fill(s|ed) some needs or maybe still fits for some.

However for modern application with many concurrent requests or nodes,
it does not fit anymore, since long. Do we need to fix a broken hammer
to fix the screw? I do not think so.

I'm all for bringing UUID to PHP by default, encourage users to use it
for applications requires very unique ID. Let's have UUID module by
default someday!

I suggested already to simply improve it if we can do it without
breaking BC and recommend to use something designed for such tasks,
UUID. ramsey/uuid is one of them, other are available.

I'm aware of that entropy must be enabled by default, too. I guess you
feel uniqid() improvement is impossible due to vote. You could be
right about it.

As far as I searched, there is no code have problem even when entropy
on/off and additional chars/format. (Found one test script that tests
uniqid(). Cannot tell if db has char(13) or like, though) We made
rand() a alias of mt_rand(). IMHO, we are better not to leave too
weak unique ID generation function alone. HTML ID attribute and test
data ID is common uniqid() use case. Let's make it reasonably unique
for ID attributes, test database unique values, etc.

Regards,

P.S. If uniqid() is a "Shouldn't use function", we don't have to care
little BC too much. Making it a "Can use function" for proper purposes
is reasonable choice. I'm +1 for deprecating and removing uniqid(),
but I presume it will never happen. That's why I'm trying to improve
it.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

[RFC][DISCUSSION] Improve uniqid() uniqueness

Regards,

-- Lester Caine - G8HFL

Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

As I wrote, it's simple bug fix.

Who argue result is reasonably unique? Who don't use NTP to adjust system time?

Who argue result is reasonably unique? Who don't use NTP to adjust system time?

What's wrong with this?

Who argue result is reasonably unique? Who don't use NTP to adjust system time?

Who argue result is reasonably unique? Who don't use NTP to adjust system time?

What's wrong with this simple fix? What makes this a special requires RFC?

Who argue result is reasonably unique? Who don't use NTP to adjust system time?

Cheers,

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?

What's wrong with this simple fix?
What makes this a special requires RFC?

Who argue result is reasonably unique?
Who don't use NTP to adjust system time?