# Fixing bug #68063 - Thread: https://externals.io/message/90576 - Original archive: https://news-web.php.net/php.internals/90576 - Started by Yasuo Ohgaki on 2016-01-12T22:39:16+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #90576 — Yasuo Ohgaki — 2016-01-12T22:39:16+00:00 URL: https://externals.io/message/90576#90576 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 --- ## #90578 — Stanislav Malyshev — 2016-01-12T23:03:19+00:00 URL: https://externals.io/message/90576#90578 · In reply to: #90576 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 --- ## #90584 — Yasuo Ohgaki — 2016-01-13T00:28:29+00:00 URL: https://externals.io/message/90576#90584 · In reply to: #90578 Hi Stas, > > […] > > 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. > > > […] > > 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 --- ## #90586 — Stanislav Malyshev — 2016-01-13T01:08:43+00:00 URL: https://externals.io/message/90576#90586 · In reply to: #90584 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 --- ## #90588 — Yasuo Ohgaki — 2016-01-13T02:47:50+00:00 URL: https://externals.io/message/90576#90588 · In reply to: #90586 Hi Stas, On Wed, Jan 13, 2016 at 10:08 AM, Stanislav Malyshev wrote: > > […] > > 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. > > > […] > > 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. > > > […] > > 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 --- ## #90636 — Julien Pauli — 2016-01-14T10:21:46+00:00 URL: https://externals.io/message/90576#90636 · In reply to: #90578 On Wed, Jan 13, 2016 at 12:03 AM, Stanislav Malyshev wrote: > Hi! > > > […] > > 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? > > > […] > > 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 --- ## #90637 — Yasuo Ohgaki — 2016-01-14T10:58:32+00:00 URL: https://externals.io/message/90576#90637 · In reply to: #90636 Hi Julien, > On Wed, Jan 13, 2016 at 12:03 AM, Stanislav Malyshev > wrote: > > […] > > 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 --- ## #90649 — Stanislav Malyshev — 2016-01-14T19:32:48+00:00 URL: https://externals.io/message/90576#90649 · In reply to: #90637 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 --- ## #90650 — Yasuo Ohgaki — 2016-01-15T00:10:38+00:00 URL: https://externals.io/message/90576#90650 · In reply to: #90649 Hi Stas, > > > […] > > 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 --- ## #90651 — Yasuo Ohgaki — 2016-01-15T00:32:01+00:00 URL: https://externals.io/message/90576#90651 · In reply to: #90650 Hi Julien, > > > > […] > > 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 --- ## #90659 — Julien Pauli — 2016-01-15T10:36:24+00:00 URL: https://externals.io/message/90576#90659 · In reply to: #90651 > Hi Julien, > > > > […] > > 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 --- ## #90668 — Yasuo Ohgaki — 2016-01-15T19:13:27+00:00 URL: https://externals.io/message/90576#90668 · In reply to: #90659 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