Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][DISCUSSION] Improve uniqid() uniqueness »

[RFC][DISCUSSION] Improve uniqid() uniqueness

php.internals

Yasuo Ohgaki

9 years ago
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

OISHI Kazuo

9 years ago
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

Yasuo Ohgaki

9 years ago
Hi Kazuo, On Mon, Sep 12, 2016 at 1:41 PM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
> >> 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

OISHI Kazuo

9 years ago
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

Yasuo Ohgaki

9 years ago
Hi Kazuo, On Mon, Sep 12, 2016 at 7:48 PM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
>> 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

Yasuo Ohgaki

9 years ago
On Mon, Sep 12, 2016 at 9:08 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> 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

Rowan Collins

9 years ago
Hi Yasuo, uniqid() has never been, and is not claimed to be, guaranteed unique to any particular standard. On 12/09/2016 13:08, Yasuo Ohgaki wrote:
> 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]

Yasuo Ohgaki

9 years ago
Hi Rowan, On Mon, Sep 12, 2016 at 10:47 PM, Rowan Collins <rowan.collins@gmail.com> wrote:
> > 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.
> On 12/09/2016 13:08, Yasuo Ohgaki wrote: >> >> 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

Rowan Collins

9 years ago
On 13/09/2016 02:07, Yasuo Ohgaki wrote:
> 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]

Yasuo Ohgaki

9 years ago
Hi Rowan, On Tue, Sep 13, 2016 at 6:47 PM, Rowan Collins <rowan.collins@gmail.com> wrote:
> 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

OISHI Kazuo

9 years ago
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

Marco Pivetta

9 years ago
On Mon, Sep 12, 2016 at 4:46 PM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
> 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
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/

Yasuo Ohgaki

9 years ago
Hi Marco, On Mon, Sep 12, 2016 at 11:48 PM, Marco Pivetta <ocramius@gmail.com> wrote:
> > 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

Yasuo Ohgaki

9 years ago
On Tue, Sep 13, 2016 at 10:14 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>> 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

Yasuo Ohgaki

9 years ago
Hi Kazuo, On Mon, Sep 12, 2016 at 11:46 PM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
>>> 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

OISHI Kazuo

9 years ago
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

Yasuo Ohgaki

9 years ago
On Tue, Sep 13, 2016 at 11:23 AM, Kazuo Oishi <kazuo@o-ishi.jp> wrote:
>>>>> 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

Yasuo Ohgaki

9 years ago
Hi Kazuo, On Tue, Sep 13, 2016 at 11:48 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>> 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

Yasuo Ohgaki

9 years ago
Hi all, On Mon, Sep 12, 2016 at 11:54 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> 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

Marco Pivetta

9 years ago
On Sun, Oct 2, 2016 at 8:56 PM, 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. >
If you need comments on a patch, send a PR? Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Kalle Sommer Nielsen

9 years ago
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

Yasuo Ohgaki

9 years ago
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.
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

Leigh

9 years ago
On 2 October 2016 at 21:03, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> 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. > > 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php >
I'm curious, did you consider using random_int? It already handles biasing, and you can reduce the repeated calls to random_bytes.

Yasuo Ohgaki

9 years ago
Hi Leigh, On Mon, Oct 3, 2016 at 9:06 PM, Leigh <leight@gmail.com> wrote:
> 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

Leigh

9 years ago
On 4 October 2016 at 02:39, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Leigh, > > On Mon, Oct 3, 2016 at 9:06 PM, Leigh <leight@gmail.com> wrote: >> 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);

Davey

9 years ago
On Sunday, October 2, 2016, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> 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, - Davey

Yasuo Ohgaki

9 years ago
Hi Davey, On Tue, Oct 4, 2016 at 4:59 AM, Davey Shafik <davey@php.net> wrote:
> On Sunday, October 2, 2016, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: >> >> 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. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pierre Joye

9 years ago
On Oct 4, 2016 8:55 AM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:
> > Hi Davey, > > On Tue, Oct 4, 2016 at 4:59 AM, Davey Shafik <davey@php.net> wrote: > > On Sunday, October 2, 2016, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > >> > >> 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

Niklas Keller

9 years ago
Yasuo Ohgaki <yohgaki@ohgaki.net> schrieb am Di., 4. Okt. 2016, 03:54:
> Hi Davey, > > On Tue, Oct 4, 2016 at 4:59 AM, Davey Shafik <davey@php.net> wrote: > > On Sunday, October 2, 2016, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > >> > >> 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,