Hi all,
Following commit is reverted because someone requested "RFC" and/or
demand "Merge approval from RM",
http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
I don't understand the reasoning why it requires RFC nor Merge
approval. As we all know, there are many bug fixes without RFC nor
discussion.
So let's discuss about what bug fix requires RFC and merge approval.
The patch fixes "uniqid() is not unique enough" bug.
==Rationale==
uniqid() is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg().
(large warning is added to the manual recently)
unique id (time stamp) + entropy (timestamp based entropy)
Problems of this implementation
- NTP is used to adjust system time.
- Result is not reasonably unique by logic.
==Resolution==
Replace php_combined_lcg() (Poor PRNG) to php_random_bytes() (CSPRNG).
Patch
http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
(less than 30 lines of diff)
==BC impact==
There is no BC. php_random_bytes() raises exception for bad CSPRNG,
but this very unlikely and such system shouldn't be used anyway.
Discussions in previous thread
=="new uniqid() can cause /dev/urandom cannot read error"==
This is FUD. Almost all "/dev/urandom cannot read" issues on the net
is due to "open_basedir" restriction. "open_basedir" is nothing to do
with internal functions, so the discussion is FUD.
=="Any new error is BC so the patch should be reverted"==
We do have many bug fixes emit new errors. We don't revert them at all
even when it's very common and annoying.
Example: https://bugs.php.net/bug.php?id=73238
This bug fix made WordPress display few additional E_WARNING errors
that can be remove by php.ini or code fix.
=="Someone explicitly requested RFC/Merge approval, so it should be reverted"==
IMHO, the patch is very simple patch only fixes problem. Many bug
fixes include more severe BC issues than the patch, behavior changes
and raised errors. Yet, no RFC nor discussions.
If someone requested RFC/Merge request for very simple patch, should
we follow always?
=="It does not have to hurry"==
I agree with this. However, reverting simple bug fix patch does not make sense.
Question is:
- What kind of bug fix requires RFC?
- What kind of bug fix requires discussion and approval to merge
released versions?
It does not have to be generic. It's okay for providing reasons why
this specific bug fix requires RFC or merge approval. If this bug fix
requires RFC or merge approval, almost all bug fixes requires RFC and
merge approval.
Thank you.
P.S.
The original discussion is done in "[RFC][DISCUSSION] Improve uniqid()
uniqueness" thread.
After all, my question is "Should we discuss all bugs before commits?"
"The revert is valid and reasonable?"
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Yasuo!
After all, my question is "Should we discuss all bugs before commits?"
No, I don't think that would be reasonable for time reasons. However,
if someone steps in, raising doubts about a certain bug fix, that has to
be discussed and if no consensus can be found, an RFC seems to be the
most sensible way to resolve the issue. Until the RFC has been decided
upon, (temporarily) reverting the commit also makes sense, in my
opinion. :-)
--
Christoph M. Becker
hi Yasuo,
Hi all,
Following commit is reverted because someone requested "RFC" and/or
demand "Merge approval from RM",
Yes, this is the rule for non obvious things. Or no matter what for
stable branches in security mode or RC phase.
http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
I don't understand the reasoning why it requires RFC nor Merge
approval. As we all know, there are many bug fixes without RFC nor
discussion.So let's discuss about what bug fix requires RFC and merge approval.
The patch fixes "uniqid() is not unique enough" bug.
==Rationale==
uniqid()is simply broken because it does not provide expected uniqueness due
to timestamp based php_combined_lcg().
(large warning is added to the manual recently)unique id (time stamp) + entropy (timestamp based entropy)
Problems of this implementation
- NTP is used to adjust system time.
- Result is not reasonably unique by logic.
==Resolution==
Replace php_combined_lcg() (Poor PRNG) to php_random_bytes() (CSPRNG).Patch
http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
(less than 30 lines of diff)==BC impact==
There is no BC. php_random_bytes() raises exception for bad CSPRNG,
but this very unlikely and such system shouldn't be used anyway.Discussions in previous thread
=="new
uniqid()can cause /dev/urandom cannot read error"==This is FUD. Almost all "/dev/urandom cannot read" issues on the net
is due to "open_basedir" restriction. "open_basedir" is nothing to do
with internal functions, so the discussion is FUD.=="Any new error is BC so the patch should be reverted"==
We do have many bug fixes emit new errors. We don't revert them at all
even when it's very common and annoying.Example: https://bugs.php.net/bug.php?id=73238
This bug fix made WordPress display few additionalE_WARNINGerrors
that can be remove by php.ini or code fix.=="Someone explicitly requested RFC/Merge approval, so it should be reverted"==
IMHO, the patch is very simple patch only fixes problem. Many bug
fixes include more severe BC issues than the patch, behavior changes
and raised errors. Yet, no RFC nor discussions.If someone requested RFC/Merge request for very simple patch, should
we follow always?=="It does not have to hurry"==
I agree with this. However, reverting simple bug fix patch does not make sense.
Question is:
- What kind of bug fix requires RFC?
No behavior change, strong consensus, no BC break. But really a case
by case question.
- What kind of bug fix requires discussion and approval to merge
released versions?
Same answer.
After all, my question is "Should we discuss all bugs before commits?"
"The revert is valid and reasonable?"
The main point is what is a bug, what is a BC break. And we had many
different opinions in the past. Even in this case where "this system
should not be used anyway" makes me wonder.
Like your session change, while not really critical to me (has been
like that for ages) is relatively logical while as a RM, I would not
merge it in 7.1 at this stage (RC).
Cheers,
Pierre
Hi Pierre,
Like your session change, while not really critical to me (has been
like that for ages) is relatively logical while as a RM, I would not
It's off topic, but I'll paste answer to a question regarding
session.use_strict_mode. i.e. session ID validation. I recently got
this question personally.
I have a question regarding this quite old RFC - my question is, how was it practically possible to attack a PHP app that uses sessions prior to PHP 5.5.2?
Doesn't it require the ability for the attackers to place an SID cookie in the victim's browser? How would they do that (assuming trans_sid is off)?
Possible scenarios are:
- Attacker sets SID by XSS. (Set cookies via JavaScript)
- Physical access to victim's device. (e.g. Friends, Colleague
attacker, Shared PC) - Compromised network. (WiFi access point, ARP spoofing)
When aboves are the case, attacker may set malicious cookies including
unchangeable cookies.
I think this answers to your question. Followings are additional info.
Malicious active SID is security risk even without authentication.
e.g. Shopping sites may store user info (name, address, purchased
product, etc) w/o authentication to enable shopping without user
registration. Another example is session ID has meaning. e.g. User ID
prefixed session that I'm currently proposing. Attacker may set
attacker generated SIDs that includes user ID for system's checks max
number of active session per user, and perform personal DoS attacks.
Another possible attack scenario is broken session_regenerate_id()
usage. When session_regenerate_id() is used properly, attacker cannot
gain authenticated session ID. However, if user is aware of risks of
session management w/o timestamp and try to set timestamp by
him/herself, the code could be broken by mistakes, and PHP may keep
using attacker set session ID, and attacker may gain authenticated
session ID. (Or more simpler, program didn't call
session_regenerate_id() at all, or at least call improperly)
When latter scenario is effective, attacker can set undeletable
session ID cookie, and attacker may gain authenticated session almost
forever.
To remove risks,
- Enable use_strict_session (Attackers cannot set their SIDs)
- Make
session_regenerate_id()can set programmer specified session
ID. (Need this because I can modifysession_regenerate_id()work
safely w/o knowing session management details) - Adopt timestamp management via session data (i.e. $_SESSION) so
that programs can manage session data precisely.
I hope this helps!
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
I don't understand the reasoning why it requires RFC nor Merge
approval. As we all know, there are many bug fixes without RFC nor
discussion.
If the fix is uncontroversial (everybody agrees), no RFC/approval is
needed, since there already is approval, since nobody objects, which is
by definition approval. However, if somebody does object, then we need
to go through the process of determining consensus, namely, the RFC
vote, or relying on the judgment of the person we selected to do this -
namely, the RM.
The patch fixes "uniqid() is not unique enough" bug.
I'm still not sure why this is even a bug, but I was pretty silent since
I also don't mind changing it that much.
=="Someone explicitly requested RFC/Merge approval, so it should be reverted"==
IMHO, the patch is very simple patch only fixes problem. Many bug
fixes include more severe BC issues than the patch, behavior changes
and raised errors. Yet, no RFC nor discussions.
If somebody objected, there should be discussion and RFC. If nobody
objects, what's there to discuss?
If someone requested RFC/Merge request for very simple patch, should
we follow always?
Depends on how simple. Requesting RFC on something like fixing a typo in
error message would be obvious trolling, but if there's a reasonable
objection, then we need to discuss, even if the result of the discussion
is "it's not a problem, merge in". It's not that big of a deal - whole
RFC process can be done in 2-3 weeks if the topic was previously discussed.
Question is:
- What kind of bug fix requires RFC?
One that a) makes user-visible changes, b) serious internal changes that
may substantially influence other developers or c) does not have consensus.
- What kind of bug fix requires discussion and approval to merge
released versions?
One that does not have obvious consensus or introduces BC breaks which
are borderline (i.e. can be allowed in targeted version, in theory).
After all, my question is "Should we discuss all bugs before commits?"
No, only those that are unobvious changes (fixing obvious crash or bug
or typo does not need discussion) or are controversial.
"The revert is valid and reasonable?"
If there were objections raised in discussion before merge and they were
not reconciled and merge was still performed, or if there was no
opportunity to raise objections, then yes. I didn't follow the
discussion so I don't know if it's the case here.
Stas Malyshev
smalyshev@gmail.com