session_regenerate_id(true) by default

11 years ago by Derick Rethans — view source

unread

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

11 years ago by Andrea Faulds — view source

unread

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/

11 years ago by Yasuo Ohgaki — view source

unread

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

11 years ago by Ferenc Kovacs — view source

unread

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

11 years ago by Patrick Schaaf — view source

unread

Am 22.10.2013 12:48 schrieb "Ferenc Kovacs" tyra3l@gmail.com:

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

11 years ago by Ferenc Kovacs — view source

unread

Am 22.10.2013 12:48 schrieb "Ferenc Kovacs" tyra3l@gmail.com:

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

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

11 years ago by Patrick Schaaf — view source

unread

Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" tyra3l@gmail.com:

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

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

11 years ago by Ferenc Kovacs — view source

unread

Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" tyra3l@gmail.com:

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

you could do @session_write_close() and error_get_last() instead of the
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.

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

a short one would be sufficient, better to be safe than sorry.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

11 years ago by Derick Rethans — view source

unread

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

11 years ago by Yasuo Ohgaki — view source

unread

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

11 years ago by Patrick Schaaf — view source

unread

Hi Yasuo,

Am 23.10.2013 02:56 schrieb "Yasuo Ohgaki" yohgaki@ohgaki.net:

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.

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

11 years ago by Yasuo Ohgaki — view source

unread

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

11 years ago by Rowan Collins — view source

unread

Ferenc Kovacs wrote (on 22/10/2013):

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.

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]

11 years ago by Yasuo Ohgaki — view source

unread

Hi Rowan,

On Wed, Oct 23, 2013 at 8:55 PM, Rowan Collins rowan.collins@gmail.comwrote:

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

11 years ago by Yasuo Ohgaki — view source

unread

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 rowan.collins@gmail.com
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

11 years ago by Christopher Jones — view source

unread

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

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

11 years ago by Yasuo Ohgaki — view source

unread

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

11 years ago by Andi Gutmans — view source

unread

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

11 years ago by Yasuo Ohgaki — view source

unread

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