# [RFC Discussion] Precise Session Management - Thread: https://externals.io/message/89847 - Original archive: https://news-web.php.net/php.internals/89847 - Started by Yasuo Ohgaki on 2015-12-18T22:33:10+00:00 - Messages: 39 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/89847.md?page=2 - 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 --- ## #89847 — Yasuo Ohgaki — 2015-12-18T22:33:10+00:00 URL: https://externals.io/message/89847#89847 Hi all, I would like to restart better session management for PHP 7.1. https://wiki.php.net/rfc/precise_session_management Although this RFC targets PHP 7.1, new session management could be applied to older releases also if majority of us agree. Please comment. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #89852 — Yasuo Ohgaki — 2015-12-21T01:01:47+00:00 URL: https://externals.io/message/89847#89852 · In reply to: #89847 Hi all, > I would like to restart better session management for PHP 7.1. > > https://wiki.php.net/rfc/precise_session_management > > Although this RFC targets PHP 7.1, new session management > could be applied to older releases also if majority of us agree. > Please comment. I would like to write patch for this next week. If you have comment, please comment this week. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #89853 — Mike Willbanks — 2015-12-21T04:24:24+00:00 URL: https://externals.io/message/89847#89853 · In reply to: #89852 Hello Yasuo, > Hi all, > > > > […] > > I would like to write patch for this next week. > If you have comment, please comment this week. > This week is hard due to several holidays, I would recommend postponing discussion until after. However, I will comment on a few things that I dislike of the RFC: Exposing the internal state of the session via a key on the session __SESSION_INTERNAL__ may be dangerous. How are you preventing writes to this area? Is an exception or error thrown? I also do not feel that it is worth encoding this directly into the session value but would be of far greater benefit to expose through functions and ensure it is not touched and protected from user land. Anything that messes with the $_SESSION can cause major issues (for instance upload progress did this and can cause session corruption in certain cases as it manipulates the session state). I fully agree that session_regenerate_id needs some additional work. Although. I do not think that the implementation here seems like the correct path as a general comment. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #89861 — Yasuo Ohgaki — 2015-12-22T05:37:12+00:00 URL: https://externals.io/message/89847#89861 · In reply to: #89853 Hi Mike, > > > > […] > > > This week is hard due to several holidays, I would recommend postponing > discussion until after. Thanks. I'm working on this for years. I'm not in hurry. > > > However, I will comment on a few things that I dislike of the RFC: > > Exposing the internal state of the session via a key on the session > __SESSION_INTERNAL__ may be dangerous. How are you preventing writes to > this area? Is an exception or error thrown? I also do not feel that it is The session internal data structure (__SESSION_INTERNAL__) is removed when the data is unserialized. It is added back when session data is serialized. So user cannot see/touch private session data at all. If user add $_SESSION['__SESSION_INTERNAL__'] in user script, the module remove it and raise E_WARNING. > worth encoding this directly into the session value but would be of far > greater benefit to expose through functions and ensure it is not touched and > protected from user land. Anything that messes with the $_SESSION can cause > major issues (for instance upload progress did this and can cause session > corruption in certain cases as it manipulates the session state). Please file a bug report for this. As I wrote in previously, $_SESSION['__SESSION_INTERNAL__'] is completely hidden and untouchable from users > I fully agree that session_regenerate_id needs some additional work. > Although. I do not think that the implementation here seems like the correct > path as a general comment. From user point of view, $_SESSION['__SESSION_INTERNAL__'] is a new reserved/ restricted session key. We had such restriction before "php_serialize" serialize option. e.g. $_SESSION['1'] , $_SESSION['1abc'], etc are invalid keys and raises confusing error messages. Key name must be valid as variable name for serialization method other than "php_serialize". I could choose to use completely new session serialization method like $__PSEUDO_SESSION_DATA__ = $SESSION_INTERNAL_DATA[] + $_SESSION[] where $SESSION_INTERNAL_DATA is $_SESSION['__SESSION_INTERNAL__'] and $_SESSION is the current session data array. No reserved key is required with this. However, this method breaks almost all 3rd party session data management libraries that access session data storage directly. Therefore, I chose to add and hide session internal data structure in $_SESSION. However, this could be vote option. Thank you for your comment! Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #89862 — Stanislav Malyshev — 2015-12-22T06:22:09+00:00 URL: https://externals.io/message/89847#89862 · In reply to: #89847 Hi! > I would like to restart better session management for PHP 7.1. > > https://wiki.php.net/rfc/precise_session_management I've read the RFC and I have some questions and comments: 1. I do not see why old session being active is a problem when you regenerate. You write "Attacker may abuse stolen session forever." - I'm not sure what abuse is meant. If the old session was unauthenticated, then you can not abuse it in any meaningful way. If the old session was authenticated, then a) you already lost b) regenerate is not needed, since regenerate happens when crossing security border and c) sessions can expire, so it won't be forever anyway, unless you maintain it, but even then application can easily implement limit on session length in userspace. 2. I do not see why you need to know specifically when the session is deleted. Also, I'm not sure what you mean by "invalid session". 3. "Stealing session ID is easy regardless of HTTPS" - I certainly disagree with this. Is that were easy, every application in PHP using sessions for authentication (which is close to all of them ever using authentication) would be trivially vulnerable. As I do not see any reports of such mass security failure happening, it must not be as easy as you claim. Also, "search the internet to find the proof of what I am saying" is not a good argument in an RFC. It's your argument, so it's your job to search the internet and present the proof. If it's very easy, then it should not even take long. 4. The bugs you quote are: - 69127 - unclear what is going on there, but looks like you understand what is going on, could you explain on the bug or here? - 68063 - easily fixable by rejecting empty ID< but it is a trivial matter, since practical importance of this seems to be nil - 70584 - unclear what is going on here, but doesn't seem to be related to anything discussed above, at least. Maybe caused by the new code, according to 70013 They are from 2015 and 2014, so I don't see how any of these are "known more than 10 years" - could you explain what you mean by that? 5. I don't like magic session variables too much, though if they will be implemented transparently from the user, it might be ok. 6. Session TTLs are very dependent on application nature - for some, it's ok to have sessions live nearly forever, for others, sessions may live only minutes. Also, some applications may count it from session start, others from session last use, others have more complex criteria. Trying to do one-fits-all solution is not good here. While having the date when session was initiated may be useful and would not be hurtful for most scenarios, messing with sessions - such as deleting them - when app developer does not expect it may not be a good idea. 7. Not sure I understand what ttl_update is for. If we already have last access time (which we already do, AFAIK) and session start time (which is your proposal) - why do we need anything else? 8. "Therefore, there should be GC API for cron task for instance." - I am not sure what are you meaning here. Could you describe the scenario where such function would be useful and how exactly would it be used? What people that do not have cron functionality or access to it should do? 9. "Raise error/exception for invalid access" - I do not think it is clear what is "invalid access". Does it mean access with non-existing session ID would raise an error? That would definitely break almost every application using sessions (since now they all rely on PHP treating expired session the same as if there were no session, thus triggering standard "not authenticated" workflow). I think this RFC needs a bit more work now to clarify those points. Thanks, -- Stas Malyshev smalyshev@gmail.com --- ## #89863 — Grzegorz Zdanowski — 2015-12-22T08:42:26+00:00 URL: https://externals.io/message/89847#89863 · In reply to: #89861 > > (…) > From user point of view, $_SESSION['__SESSION_INTERNAL__'] is a new reserved/ > restricted session key. > Personally I think it’s a bad way to handle such thing. Adding yet another „magic“ keyword/reserved field is going to make current situation worse. I know users should not use names starting with __, but in reality I see them almost everyday. I even seen __SESSION_INTERNAL__ used once. — Grzegorz Zdanowski --- ## #89865 — Yasuo Ohgaki — 2015-12-22T09:11:39+00:00 URL: https://externals.io/message/89847#89865 · In reply to: #89862 Hi Stas, > Hi! > > > […] > > I've read the RFC and I have some questions and comments: > > 1. I do not see why old session being active is a problem when you > regenerate. You write "Attacker may abuse stolen session forever." - I'm > not sure what abuse is meant. If the old session was unauthenticated, > then you can not abuse it in any meaningful way. If the old session was > authenticated, then a) you already lost b) regenerate is not needed, > since regenerate happens when crossing security border and c) sessions > can expire, so it won't be forever anyway, unless you maintain it, but > even then application can easily implement limit on session length in > userspace. The old session data may be stolen by suffering/XSS/etc. `session_regenerate_id()` is used to mitigate risks of stolen "valid/authenticated" session IDs. Good web apps should renew session ID periodically. Therefore, a) Session could be authenticated/valid. b) Periodic session ID renewal is needed for better security. c) Attacker may keep accessing stolen session to avoid GC. Users can implement the same feature. I've been advocating this for years, but it's not common even in Japan. > 2. I do not see why you need to know specifically when the session is > deleted. Also, I'm not sure what you mean by "invalid session". Old session ID is the invalid one. New session ID that is generated by `session_regenerate_id()` is valid one. Please note that old session ID is valid until TTL also. > 3. "Stealing session ID is easy regardless of HTTPS" - I certainly > disagree with this. Is that were easy, every application in PHP using > sessions for authentication (which is close to all of them ever using > authentication) would be trivially vulnerable. As I do not see any > reports of such mass security failure happening, it must not be as easy > as you claim. Also, "search the internet to find the proof of what I am > saying" is not a good argument in an RFC. It's your argument, so it's > your job to search the internet and present the proof. If it's very > easy, then it should not even take long. Recent browsers became stricter than before for TLS/SSL connections. It's not trivial as few years ago, but it's still possible intercept TLS/SSL connections. e.g. Enterprise network admins sniff their TLS/SSL traffic. Currently, there is no chance to know if there is stolen sessions. New session module give a chance to know possible risk by raising error for invalid/obsolete/old session accesses. BTW, if attacker such as colleague in office may directly see the cookie value of a client/browser. If web apps do not renew session ID, the colleague may abuse the session as long as session ID is valid/authenticated. > 4. The bugs you quote are: > - 69127 - unclear what is going on there, but looks like you understand > what is going on, could you explain on the bug or here? It's race in browsers. Try test code that I've posted. Browser developers might implement proper locks to avoid race, but they may not yet. Even if firefox/chrome uses proper locks, we may not rely on them as RFC does not require such cookie management. https://bugs.php.net/bug.php?id=69127 ------------------------------------------------------------------------------- [2015-09-19 23:41 UTC] yohgaki@php.net Procedure to reproduce this issue test.php -------------- "; var_dump(session_id(), $_SESSION['v']++); session_regenerate_id(true); var_dump(session_id()); ?> -------------- Start CLI server $ php -S 127.0.0.1:8888 Access test.php and press F5 few minutes http://127.0.0.1:8888/test.php You'll see counter value ($_SESSION['v']) is resetted sometimes. PHP 7.0 git + Fedora 22 + Chrome 45.0.2454.93 (64-bit) : Easy to reproduce. Thousands of requests are enough. PHP 7.0 git + Fedora 22 + Firefox 40.0.3 : Very hard to reproduce. Tens of thousands of requests are required. CLI server process requests one by one. The reason why there is difference would be how browser locks cookie data. It seems Chrome locking is more lazy or no locks at all. (BTW, even if browser locks cookie strictly, lost packet/etc could cause lost session. Therefore, "eventually consistent" approach is required for reliable HTTP session management.) Let me know if you(anyone) could reproduce the bug or not by this procedure - PHP version, OS name/version, Browser name/version and easiness/hardness of reproducibility. ------------------------------------------------------------------------------- > - 68063 - easily fixable by rejecting empty ID< but it is a trivial > matter, since practical importance of this seems to be nil https://bugs.php.net/bug.php?id=68063 The root cause of session ID being `NULL` string is browser race. When session_regenerate_id(TRUE) is called and session.use_strict_mode=1, session module may generates new session IDs (multiple IDs since browsers use multiple requests) in a short period. This causes browser race condition and results in null session ID cookie randomly/rarely. My client observed this only a few in a millions of requests. session.use_strict_mode=0 cannot be a mitigation nor resolution. Allowing use of uninitialized session will open door for abusing "unchangeable cookie". i.e. Cookie can be undeletable/unchangeable by combination of path/domain/ httponly/secure attributes. Those who need higher security for web must use session.use_strict_mode=1. Otherwise, attacker may steal active session forever by exploiting unchangeable cookie and adoptive session ID management. > - 70584 - unclear what is going on here, but doesn't seem to be related > to anything discussed above, at least. Maybe caused by the new code, > according to 70013 https://bugs.php.net/bug.php?id=70584 Since the reporter does not give feedback if he/she uses session_regenerate_id(true) or not, I cannot tell if it relates to https://bugs.php.net/bug.php?id=70013. However, if it related to 70013, "we observed the complete loss of all $_SESSION variables occurring about 1 to 10 times/day" wouldn't be possible. 70013 is a bug that reference to $_SESSION loose the reference _always_. > They are from 2015 and 2014, so I don't see how any of these are "known > more than 10 years" - could you explain what you mean by that? Deleting old session data causes problems. I thought it was due to unstable network used to be and problem wouldn't occur under stable TCP network. This discussion popped up on occasions and the first one was very long time ago. However, I cannot be sure when it was. I don't mind at all drop this phrase. It does not worthwhile researching records. And my memory could be wrong. > 5. I don't like magic session variables too much, though if they will be > implemented transparently from the user, it might be ok. Good to hear! > 6. Session TTLs are very dependent on application nature - for some, > it's ok to have sessions live nearly forever, for others, sessions may > live only minutes. Also, some applications may count it from session > start, others from session last use, others have more complex criteria. > Trying to do one-fits-all solution is not good here. While having the > date when session was initiated may be useful and would not be hurtful > for most scenarios, messing with sessions - such as deleting them - when > app developer does not expect it may not be a good idea. This proposal gives developers full control of session life time, so developers may choose proper values for their needs. I think most developers expect old session data will be deleted depending on session.gc_maxlifetime. However, this assumption is not true. e.g. attacker may keep session alive by simply accessing stolen session ID. > > 7. Not sure I understand what ttl_update is for. If we already have last > access time (which we already do, AFAIK) and session start time (which > is your proposal) - why do we need anything else? If I update TTL timestamp always, lazy_write would not work at all. I need to lower write frequency lazy_write to work. > > 8. "Therefore, there should be GC API for cron task for instance." - I > am not sure what are you meaning here. Could you describe the scenario > where such function would be useful and how exactly would it be used? > What people that do not have cron functionality or access to it should do? GC is performed at `session_start()` by probability. GC could be long and some user may experience very slow response on occasion. To avoid this, developers may set up cron task for removing obsolete sessions. For users do not have access to cron or like, they may leave probability based GC as it is now. > > 9. "Raise error/exception for invalid access" - I do not think it is > clear what is "invalid access". Does it mean access with non-existing > session ID would raise an error? That would definitely break almost > every application using sessions (since now they all rely on PHP > treating expired session the same as if there were no session, thus > triggering standard "not authenticated" workflow). > Does it mean access with non-existing session ID would raise an error? No. Access to non-existing session ID is handled by session.use_strict_mode=1 already and does not raise error at all. It creates new session ID and uses it simply. "Raise error/exception for invalid access" I mean "Raise error/exception for accessing session data that should not be accessed anymore." The invalid session is session ID that is obsoleted by `session_regenerate_id()`. Access to old/obsolete session 300 seconds after `session_regenerate_id()` is invalid in most cases. However, unstable network (e.g. wireless network) may cause "invalid access" in rare cases. Example scenario is 1. Client: Browser accesses to web site 2. Server: PHP's `session_regenerate_id()` is called. 2.1. Internal php_session_reset_id() which sends new session ID cookie is called. 2.2. PHP sends new session ID. 2.3. Network connection is lost (Subway, elevator, etc) 3. Client: Browser does not get new session ID and keep using old session ID. 4. Client: Browser sends old session ID which is obsolete in the server. This proposal allows to resend new session ID once after new session ID is sent. (60 seconds later up to old session TTL which is 300 seconds, currently) This mitigates risk of lost session and invalid session access above scenario or like. Risks of abuse increase a little in return, though. Even if this scenario is rare, it seems it happens. Thank you for your comment. I hope I explained well. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #89866 — Yasuo Ohgaki — 2015-12-22T09:22:45+00:00 URL: https://externals.io/message/89847#89866 · In reply to: #89863 Hi Grzegorz, On Tue, Dec 22, 2015 at 5:42 PM, Grzegorz Zdanowski wrote: > > […] > > Personally I think it’s a bad way to handle such thing. Adding yet another „magic“ > keyword/reserved field is going to make current situation worse. Current situation is bad enough already. I've tried to advocate proper session management including "strict session ID management" for years w/o success. i.e. session.use_strict_mode=1 I think enough time is gone by already. The same argument applies to CSRF protection. I'll add automatic and site wide CSRF protection by using internal session data structure in the future, hopefully for 7.1. > I know users should not use names starting with __, but in reality I see them > almost everyday. I even seen __SESSION_INTERNAL__ used once. Thank you for good feedback. I may use more cryptic name for it. Any suggestions? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #89876 — Yasuo Ohgaki — 2015-12-22T23:27:09+00:00 URL: https://externals.io/message/89847#89876 · In reply to: #89866 Hi all, > > > […] > > Thank you for good feedback. > I may use more cryptic name for it. > Any suggestions? Changed __SESSION_INTERNAL__ to __PHP_SESSION__. https://wiki.php.net/rfc/precise_session_management I guess most users would not use __PHP_ prefix. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90031 — Yasuo Ohgaki — 2016-01-05T01:21:16+00:00 URL: https://externals.io/message/89847#90031 · In reply to: #89847 Hi all, > I would like to restart better session management for PHP 7.1. > > https://wiki.php.net/rfc/precise_session_management > > Although this RFC targets PHP 7.1, new session management > could be applied to older releases also if majority of us agree. > Please comment. I suppose many of us back from holiday vacation. If you have any comments/questions/suggestions, I would like to hear now. Thank you. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90256 — Dan Ackroyd — 2016-01-07T15:54:16+00:00 URL: https://externals.io/message/89847#90256 · In reply to: #90031 Hi Yasuo, I find it hard to give feedback on this RFC as I cannot understand what it is saying. In an RFC, defining behaviour just through example like this: > Obsolete session data has NEW_SID and TTL upto session.ttl_destroy. > > $_SESSION['__PHP_SESSION__']['NEW_SID'] = ; > $_SESSION['__PHP_SESSION__']['TTL'] = `time()` + ini_get('session.ttl_destroy'); doesn't communicate clearly what the behaviour is going to be. There needs to be a clear description of what is going to happen. The only feedback I can give is that I think complex session behaviour need to be managed through objects or functions which can be tested inside an application. Adding complex behaviour that happens just when certain elements of a global array is set, is not the right way to add more complexity to the session management. My personal belief is that if people want to have more complex session management, they should do so in userland code. If we do want more advanced session in core, it should be done as a new extension; one that doesn't use any ini settings at all... btw this appears to be a list of the RFCs you currently have open. https://wiki.php.net/rfc/allow_url_include https://wiki.php.net/rfc/consistent-names https://wiki.php.net/rfc/consistent_function_names https://wiki.php.net/rfc/dbc2 https://wiki.php.net/rfc/deprecate_ini_set_get_aliases https://wiki.php.net/rfc/escaper https://wiki.php.net/rfc/introduce_design_by_contract https://wiki.php.net/rfc/inconsistent-behaviors https://wiki.php.net/rfc/introduce-type-affinity https://wiki.php.net/rfc/precise_float_value https://wiki.php.net/rfc/script_only_include https://wiki.php.net/rfc/secure-session-options-by-default https://wiki.php.net/rfc/session-gc Perhaps spending more time polishing one or two ideas would lead to a better result than spreading your efforts thinly across many ideas? cheers Dan --- ## #90337 — Yasuo Ohgaki — 2016-01-08T01:28:31+00:00 URL: https://externals.io/message/89847#90337 · In reply to: #90256 Hi Dan, > > I find it hard to give feedback on this RFC as I cannot understand > what it is saying. > > In an RFC, defining behaviour just through example like this: > > > […] > > doesn't communicate clearly what the behaviour is going to be. There > needs to be a clear description of what is going to happen. $_SESSION['__PHP_SESSION__']['NEW_SID'] = ; This is used to re-send new session ID as follows if (isset($_SESSION['__PHP_SESSION__']['NEW_SID'])) { // Must not update obsolete session TTL. if ($_SESSION['__PHP_SESSION__']['TTL'] - `time()` < -60 && !isset($_SESSION['__PHP_SESSION__']['NEW_SID_SENT']) { // Resend new session ID once. This will reduce chance of client race and lost session by unstable network to acceptable level. $_SESSION['__PHP_SESSION__']['NEW_SID_SENT'] = `time()`; } In English, it checks obsolete session by $_SESSION['__PHP_SESSION__']['NEW_SID'] and if it is older than 60 seconds, re-send new session ID once. The reason why this is mandatory is described in the RFC. $_SESSION['__PHP_SESSION__']['TTL'] = `time()` + ini_get('session.ttl_destroy'); This is simply TTL (Tive To Live) for the session. > The only feedback I can give is that I think complex session behaviour > need to be managed through objects or functions which can be tested > inside an application. Adding complex behaviour that happens just when > certain elements of a global array is set, is not the right way to add > more complexity to the session management. It may sound complex, but what is does is simple and decent session manager should have. The objectives are - Make sure old/obsolete session is deleted within certain period. (Currently, it does not) - Make sure session manager will not lost outstanding session. It should be basic session manager task, IMHO. Random lost session and use of obsolete session is unacceptable. Detecting lost session by unit test is impossible also. > > My personal belief is that if people want to have more complex session > management, they should do so in userland code. If we do want more > advanced session in core, it should be done as a new extension; one > that doesn't use any ini settings at all... PHP should be able to build secure/reliable web apps quick and easy. I don't think many of PHP users know/understand details of precise session management. It's better to provide precise session manager out of the box. > > btw this appears to be a list of the RFCs you currently have open. > > https://wiki.php.net/rfc/allow_url_include > https://wiki.php.net/rfc/consistent-names > https://wiki.php.net/rfc/consistent_function_names > https://wiki.php.net/rfc/dbc2 > https://wiki.php.net/rfc/deprecate_ini_set_get_aliases > https://wiki.php.net/rfc/escaper > https://wiki.php.net/rfc/introduce_design_by_contract > https://wiki.php.net/rfc/inconsistent-behaviors > https://wiki.php.net/rfc/introduce-type-affinity > https://wiki.php.net/rfc/precise_float_value > https://wiki.php.net/rfc/script_only_include > https://wiki.php.net/rfc/secure-session-options-by-default > https://wiki.php.net/rfc/session-gc > > Perhaps spending more time polishing one or two ideas would lead to a > better result than spreading your efforts thinly across many ideas? I was proposing this idea over years in fact. Only relevant RFC is session-gc which I included it into this RFC. This RFC was part of old declined RFC and I think I polished the idea generic enough. e.g. Extendable to implement automatic CSRF protection. Anyway, I don't believe all of PHP users can do the same session management even if it could be done in user script. Few people were suggested the same, it should/could be done in userland, for the old RFC, too. How many web apps/frameworks are adopted the suggested session management since the last declined RFC? https://wiki.php.net/rfc/session-lock-ini#proposal_4_-_lazy_destroy There weren't many. Therefore, providing it out of the box is worth the effort. Regards, P.S. Priority of strict_session RFC https://wiki.php.net/rfc/strict_sessions was a lot higher than this RFC as loose(adoptive) session manager allows to steal session _permanently_. Since it's implemented, this RFC became 1st priority for me. This RFC enables session.use_strict_mode by default also. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90817 — Yasuo Ohgaki — 2016-01-22T01:19:36+00:00 URL: https://externals.io/message/89847#90817 · In reply to: #89847 Hi all, > I would like to restart better session management for PHP 7.1. > > https://wiki.php.net/rfc/precise_session_management > > Although this RFC targets PHP 7.1, new session management > could be applied to older releases also if majority of us agree. > Please comment. Although it's still work in progress for https://wiki.php.net/rfc/precise_session_management I've added PR for this. https://github.com/php/php-src/pull/1734 Few things are missing still, but it's good enough to review basic features. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90818 — Yasuo Ohgaki — 2016-01-22T01:32:53+00:00 URL: https://externals.io/message/89847#90818 · In reply to: #90817 Hi all, > > https://github.com/php/php-src/pull/1734 > > Few things are missing still, but it's good enough to review basic features. Please note that if you execute run-tests.php with this patch, it causes failures on other branches/versions' session tests. To remove these test errors, you can do rm -f /tmp/sess_* rm -f /tmp/u_sess* This issue will be fixed by the time when patch is completed. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90926 — Yasuo Ohgaki — 2016-01-26T02:22:19+00:00 URL: https://externals.io/message/89847#90926 · In reply to: #90818 Hi all, > > > […] > > Please note that if you execute run-tests.php with this patch, > it causes failures on other branches/versions' session tests. > > To remove these test errors, you can do > > rm -f /tmp/sess_* > rm -f /tmp/u_sess* > > This issue will be fixed by the time when patch is completed. Other than test dependency issue, all issues and proposals are fixed/implemented. Test dependency will be fixed before merge. https://wiki.php.net/rfc/precise_session_management https://github.com/php/php-src/pull/1734 Session will be more reliable and secure with this. Please review and comment. If there is no issues, I'll start vote shortly. Thank you. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90928 — Yasuo Ohgaki — 2016-01-26T07:01:55+00:00 URL: https://externals.io/message/89847#90928 · In reply to: #90926 Hi all, > > > […] > > Other than test dependency issue, all issues and proposals are > fixed/implemented. Test dependency will be fixed before merge. > > https://wiki.php.net/rfc/precise_session_management > https://github.com/php/php-src/pull/1734 > > Session will be more reliable and secure with this. > Please review and comment. If there is no issues, I'll start > vote shortly. I didn't pay attention that current `session_id()` allows any chars for session ID while session module and save handlers explicitly disallow characters for security reasons. e.g. Session module silently removes HTML special chars, files save handler does not allow PATH special chars. Current situation is not good at all. Since this RFC is about preciseness of session management, I would like to change `session_id()` validates against default allowed chars as follows. (As well as enabling already written `session_create_id()` function) This patch is against the PR. Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler, but it should be checked always by session module. Since the function only allows chars used by ID, I would like to add "_" a valid char. "_" should be very safe char. These changes for session ID would be the largest BC of this RFC. Any comments for this change? diff --git a/ext/session/session.c b/ext/session/session.c index 7b3203a..fdc6a91 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -566,7 +566,7 @@ PHPAPI int php_session_valid_key(const char *key) /* {{{ */ /* Somewhat arbitrary length limit here, but should be way more than anyone needs and avoids file-level warnings later on if we exceed MAX_PATH */ - if (len == 0 || len > 128) { + if (len == 0 || len > PHP_SESSION_KEY_MAX_LEN) { ret = FAILURE; } @@ -2460,31 +2460,35 @@ static PHP_FUNCTION(session_save_path) Return the current session id. If newid is given, the session id is replaced with newid */ static PHP_FUNCTION(session_id) { - zend_string *name = NULL; + zend_string *id = NULL; int argc = ZEND_NUM_ARGS(); - if (zend_parse_parameters(argc, "|S", &name) == FAILURE) { + if (zend_parse_parameters(argc, "|S", &id) == FAILURE) { return; } if (PS(id)) { - /* keep compatibility for "\0" characters ??? - * see: ext/session/tests/session_id_error3.phpt */ - size_t len = strlen(ZSTR_VAL(PS(id))); - if (UNEXPECTED(len != ZSTR_LEN(PS(id)))) { - RETVAL_NEW_STR(zend_string_init(ZSTR_VAL(PS(id)), len, 0)); - } else { - RETVAL_STR_COPY(PS(id)); - } + RETVAL_STR_COPY(PS(id)); } else { RETVAL_EMPTY_STRING(); } - if (name) { + if (id) { if (PS(id)) { zend_string_release(PS(id)); } - PS(id) = zend_string_copy(name); + if (php_session_valid_key(ZSTR_VAL(id)) == FAILURE) { + zend_string *new_id; + /* Set random ID for security reason. */ + php_error_docref(NULL, E_WARNING, + "Session ID cannot exceed session max length %d " + "and/or contain special characters. Only aphanumeric, ',', '-' are allowed " + "Random session ID is set", + PHP_SESSION_KEY_MAX_LEN); + PS(id) = php_session_create_id(NULL); + } else { + PS(id) = zend_string_copy(id); + } } } /* }}} */ @@ -2519,7 +2523,6 @@ static PHP_FUNCTION(session_regenerate_id) /* {{{ proto void session_create_id([string prefix]) Generate new session ID. Intended for user save handlers. */ -#if 0 /* This is not used yet */ static PHP_FUNCTION(session_create_id) { @@ -2531,34 +2534,34 @@ static PHP_FUNCTION(session_create_id) } if (prefix && ZSTR_LEN(prefix)) { - if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) { - /* `E_ERROR` raised for security reason. */ - php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only aphanumeric, ',', '-' are allowed"); - RETURN_FALSE; + if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE + || ZSTR_LEN(prefix) > PHP_SESSION_KEY_MAX_LEN/2) { + /* Do not use prefix for security reason. */ + php_error_docref(NULL, E_WARNING, + "Prefix cannot exceed session max length %d " + "and/or contain special characters. Only aphanumeric, ',', '-' are allowed", + PHP_SESSION_KEY_MAX_LEN/2); } else { smart_str_append(&id, prefix); } } - if (PS(session_status) == php_session_active) { - new_id = PS(mod)->s_create_sid(&PS(mod_data)); - } else { - new_id = php_session_create_id(NULL); - } + /* Should call the default always to avoid problems with user handler */ + new_id = php_session_create_id(NULL); if (new_id) { smart_str_append(&id, new_id); zend_string_release(new_id); } else { smart_str_free(&id); - php_error_docref(NULL, E_WARNING, "Failed to create new ID"); + /* E_RECOVERBELE_ERROR raised for security reason. */ + php_error_docref(NULL, E_RECOVERABLE_ERROR, "Failed to create new ID"); RETURN_FALSE; } smart_str_0(&id); RETVAL_NEW_STR(id.s); smart_str_free(&id); } -#endif /* }}} */ /* {{{ proto string session_cache_limiter([string new_cache_limiter]) @@ -2901,6 +2904,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_session_id, 0, 0, 0) ZEND_ARG_INFO(0, id) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_INFO_EX(arginfo_session_create_id, 0, 0, 0) + ZEND_ARG_INFO(0, prefix) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_INFO_EX(arginfo_session_regenerate_id, 0, 0, 0) ZEND_ARG_INFO(0, delete_old_session) ZEND_END_ARG_INFO() @@ -2989,6 +2996,7 @@ static const zend_function_entry session_functions[] = { PHP_FE(session_module_name, arginfo_session_module_name) PHP_FE(session_save_path, arginfo_session_save_path) PHP_FE(session_id, arginfo_session_id) + PHP_FE(session_create_id, arginfo_session_create_id) PHP_FE(session_regenerate_id, arginfo_session_regenerate_id) PHP_FE(session_decode, arginfo_session_decode) PHP_FE(session_encode, arginfo_session_void) -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90929 — Yasuo Ohgaki — 2016-01-26T07:16:41+00:00 URL: https://externals.io/message/89847#90929 · In reply to: #90928 > Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler, > but it should be checked always by session module. Since the function > only allows Oops, sorry s/PHPAPI php_session_valid_chars()/PHPAPI php_session_valid_key()/ -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90930 — Yasuo Ohgaki — 2016-01-26T07:37:21+00:00 URL: https://externals.io/message/89847#90930 · In reply to: #90928 Hi all, > Since the function only allows chars used by ID, I would like to add "_" a > valid char. "_" should be very safe char. I think about possible attack/misuse scenario a little more and come up with following. "_" is wild card char of SQL's LIKE query. Although, it should be rare to use session ID string for LIKE query, one may do SELECT * FROM my_sess_table WHERE sess_id LIKE '$id'; where $id is '______________________'. This may allow to fetch all session IDs in DB. Users will likely write such query with prefixed session ID, so I don't think allowing "_" is not good idea after all. I'll keep as it is now, but if you have good option. Please let me know. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90931 — Stanislav Malyshev — 2016-01-26T08:01:07+00:00 URL: https://externals.io/message/89847#90931 · In reply to: #90926 Hi! > https://wiki.php.net/rfc/precise_session_management > https://github.com/php/php-src/pull/1734 I'm re-reading this RFC and I notice it does quite a lot of things: - Add five new INI values - Add two new functions - Changes behavior of two widely used functions - Changes four different defaults - Adds magic data to session This alone makes it very hard to figure out what is going on. It looks like "kitchen sink" RFC instead of targeted one. I appreciate very much your efforts to make sessions more secure, but it is very hard to review such big thing with so many things being changed at the same time. And it looks like not only myself think this way, judging by amount of feedback this is getting. Maybe I just don't understand bigger picture and all this is united by some common goal but then I'd like to have some description of this goal which will enhance my understanding. Going through RFC, here are the things that are not clear to me: 1. Why we need to change `session_regenerate_id()` ? It has session delete option already. You may argue it's not safe to not delete, but then you can opt to delete. I can even see adding option to expire session after a short period, but what is proposed looks a bit overcomplicated. 2. Why you need to keep session create and update time? IIRC we already have the time of last use (that's how GC) works, and we could have expiration timestamp for scenarios like `session_regenerate_id()` but I don't see why we need update TS in session, especially one that changes, and same for session.ttl_update. 3. Not sure what function NEW_SID serves? Why we need to know new ID in the old session? 4. Not sure how session.ttl would work. Would session expire in that time regardless of usage? Then it should be unset by default. I understand there is a use case for hard expiration, but that is separate functionality and I'm not even sure it should be in the same RFC. 5. Same for session.regenerate_id - that seems to be a piece of functionality not related to anything else. How it is connected to the rest of the RFC? In any case, I don't see why it should be enabled by default - RFC does not explain it. Same for session.num_sids and SIDS value - does not explain what is the use case for it and why it is necessary. 6. Changes like "Use strict mode by default (disallow stealing session forever). Use httponly cookie by default. Use SHA1 as hash (and use 5 bits for hashed value string for better compatibility)" are not described further and not clear how they are connected to the rest - is it necessary for the rest of the RFC to work? Why? Again, myself and I think others appreciate very much your efforts of making sessions better, but such big changes in a function that underlies almost every PHP application should be done very carefully and with full understanding of what is going on, and doing it in a huge package may be hard. I wonder maybe it'd be better to make RFC do one necessary thing - e.g. handle session regeneration - and then proceed to other things? Can we identify one problem we are trying to fix, define functionality that fixes it, agree on it, and then proceed to the next problem, etc.? Thanks, -- Stas Malyshev smalyshev@gmail.com --- ## #90932 — Stanislav Malyshev — 2016-01-26T08:15:50+00:00 URL: https://externals.io/message/89847#90932 · In reply to: #90928 Hi! > Since this RFC is about preciseness of session management, I would like to > change `session_id()` validates against default allowed chars as follows. > (As well as enabling already written `session_create_id()` function) > This patch is against the PR. I would strongly advise not to add more things into this RFC (see my other email). If you want to change which chars are allowed in session ID, fine, but let's discuss it in separate topic. However, I would proceed *very* carefully here, as there are apps that produce their own session IDs, and breaking them does not help anybody. About, since `session_id()` is a user function, what do we gain by limiting what it does? For `session_create_id()`, don't we already have SessionHandler::create_sid()? -- Stas Malyshev smalyshev@gmail.com --- ## #90933 — Yasuo Ohgaki — 2016-01-26T09:29:02+00:00 URL: https://externals.io/message/89847#90933 · In reply to: #90932 Hi Stas, > > […] > > I would strongly advise not to add more things into this RFC (see my > other email). If you want to change which chars are allowed in session > ID, fine, but let's discuss it in separate topic. Fine with me. > However, I would proceed *very* carefully here, as there are apps that > produce their own session IDs, and breaking them does not help anybody. Sounds good. As I wrote in previous mail this is going to be largest BC impact of changes I proposed. > About, since `session_id()` is a user function, what do we gain by > limiting what it does? Prefix is a part of session ID and it should have the same requirement as session ID for security reasons. > For `session_create_id()`, don't we already have > SessionHandler::create_sid()? There is SessionHandler::create_sid(), but there isn't a function that creates secure session ID. We may do sha1(random_bytes(32)), but it's better to have function that uses specified hash/data by session.hash_function/hash_bits_per_characters. So "sha1(random_bytes(32))" and "session_create_id()" is not equal, for example. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90934 — Yasuo Ohgaki — 2016-01-26T10:28:13+00:00 URL: https://externals.io/message/89847#90934 · In reply to: #90931 Hi Stas, TL;DR; for others. Those who have no idea why this RFC is mandatory and how current session management is broken, please read the RFC's TL;DR;. > > […] > > I'm re-reading this RFC and I notice it does quite a lot of things: > - Add five new INI values > - Add two new functions > - Changes behavior of two widely used functions > - Changes four different defaults > - Adds magic data to session > > This alone makes it very hard to figure out what is going on. It looks > like "kitchen sink" RFC instead of targeted one. I appreciate very much > your efforts to make sessions more secure, but it is very hard to review > such big thing with so many things being changed at the same time. And > it looks like not only myself think this way, judging by amount of > feedback this is getting. Maybe I just don't understand bigger picture > and all this is united by some common goal but then I'd like to have > some description of this goal which will enhance my understanding. > > Going through RFC, here are the things that are not clear to me: > > 1. Why we need to change `session_regenerate_id()` ? It has session delete > option already. You may argue it's not safe to not delete, but then you > can opt to delete. I can even see adding option to expire session after > a short period, but what is proposed looks a bit overcomplicated. Previous proposal was much simpler than this because it addressed only destroy duration issue. It was rejected even if it was simple. I think difficulty is not about proposal volume, but basic understanding about what is broken in current session. Many of issues that are reported could be resolved by this, but it's not complete as this proposal explains. > 2. Why you need to keep session create and update time? IIRC we already > have the time of last use (that's how GC) works, and we could have > expiration timestamp for scenarios like `session_regenerate_id()` but I > don't see why we need update TS in session, especially one that changes, > and same for session.ttl_update. CREATED is used to determine if the session should be renewed. i.e. `session_regenerate_id()` UPDATED is used to determine if the session is expired or not. "UPDATED+ttl < now" is expired. UPDATED timestamp is _not_ updated always not to disturb lazy_write. UPDATED is updated when "UPDATED + ttl_update < now" is true. For most save handlers, gc's timestamping is not precise enough for gc. gc timestamp is updated even for obsolete session also. It cannot replace UPDATED timestamp. > 3. Not sure what function NEW_SID serves? Why we need to know new ID in > the old session? Browser may not get new session ID always due to bad network, etc. e.g. Server send new session ID cookie, but connection is lost, then browser will continue to use old session cookie ID. This is one of the cause of random lost sessions. My patch try to resend new session ID once for this case. This will reduce number of lost sessions close to zero, hopefully. > > 4. Not sure how session.ttl would work. Would session expire in that > time regardless of usage? Then it should be unset by default. I > understand there is a use case for hard expiration, but that is separate > functionality and I'm not even sure it should be in the same RFC. It's not hard expiration. I think this should be done in user space. UPDATED will be updated by specified frequency(ttl_update). Session expiration is checked "UPDATED + ttl < now". > > 5. Same for session.regenerate_id - that seems to be a piece of > functionality not related to anything else. How it is connected to the > rest of the RFC? In any case, I don't see why it should be enabled by > default - RFC does not explain it. Same for session.num_sids and SIDS > value - does not explain what is the use case for it and why it is > necessary. Users must regenerate session ID for security reasons. Even if periodic session ID regeneration is mandatory for security, there are many codes that don't do this. Because it is mandatory, the feature should be part of session management. Old session IDs are needed because users may use session IDs for security protections. CSRF protection key is common usage. e.g. $csrf_key = sha1(session_id() . $mysecret) something like that. If user sets session ID regeneration for every 30 minutes, 8 session ID is good enough for 4 hours. BTW, I'm not recommending this CSRF protection method. There is better ways for it. > > 6. Changes like "Use strict mode by default (disallow stealing session > forever). Use httponly cookie by default. Use SHA1 as hash (and use 5 > bits for hashed value string for better compatibility)" are not > described further and not clear how they are connected to the rest - is > it necessary for the rest of the RFC to work? Why? This RFC target is precise (~= secure) session management. This is the reason why these are included. (As well as `session_id()` accepts any chars, but we agreed to work on this in another RFC) > > Again, myself and I think others appreciate very much your efforts of > making sessions better, but such big changes in a function that > underlies almost every PHP application should be done very carefully and > with full understanding of what is going on, and doing it in a huge > package may be hard. I wonder maybe it'd be better to make RFC do one > necessary thing - e.g. handle session regeneration - and then proceed to > other things? Can we identify one problem we are trying to fix, define > functionality that fixes it, agree on it, and then proceed to the next > problem, etc.? If you see the patch, there are many phpt changes, but code and logic is rather straight forward. I've already have/will have 3 RFC for session. This one, `session_id()` and user space serialize handler. https://github.com/php/php-src/pull/1732 I would like not to have too many RFCs for session. As you can see from patches, the more I have RFC, the more works for phpt adjustment are required _separately_. Besides writing and managing RFC, it consumes my time a lot. I'm certain that changes proposed by this RFC are mandatory or almost mandatory for better security and session stability. So I hope you feel fine with this RFC by this explanation. If you have further questions, it's welcomed. The logic is written in PHP pseudo code. I'll appreciate if one could find defect if there is. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90938 — Yasuo Ohgaki — 2016-01-26T12:17:27+00:00 URL: https://externals.io/message/89847#90938 · In reply to: #90934 Hi Stas, > I've already have/will have 3 RFC for session. > This one, `session_id()` and user space serialize handler. > https://github.com/php/php-src/pull/1732 > I would like not to have too many RFCs for session. I would like to make PHP more secure. Session is only a part of it. I have https://wiki.php.net/rfc/dbc2 https://wiki.php.net/rfc/secure_serialization https://wiki.php.net/rfc/introduce-type-affinity https://wiki.php.net/rfc/script_only_include and so on. Even there will new RFCs such as automatic CSRF protection when this RFC is finished and URL rewriter bug is fixed. I also have https://bugs.php.net/bug.php?id=68599 https://bugs.php.net/bug.php?id=55391 https://bugs.php.net/bug.php?id=68728 https://bugs.php.net/bug.php?id=69791 and so on Three RFCs for session is just too much for me already... Anyway, we may be better to talk about how it should be. For this thread, how session management should be. It's just not good enough currently. Besides exploiting PHP session is too easy, random lost sessions is not acceptable. Weak defaults are not acceptable also. Let's talk about what's missing still even with this RFC to make session secure/stable if any. Better ideas are welcomed. If there is, I'll implement it. Let's finish mandatory work and move on. Thanks, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90940 — Dan Ackroyd — 2016-01-26T15:05:46+00:00 URL: https://externals.io/message/89847#90940 · In reply to: #90938 > I'll appreciate if one could find defect if there is. The problem is that you're trying to build on a foundation of sand. The session handling works but is incredibly fragile. Or to put it more colloquially this is a "how to get to Dublin problem". To get to having a more secure and reliable session handling, we need to start from some where else, not just keep building on top of the current session handler code. The proposed RFC is not a good way to proceed. Having functionality depend on setting values in an array like this (as far a I've undertood the RFC): // Resend new session ID once. This will reduce chance of client // race and lost session by unstable network to acceptable level. $_SESSION['__PHP_SESSION__']['NEW_SID_SENT'] = `time()`; is not a sensible way of programming imo. Additionally, changing code that is apparently very fragile and so breaking people's applications in subtle ways, is not something we should be doing. > Anyway, we may be better to talk about how it should be. > For this thread, how session management should be. > It's just not good enough currently. To me, there are two good ways to proceed: i) Develop a new session extension, that doesn't depend on magic behaviour of globals and leave the current session handler as it is. The new session extension could be shipped as a 'work in progres' when it's good enough, before PHP 8. Then when it's stable, we could figure out how to transition users from the old extension. ii) Develop a session handler in userland code only. PHP is powerful enough to support this. Although obviously there are big benefits to shipping a session handler with PHP, I don't see any need for it to be done internally other than we don't currently have a way of shipping userland code with an extension. I'm hoping that before PHP 8, the ability to ship PHP code as part of extensions would be in place. Either of those two would be feasible. The proposed RFC doesn't appear feasible to me. cheers Dan --- ## #90945 — Yasuo Ohgaki — 2016-01-26T22:14:59+00:00 URL: https://externals.io/message/89847#90945 · In reply to: #90940 Hi Dan, > > […] > > The problem is that you're trying to build on a foundation of sand. > The session handling works but is incredibly fragile. > > Or to put it more colloquially this is a "how to get to Dublin > problem". To get to having a more secure and reliable session > handling, we need to start from some where else, not just keep > building on top of the current session handler code. > > The proposed RFC is not a good way to proceed. Having functionality > depend on setting values in an array like this (as far a I've > undertood the RFC): Why depending setting values (timestamp) is not good way. If you know, please let me know how to manage expiration w/o timestamp. > > // Resend new session ID once. This will reduce chance of client > // race and lost session by unstable network to acceptable level. > $_SESSION['__PHP_SESSION__']['NEW_SID_SENT'] = `time()`; > > is not a sensible way of programming imo. Additionally, changing code > that is apparently very fragile and so breaking people's applications > in subtle ways, is not something we should be doing. I agree that it is difficult for people who don't know PHP session module code. Function dependency and state management may seem like maze. However, once you understand, it's not that hard. It's only a few thousands lines. > > > […] > > To me, there are two good ways to proceed: > > i) Develop a new session extension, that doesn't depend on magic > behaviour of globals and leave the current session handler as it is. > The new session extension could be shipped as a 'work in progres' when > it's good enough, before PHP 8. Then when it's stable, we could figure > out how to transition users from the old extension. > > ii) Develop a session handler in userland code only. PHP is powerful > enough to support this. Although obviously there are big benefits to > shipping a session handler with PHP, I don't see any need for it to be > done internally other than we don't currently have a way of shipping > userland code with an extension. I'm hoping that before PHP 8, the > ability to ship PHP code as part of extensions would be in place. > > Either of those two would be feasible. The proposed RFC doesn't appear > feasible to me. > Why? it's already there and it works. You are proposing - Leave random lost session bug - Leave session exploit easy I think we must fix these. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- Next page: https://externals.io/message/89847.md?page=2