Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Fixing bug #68063 »

Fixing bug #68063

php.internals

Yasuo Ohgaki

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

Stas Malyshev

10 years ago
Hi!
> I've disallowed empty session ID, but it wasn't a > appropriate fix. > > https://bugs.php.net/bug.php?id=68063
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

Yasuo Ohgaki

10 years ago
Hi Stas, On Wed, Jan 13, 2016 at 8:03 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> I've disallowed empty session ID, but it wasn't a >> appropriate fix. >> >> https://bugs.php.net/bug.php?id=68063 > > 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

Stas Malyshev

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

Yasuo Ohgaki

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

Julien Pauli

10 years ago
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. >> >> https://bugs.php.net/bug.php?id=68063 > > 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

Yasuo Ohgaki

10 years ago
Hi Julien, On Thu, Jan 14, 2016 at 7:21 PM, Julien Pauli <jpauli@php.net> wrote:
> 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. >>> >>> https://bugs.php.net/bug.php?id=68063 >> >> 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

Stas Malyshev

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

Yasuo Ohgaki

10 years ago
Hi Stas, On Fri, Jan 15, 2016 at 4:32 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> >> 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

Yasuo Ohgaki

10 years ago
Hi Julien, On Fri, Jan 15, 2016 at 9:10 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> > On Fri, Jan 15, 2016 at 4:32 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote: >> >>> 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

Julien Pauli

10 years ago
On Fri, Jan 15, 2016 at 1:32 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Julien, > > On Fri, Jan 15, 2016 at 9:10 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: >> >> On Fri, Jan 15, 2016 at 4:32 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote: >>> >>>> 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

Yasuo Ohgaki

10 years ago
Hi Julien, On Fri, Jan 15, 2016 at 7:36 PM, Julien Pauli <jpauli@php.net> wrote:
> > 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