FAQ
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

Search Discussions

  • Derick Rethans at Oct 22, 2013 at 9:12 am

    On Tue, 22 Oct 2013, Yasuo Ohgaki wrote:

    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
  • Andrea Faulds at Oct 22, 2013 at 10:00 am

    On 22/10/2013 10:12, Derick Rethans wrote:
    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/
  • Yasuo Ohgaki at Oct 23, 2013 at 1:07 am
    Hi Derick,
    On Tue, Oct 22, 2013 at 6:12 PM, Derick Rethans wrote:

    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
  • Ferenc Kovacs at Oct 22, 2013 at 10:48 am

    On Tue, Oct 22, 2013 at 8:53 AM, Yasuo Ohgaki wrote:

    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
  • Patrick Schaaf at Oct 22, 2013 at 11:10 am

    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
  • Ferenc Kovacs at Oct 22, 2013 at 11:16 am

    On Tue, Oct 22, 2013 at 1:10 PM, Patrick Schaaf wrote:
    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
  • Patrick Schaaf at Oct 22, 2013 at 11:23 am

    Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" <tyra3l@gmail.com>:
    On Tue, Oct 22, 2013 at 1:10 PM, Patrick Schaaf wrote:

    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
  • Ferenc Kovacs at Oct 22, 2013 at 11:38 am

    On Tue, Oct 22, 2013 at 1:23 PM, Patrick Schaaf wrote:
    Am 22.10.2013 13:16 schrieb "Ferenc Kovacs" <tyra3l@gmail.com>:
    On Tue, Oct 22, 2013 at 1:10 PM, Patrick Schaaf wrote:

    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
  • Derick Rethans at Oct 22, 2013 at 12:23 pm

    On Tue, 22 Oct 2013, Patrick Schaaf wrote:

    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
  • Yasuo Ohgaki at Oct 23, 2013 at 12:56 am
    Hi Patrick,
    On Tue, Oct 22, 2013 at 8:10 PM, Patrick Schaaf wrote:

    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
  • Patrick Schaaf at Oct 23, 2013 at 7:35 am
    Hi Yasuo,

    Am 23.10.2013 02:56 schrieb "Yasuo Ohgaki" <yohgaki@ohgaki.net>:
    On Tue, Oct 22, 2013 at 8:10 PM, Patrick Schaaf wrote:

    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
  • Yasuo Ohgaki at Oct 23, 2013 at 11:59 pm
    Hi Patrick,
    On Wed, Oct 23, 2013 at 4:35 PM, Patrick Schaaf wrote:

    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
  • Rowan Collins at Oct 23, 2013 at 11:55 am

    Ferenc Kovacs wrote (on 22/10/2013):
    On Tue, Oct 22, 2013 at 8:53 AM, Yasuo Ohgaki wrote:

    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]
  • Yasuo Ohgaki at Oct 24, 2013 at 12:06 am
    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
  • Yasuo Ohgaki at Oct 29, 2013 at 10:44 am
    Hi all,
    On Tue, Oct 22, 2013 at 3:53 PM, Yasuo Ohgaki wrote:

    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
  • Christopher Jones at Oct 29, 2013 at 5:15 pm

    On 10/29/2013 03:44 AM, Yasuo Ohgaki wrote:
    Hi all,
    On Tue, Oct 22, 2013 at 3:53 PM, Yasuo Ohgaki wrote:

    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
  • Yasuo Ohgaki at Oct 29, 2013 at 7:25 pm
    Hi Christopher,
    On Wed, Oct 30, 2013 at 2:14 AM, Christopher Jones 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
  • Andi Gutmans at Nov 4, 2013 at 1:29 am

    On Oct 29, 2013, at 10:14 AM, Christopher Jones 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.

    Andi
  • Yasuo Ohgaki at Nov 4, 2013 at 3:33 am
    Hi Andi,
    On Mon, Nov 4, 2013 at 9:00 AM, Andi Gutmans wrote:

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedOct 22, '13 at 6:54a
activeNov 4, '13 at 3:33a
posts20
users8
websitephp.net

People

Translate

site design / logo © 2022 Grokbase