Hi Julien,
I've disallowed empty session ID, but it wasn't a
appropriate fix.
https://bugs.php.net/bug.php?id=68063
I made appropriate patch for this issue. It should be
applied from PHP 5.5 to master. I attached patch to
the bug report. Could you apply it from PHP 5.5? Or
shall I commit it from 5.6? then cherry pick?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
I've disallowed empty session ID, but it wasn't a
appropriate fix.
Could you explain a bit more about the part where there are empty IDs
generated? You say it "is browser's cookie handling" - could you explain
more about it?
I made appropriate patch for this issue. It should be
applied from PHP 5.5 to master. I attached patch to
the bug report. Could you apply it from PHP 5.5? Or
shall I commit it from 5.6? then cherry pick?
Is that a security issue? If so, please explain how. If not, it should
be 5.6+.
--
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
I've disallowed empty session ID, but it wasn't a
appropriate fix.Could you explain a bit more about the part where there are empty IDs
generated? You say it "is browser's cookie handling" - could you explain
more about it?
The root cause is browser's cookie handling.
It appears that browsers do not lock cookie while updating cookies.
Therefore race condition happens and browsers send empty cookie
sometimes. I haven't checked the code, but observed it happens.
I observed handful empty cookies a day with web site has millions
accesses per day. I circumvented this issue with method described
in https://wiki.php.net/rfc/precise_session_management
It worked perfectly.
I made appropriate patch for this issue. It should be
applied from PHP 5.5 to master. I attached patch to
the bug report. Could you apply it from PHP 5.5? Or
shall I commit it from 5.6? then cherry pick?Is that a security issue? If so, please explain how. If not, it should
be 5.6+.
Accepting empty cookie is security issue because multiple users
get the same session ID on occasion. Previous fix that disallows
empty cookie/raising error works partially, but it was inappropriate
fix. PHP shouldn't raise error for empty cookie, but should try to
set new session ID.
Although setting new session ID maintains uniqueness of session ID,
sessions may lost randomly. The method described in the RFC
is required to resolve this issue completely. Otherwise, web site users
may experience random lost session. It's very rare though.
I found simple way to observe lost sessions. Please refer to
https://bugs.php.net/bug.php?id=69127
It appears that even single threaded CLI server can cause race.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
The root cause is browser's cookie handling.
It appears that browsers do not lock cookie while updating cookies.
Therefore race condition happens and browsers send empty cookie
sometimes. I haven't checked the code, but observed it happens.I observed handful empty cookies a day with web site has millions
accesses per day. I circumvented this issue with method described
in https://wiki.php.net/rfc/precise_session_management
It worked perfectly.
OK, leaving aside open RFCs, I think we should treat empty ID cookie as
if no cookie were provided at all. Looks like that is what the patch
does? If true, we should merge it. I can do it later tonight.
Accepting empty cookie is security issue because multiple users
get the same session ID on occasion. Previous fix that disallows
empty cookie/raising error works partially, but it was inappropriate
fix. PHP shouldn't raise error for empty cookie, but should try to
set new session ID.
Since it is a browser bug and not a PHP bug, I'm not sure whether that
really qualifies... But I guess it woudn't hurt to fix that.
I found simple way to observe lost sessions. Please refer to
https://bugs.php.net/bug.php?id=69127
It appears that even single threaded CLI server can cause race.
That may be some race condition, but that does not demonstrate empty
session ID as I understand?
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
On Wed, Jan 13, 2016 at 10:08 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:
The root cause is browser's cookie handling.
It appears that browsers do not lock cookie while updating cookies.
Therefore race condition happens and browsers send empty cookie
sometimes. I haven't checked the code, but observed it happens.I observed handful empty cookies a day with web site has millions
accesses per day. I circumvented this issue with method described
in https://wiki.php.net/rfc/precise_session_management
It worked perfectly.OK, leaving aside open RFCs, I think we should treat empty ID cookie as
if no cookie were provided at all. Looks like that is what the patch
does? If true, we should merge it. I can do it later tonight.
Yes. The patch treats empty session ID cookie as no session ID cookie.
Accepting empty cookie is security issue because multiple users
get the same session ID on occasion. Previous fix that disallows
empty cookie/raising error works partially, but it was inappropriate
fix. PHP shouldn't raise error for empty cookie, but should try to
set new session ID.Since it is a browser bug and not a PHP bug, I'm not sure whether that
really qualifies... But I guess it woudn't hurt to fix that.I found simple way to observe lost sessions. Please refer to
https://bugs.php.net/bug.php?id=69127
It appears that even single threaded CLI server can cause race.That may be some race condition, but that does not demonstrate empty
session ID as I understand?
You should be able to see $_SESSION[v++] reset by empty session ID cookie.
It's been a while so it may be fixed in chrome/firefox, though.
Browser should read/write cookie values by atomic manner, but RFC does
not require to do it, AFAIK. I suppose we cannot rely on browser implementation.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
On Wed, Jan 13, 2016 at 12:03 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:
Hi!
I've disallowed empty session ID, but it wasn't a
appropriate fix.Could you explain a bit more about the part where there are empty IDs
generated? You say it "is browser's cookie handling" - could you explain
more about it?I made appropriate patch for this issue. It should be
applied from PHP 5.5 to master. I attached patch to
the bug report. Could you apply it from PHP 5.5? Or
shall I commit it from 5.6? then cherry pick?Is that a security issue? If so, please explain how. If not, it should
be 5.6+.
IMO, this is not security related.
Julien.Pauli
Hi Julien,
On Wed, Jan 13, 2016 at 12:03 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:Hi!
I've disallowed empty session ID, but it wasn't a
appropriate fix.Could you explain a bit more about the part where there are empty IDs
generated? You say it "is browser's cookie handling" - could you explain
more about it?I made appropriate patch for this issue. It should be
applied from PHP 5.5 to master. I attached patch to
the bug report. Could you apply it from PHP 5.5? Or
shall I commit it from 5.6? then cherry pick?Is that a security issue? If so, please explain how. If not, it should
be 5.6+.IMO, this is not security related.
Strictly speaking, it's not. IMO.
However, previous my fix (Raise warning and return false) was wrong fix.
Therefore, I would like to correct (Provide new session ID and continue)
it in 5.5 also. Does this make sense?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
However, previous my fix (Raise warning and return false) was wrong fix.
Therefore, I would like to correct (Provide new session ID and continue)
it in 5.5 also. Does this make sense?
Yes, but nit sure if it's for 5.5. It's for Julian to decide,
ultimately, but it doesn't look like 5.5 has a security issue right now
with it? Or am I wrong?
--
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
However, previous my fix (Raise warning and return false) was wrong fix.
Therefore, I would like to correct (Provide new session ID and continue)
it in 5.5 also. Does this make sense?Yes, but nit sure if it's for 5.5. It's for Julian to decide,
ultimately, but it doesn't look like 5.5 has a security issue right now
with it? Or am I wrong?
No. It does not have security bug, but has wrong fix for the security bug.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Julien,
However, previous my fix (Raise warning and return false) was wrong fix.
Therefore, I would like to correct (Provide new session ID and continue)
it in 5.5 also. Does this make sense?Yes, but nit sure if it's for 5.5. It's for Julian to decide,
ultimately, but it doesn't look like 5.5 has a security issue right now
with it? Or am I wrong?No. It does not have security bug, but has wrong fix for the security bug.
I'll commit the fix from PHP 5.6. If you think this should be included for
PHP 5.5, please cherry pick. I prefer to have this in PHP 5.5, but it's
not mandatory.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Julien,
However, previous my fix (Raise warning and return false) was wrong fix.
Therefore, I would like to correct (Provide new session ID and continue)
it in 5.5 also. Does this make sense?Yes, but nit sure if it's for 5.5. It's for Julian to decide,
ultimately, but it doesn't look like 5.5 has a security issue right now
with it? Or am I wrong?No. It does not have security bug, but has wrong fix for the security bug.
I'll commit the fix from PHP 5.6. If you think this should be included for
PHP 5.5, please cherry pick. I prefer to have this in PHP 5.5, but it's
not mandatory.Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
I will cherry pick it for 5.5 , as it is a fix for a security fix.
Is it bfb9307b2d679a91e138fd876880470ece60942b ?
Julien.Pauli
Hi Julien,
I will cherry pick it for 5.5 , as it is a fix for a security fix.
Thank you.
Is it bfb9307b2d679a91e138fd876880470ece60942b ?
It's 8c37a086c78a66517967fcb809fb53297becfe42
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net