# session_regenerate_id(true) by default - Thread: https://externals.io/message/69739 - Original archive: https://news-web.php.net/php.internals/69739 - Started by Yasuo Ohgaki on 2013-10-22T06:53:58+00:00 - Messages: 20 - 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 --- ## #69739 — Yasuo Ohgaki — 2013-10-22T06:53:58+00:00 URL: https://externals.io/message/69739#69739 Hi all, Without 'true', `session_regenerate_id()` will not delete old session data which may contain sensitive data. It was made to 'false' by default for users relying on the bug. (PHP 4.x, IIRC) Almost all users should call `session_regenerate_id()` with 'true' parameter. Therefore, I would like to suggest make it 'true' by default from next PHP. Any comments? -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69746 — Derick Rethans — 2013-10-22T09:12:54+00:00 URL: https://externals.io/message/69739#69746 · In reply to: #69739 > Hi all, > > Without 'true', `session_regenerate_id()` will not delete old session data > which may contain sensitive data. It was made to 'false' by default for > users relying on the bug. (PHP 4.x, IIRC) > > Almost all users should call `session_regenerate_id()` with 'true' parameter. > Therefore, I would like to suggest make it 'true' by default from next PHP. > > Any comments? You can't just change subtle details like this. Big changes are a lot easier to manage for users, but changing defaults that have a subtle impact on already existing code are a bad idea in my book. cheers, Derick --- ## #69752 — Andrea Faulds — 2013-10-22T10:00:06+00:00 URL: https://externals.io/message/69739#69752 · In reply to: #69746 > > You can't just change subtle details like this. Big changes are a lot > easier to manage for users, but changing defaults that have a subtle > impact on already existing code are a bad idea in my book. > I agree with Derick here. People are already calling it with (true), so I don't see a problem. If the () behaviour is an issue, put a warning in the manual, perhaps. -- Andrea Faulds http://ajf.me/ --- ## #69756 — Ferenc Kovacs — 2013-10-22T10:48:23+00:00 URL: https://externals.io/message/69739#69756 · In reply to: #69739 > Hi all, > > Without 'true', `session_regenerate_id()` will not delete old session data > which may contain sensitive data. It was made to 'false' by default for > users relying on the bug. (PHP 4.x, IIRC) > > Almost all users should call `session_regenerate_id()` with 'true' parameter. > Therefore, I would like to suggest make it 'true' by default from next PHP. > > Any comments? > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > We could we add an `E_DEPRECATED` for the session_regenerate_id(false) usage for 5.6 instead. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #69759 — Patrick Schaaf — 2013-10-22T11:10:21+00:00 URL: https://externals.io/message/69739#69759 · In reply to: #69756 Am 22.10.2013 12:48 schrieb "Ferenc Kovacs" : > We could we add an `E_DEPRECATED` for the session_regenerate_id(false) usage > for 5.6 instead. I might find that useful for the `session_regenerate_id()` case, i.e. when using the default, but IMHO there are perfectly valid reasons to keep the previous session active in a controlled way. Working on the issue for our own application, I'm in the process of teaching our session wrapping class to regenerate ID often - but when doing so, first setting up the previous session ID with two pieces of information: a short timeout of 20 seconds or something like that, and a "forwarding ID" which references the new session ID. I want to do this because I want to regenerate IDs often (also based on a rather short timeout), and I'm concerned about parallel in-flight requests - a high probability reality with ajax getting more and more traction - still presenting the old session ID a second or two after a request determined to regenerate. BTW and a bit off-topic: is there a good reason for session_write_close not returning a success indicator? Right now it spams the log with a misleading message, but gives me no chance (short of setting up a global error handler to catch and handle that message) to see (and maybe retry / use a fallback) on failure best regards Patrick --- ## #69760 — Ferenc Kovacs — 2013-10-22T11:16:56+00:00 URL: https://externals.io/message/69739#69760 · In reply to: #69759 > > Am 22.10.2013 12:48 schrieb "Ferenc Kovacs" : > > > […] > usage > > […] > > I might find that useful for the `session_regenerate_id()` case, i.e. when > using the default, but IMHO there are perfectly valid reasons to keep the > previous session active in a controlled way. > > Working on the issue for our own application, I'm in the process of > teaching our session wrapping class to regenerate ID often - but when doing > so, first setting up the previous session ID with two pieces of > information: a short timeout of 20 seconds or something like that, and a > "forwarding ID" which references the new session ID. > > I want to do this because I want to regenerate IDs often (also based on a > rather short timeout), and I'm concerned about parallel in-flight requests > - a high probability reality with ajax getting more and more traction - > still presenting the old session ID a second or two after a request > determined to regenerate. > > BTW and a bit off-topic: is there a good reason for session_write_close > not returning a success indicator? Right now it spams the log with a > misleading message, but gives me no chance (short of setting up a global > error handler to catch and handle that message) to see (and maybe retry / > use a fallback) on failure > > best regards > Patrick > you could do @session_write_close() and `error_get_last()` instead of the global handler, but I think that it is a good idea and would be a trivial and backward compatible change. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #69761 — Patrick Schaaf — 2013-10-22T11:23:02+00:00 URL: https://externals.io/message/69739#69761 · In reply to: #69760 Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" : > > > > […] not returning a success indicator? Right now it spams the log with a misleading message, but gives me no chance (short of setting up a global error handler to catch and handle that message) to see (and maybe retry / use a fallback) on failure > > you could do @session_write_close() and `error_get_last()` instead of the global handler, Oh, thanks, that's less of a pain. > but I think that it is a good idea and would be a trivial and backward compatible change. Good. I'm on vacation this week, but will try to cook up a PR when I'm back next week. About time I start to work on php-src instead of only making remarks here :) Do you think this would need an RFC? best regards Patrick --- ## #69762 — Ferenc Kovacs — 2013-10-22T11:38:42+00:00 URL: https://externals.io/message/69739#69762 · In reply to: #69761 > > Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" : > > > […] > not returning a success indicator? Right now it spams the log with a > misleading message, but gives me no chance (short of setting up a global > error handler to catch and handle that message) to see (and maybe retry / > use a fallback) on failure > > […] > global handler, > > Oh, thanks, that's less of a pain. > don't forget to include NEW and the UPGRADING changes in the PR and some tests. > > […] > compatible change. > > Good. I'm on vacation this week, but will try to cook up a PR when I'm > back next week. About time I start to work on php-src instead of only > making remarks here :) Do you think this would need an RFC? > > best regards > Patrick > a short one would be sufficient, better to be safe than sorry. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #69763 — Derick Rethans — 2013-10-22T12:22:58+00:00 URL: https://externals.io/message/69739#69763 · In reply to: #69759 > BTW and a bit off-topic: is there a good reason for > session_write_close not returning a success indicator? Right now it > spams the log with a misleading message, but gives me no chance (short > of setting up a global error handler to catch and handle that message) > to see (and maybe retry / use a fallback) on failure It's likely because the utility function that throws the warning is also called during request shutdown, where you can not really show errors anymore. Logging errors is still fine at that stage. cheers, Derick --- ## #69792 — Yasuo Ohgaki — 2013-10-23T00:56:03+00:00 URL: https://externals.io/message/69739#69792 · In reply to: #69759 Hi Patrick, > Working on the issue for our own application, I'm in the process of > teaching our session wrapping class to regenerate ID often - but when doing > so, first setting up the previous session ID with two pieces of > information: a short timeout of 20 seconds or something like that, and a > "forwarding ID" which references the new session ID. > > I want to do this because I want to regenerate IDs often (also based on a > rather short timeout), and I'm concerned about parallel in-flight requests > - a high probability reality with ajax getting more and more traction - > still presenting the old session ID a second or two after a request > determined to regenerate. > Session save handlers lock session data to avoid mess, but your approach works without lock in many cases. However, it may result in inconsistent session data (i.e. over written data), so I would not recommend it as general usage. IIRC, the reason why session_regenerate_id(false) by default is compatibility for the same minor version release. We should have cleaned up this years ago. The main idea of this proposal is "Making PHP secure by default". It does not worth to keep insecure default forever because of the initial implementation had bug. IMHO. "Making PHP secure by default" also achieves "Easy to learn and use". BTW, I prefer not to raise errors for "false", since it has valid usage with save handlers allow disabling/without lock. e.g. memcached, mm save handlers. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69793 — Yasuo Ohgaki — 2013-10-23T01:06:28+00:00 URL: https://externals.io/message/69739#69793 · In reply to: #69746 Hi Derick, > You can't just change subtle details like this. Big changes are a lot > easier to manage for users, but changing defaults that have a subtle > impact on already existing code are a bad idea in my book. > We can introduce new function and make `session_regenerate_id()` depreciated few versions later. This is valid option. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69798 — Patrick Schaaf — 2013-10-23T07:35:19+00:00 URL: https://externals.io/message/69739#69798 · In reply to: #69792 Hi Yasuo, Am 23.10.2013 02:56 schrieb "Yasuo Ohgaki" : > > > > […] teaching our session wrapping class to regenerate ID often - but when doing so, first setting up the previous session ID with two pieces of information: a short timeout of 20 seconds or something like that, and a "forwarding ID" which references the new session ID. > > […] a rather short timeout), and I'm concerned about parallel in-flight requests - a high probability reality with ajax getting more and more traction - still presenting the old session ID a second or two after a request determined to regenerate. > > Session save handlers lock session data to avoid mess, but your approach works without lock > in many cases. However, it may result in inconsistent session data (i.e. over written data), > so I would not recommend it as general usage. While true for us, that is a separate issue. The parallel in-flight requests meeting an invalidated session, will happen as well with fully locked sessions - for example they will wait on the lock for the old session ID while the regenerating script is still before the regeneration point. When the lock is lifted (with regenerate(true)) they will then see an empty session, potentially affecting their operation due to missing context. Regarding non-full session locking, going that way is a conscious decision for us. We find that most of the time the session data we use is readonly, i.e. a once set up set of contextual information is used but not modified by e.g. additional ajax requests. Full locking in these cases, coupled with the occasional longer running script, results in full serialization of request processing, which translates into much increased total latency for the end user. Thus, our general approach is to shortly open the session, capturing $_SESSION to class static storage, and closing the session again. Setter methods of our helper class then maintain a dirty flag, and at the end of the script run (or explicitly called), a "flush" method examines that dirty flag, and only when dirty, it opens the session again and replaces all session variables with the full set of variables modified by that script (so no modifications of several parallel scripts are mixed together - simple last-one-wins, which makes reasoning about correctness somewhat easier). > The main idea of this proposal is "Making PHP secure by default". > It does not worth to keep insecure default forever because of the initial implementation > had bug. IMHO. I'm fully agreed on changing the default! > BTW, I prefer not to raise errors for "false", since it has valid usage Exactly. best regards Patrick --- ## #69801 — Rowan Collins — 2013-10-23T11:55:13+00:00 URL: https://externals.io/message/69739#69801 · In reply to: #69756 Ferenc Kovacs wrote (on 22/10/2013): > > > > […] > > We could we add an `E_DEPRECATED` for the session_regenerate_id(false) usage > for 5.6 instead. > Presumably what we want to deprecate is not the *ability* to pass false, but the *default* of false. So raise an `E_DEPRECATED` if you don't pass the parameter, and document that passing true will normally be the desired behaviour. Then in some future major version, remove the default value, making it an `E_ERROR` or whatever to omit it. -- Rowan Collins [IMSoP] --- ## #69820 — Yasuo Ohgaki — 2013-10-23T23:59:06+00:00 URL: https://externals.io/message/69739#69820 · In reply to: #69798 Hi Patrick, > While true for us, that is a separate issue. The parallel in-flight > requests meeting an invalidated session, will happen as well with fully > locked sessions - for example they will wait on the lock for the old > session ID while the regenerating script is still before the regeneration > point. When the lock is lifted (with regenerate(true)) they will then see > an empty session, potentially affecting their operation due to missing > context. > > Regarding non-full session locking, going that way is a conscious decision > for us. We find that most of the time the session data we use is readonly, > i.e. a once set up set of contextual information is used but not modified > by e.g. additional ajax requests. Full locking in these cases, coupled with > the occasional longer running script, results in full serialization of > request processing, which translates into much increased total latency for > the end user. > > Thus, our general approach is to shortly open the session, capturing > $_SESSION to class static storage, and closing the session again. Setter > methods of our helper class then maintain a dirty flag, and at the end of > the script run (or explicitly called), a "flush" method examines that dirty > flag, and only when dirty, it opens the session again and replaces all > session variables with the full set of variables modified by that script > (so no modifications of several parallel scripts are mixed together - > simple last-one-wins, which makes reasoning about correctness somewhat > easier). > I can make deletion of old session data at the end of request (i.e. delete old session data at RSHUTDOWN) by keeping deleted session id in a hash. Hash or any other storage is required since users can call `session_commit()`/session_write_close() and call `session_start()` again, then call session_regenerated_id()/session_destory(). Deleting session data at RSHUTDOWN would leave time read old session data for multiple requests at the same time. `session_start()` - locks session data `session_regenerate_id()` - release lock for old session data and stores id to a hash to delay deletion. RSHUTDOWN - check hash and if there is session data to be deleted, delete them. It may be good idea to have short wait time (configurable, 100 topms or so by default) and/or get lock for the deletion so that give better chance to have non-empty session data. Simple applications don't have to care much but complex apps may have race. I'll try to see if this idea works well or not. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69821 — Yasuo Ohgaki — 2013-10-24T00:05:27+00:00 URL: https://externals.io/message/69739#69821 · In reply to: #69801 Hi Rowan, On Wed, Oct 23, 2013 at 8:55 PM, Rowan Collins wrote: > So raise an `E_DEPRECATED` if you don't pass the parameter, and document > that passing true will normally be the desired behaviour. Then in some > future major version, remove the default value, making it an `E_ERROR` or > whatever to omit it. Making the parameter to required parameter works well. I like your idea! I have similar proposal for `uniqid()`'s more entropy parameter. Raising `E_DEPRECATED` works well for `uniqid()`, also. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69932 — Yasuo Ohgaki — 2013-10-29T10:44:08+00:00 URL: https://externals.io/message/69739#69932 · In reply to: #69739 Hi all, > Hi all, > > Without 'true', `session_regenerate_id()` will not delete old session data > which may contain sensitive data. It was made to 'false' by default for > users relying on the bug. (PHP 4.x, IIRC) > > Almost all users should call `session_regenerate_id()` with 'true' > parameter. Therefore, I would like to suggest make it 'true' by default > from next PHP. > > Any comments? > I've created RFC for this. https://wiki.php.net/rfc/session_regenerate_id I think Rowan's proposal is the best, so this RFC proposes to raise `E_DEPRECATED` error. On Wed, Oct 23, 2013 at 8:55 PM, Rowan Collins wrote: > So raise an `E_DEPRECATED` if you don't pass the parameter, and document > that passing true will normally be the desired behavior. Then in some > future major version, remove the default value, making it an `E_ERROR` or > whatever to omit it. If there are any more comment, I'll appreciate it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #69943 — Christopher Jones — 2013-10-29T17:14:53+00:00 URL: https://externals.io/message/69739#69943 · In reply to: #69932 > Hi all, > > > > > […] > > I've created RFC for this. > > https://wiki.php.net/rfc/session_regenerate_id Hi Yasuo, If parameter omission is an issue, I think you should update the PHP function doc ASAP and explain the problem. Most `E_DEPRECATED` messages include the word "deprecated". I think your message could be: "Calling `session_regenerate_id()` without a parameter is deprecated. Passing true is encouraged for better security" Can you review whether "false" should ever be an allowed value? The PHP doc could be improved to explain why someone might use true or false. FWIW, the message line in the RFC patch got truncated. Chris -- christopher.jones@oracle.com http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html --- ## #69954 — Yasuo Ohgaki — 2013-10-29T19:25:08+00:00 URL: https://externals.io/message/69739#69954 · In reply to: #69943 Hi Christopher, On Wed, Oct 30, 2013 at 2:14 AM, Christopher Jones < christopher.jones@oracle.com> wrote: > If parameter omission is an issue, I think you should update the PHP > function doc ASAP and explain the problem. > > Most `E_DEPRECATED` messages include the word "deprecated". I think > your message could be: > > "Calling `session_regenerate_id()` without a parameter is > deprecated. Passing true is encouraged for better security" > > Can you review whether "false" should ever be an allowed value? > > The PHP doc could be improved to explain why someone might use true or > false. > > FWIW, the message line in the RFC patch got truncated > Thank you! I'll fix them soon. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #70000 — Andi Gutmans — 2013-11-04T00:00:09+00:00 URL: https://externals.io/message/69739#70000 · In reply to: #69943 > > Hi Yasuo, > > If parameter omission is an issue, I think you should update the PHP > function doc ASAP and explain the problem. > > Most `E_DEPRECATED` messages include the word "deprecated". I think > your message could be: > > "Calling `session_regenerate_id()` without a parameter is > deprecated. Passing true is encouraged for better security" > > Can you review whether "false" should ever be an allowed value? I think we would want to continue to support false (we can check code.google.com or something to see how much it’s being used without parameters or with false). [I am not online now unfortunately]. Eliminating the default option can absolutely work as it means users need to make a conscious decision. Andi --- ## #70001 — Yasuo Ohgaki — 2013-11-04T03:32:20+00:00 URL: https://externals.io/message/69739#70001 · In reply to: #70000 Hi Andi, > On Oct 29, 2013, at 10:14 AM, Christopher Jones < > christopher.jones@oracle.com> wrote: > > > Hi Yasuo, > > If parameter omission is an issue, I think you should update the PHP > function doc ASAP and explain the problem. > > Most `E_DEPRECATED` messages include the word "deprecated". I think > your message could be: > > "Calling `session_regenerate_id()` without a parameter is > deprecated. Passing true is encouraged for better security" > > Can you review whether "false" should ever be an allowed value? > > > I think we would want to continue to support false (we can check > code.google.com or something to see how much it’s being used without > parameters or with false). [I am not online now unfortunately]. > > Eliminating the default option can absolutely work as it means users need > to make a conscious decision. > I think the option should be kept forever. I'll add race condition mitigation into session module, but it's a mitigation, not a solution. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net