php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #65746 session_regenerate_id() should not delete old session data immediately.
Submitted: 2013-09-23 22:41 UTC Modified: 2018-06-02 08:49 UTC
Votes:4
Avg. Score:3.5 ± 1.5
Reproduced:2 of 3 (66.7%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: yohgaki@php.net Assigned: yohgaki (profile)
Status: Assigned Package: Session related
PHP Version: Any OS: Any
Private report: No CVE-ID: None
Have you experienced this issue?
Rate the importance of this bug to you:

 [2013-09-23 22:41 UTC] yohgaki@php.net
Description:
------------
session_regenerate_id() do not delete old session data.
It should delete old data.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-09-23 22:41 UTC] yohgaki@php.net
-Assigned To: +Assigned To: yohgaki
 [2013-09-23 23:17 UTC] requinix@php.net
It doesn't?

bool session_regenerate_id ([ bool $delete_old_session = false ] )
 [2013-09-24 01:02 UTC] yohgaki@php.net
-Type: Bug +Type: Feature/Change Request
 [2013-09-24 01:02 UTC] yohgaki@php.net
Make this to change request :)
 [2014-03-06 15:26 UTC] narf at devilix dot net
Shouldn't this one be closed already?
 [2014-03-12 06:43 UTC] yohgaki@php.net
Unfortunately no.
To delete old session properly, old session data should be deleted after a while. This behavior is under discussion now.
 [2014-03-12 11:44 UTC] narf at devilix dot net
Huh, well ... it was last discussed 4 months ago and the RFC hasn't been updated since.
 [2015-05-24 06:21 UTC] yohgaki@php.net
Patch was there, but there are some objections for lazy destroy. I think there is no objection now.

This bug is related to 
https://bugs.php.net/bug.php?id=69127
 [2015-12-29 00:30 UTC] yohgaki@php.net
-Status: Assigned +Status: Analyzed
 [2016-08-28 21:12 UTC] yohgaki@php.net
-Summary: session_regenerate_id() do not delete old session data. +Summary: session_regenerate_id() does not delete old session data.
 [2016-10-17 06:38 UTC] yohgaki@php.net
-Summary: session_regenerate_id() does not delete old session data. +Summary: session_regenerate_id() should not delete old session data immediately.
 [2016-10-17 06:38 UTC] yohgaki@php.net
Use proper title. Original title meant "no deletion by __default__".
Last RFC is declined, but we _MUST_ fix this issue.

session_regenerate_id() depreciation is a option. We shouldn't keep security related broken function.
 [2017-10-24 03:06 UTC] kalle@php.net
-Status: Analyzed +Status: Assigned
 [2018-05-31 09:01 UTC] tony at marston-home dot demon dot co dot uk
session_regenerate_id() was never meant to destroy the session data, that is what unset($_SESSION) is for. Not even session_destroy() will delete the $_SESSION array, which means that it can be reused with the next call to session_start().

It is perfectly legitimate to open a session with one id, thus obtaining the $_SESSION array, then to use session_regenerate_id() to change the session_id() so that it can be saved under the new id. Any scripts still using the previous session_id() will still work. While both sessions start off with copies of the same $_SESSION array, these copies can quickly diverge because they are different copies being accessed with different ids.
 [2018-05-31 11:34 UTC] yohgaki@php.net
It's not about $_SESSION, but old session data stored in session storage.

Anyway, my bug description was incorrect.

bool session_regenerate_id ([ bool $delete_old_session = FALSE ] )

$delete_old_session option deletes session data from the session storage immediately, but it should eventually delete old session data. i.e. It should make old session data inaccessible after a while by keeping track expiration time stamp to avoid and detect hijacked sessions.

Immediate session data deletion will cause race condition that is very hard to debug also. e.g. Few lost sessions with tens of millions requests per day.

Current behavior and design is wrong. I created 2 RFCs to fix this design bug. I shall write 3rd RFC in the future.
 [2018-05-31 11:51 UTC] yohgaki@php.net
Precisely, there are 2 issues;

 1. session_regenerate_id() will not delete/invalidate old data.
 2. session_regenerate_id(true) deletes data immediately.

1. allows attackers to abuse hijacked sessions safely(undetected) and freely(keep it as long as they want).
2. causes race condition that results in lost sessions.
 [2018-05-31 12:03 UTC] tony at marston-home dot demon dot co dot uk
Deleting session data in the file system is what session_destroy() is for.

Use unset($_SESSION) to clear the session data in memory.

Use session_destroy() to clear the session data from the file system.

Both of these operations are completely independent of session_regenerate_id() which should not clear anything. All it is supposed to do is update the current session_id. Other operations should be handled with other function calls.

Those users who think that a single function call combines several operations are simply wrong. By changing this function to automatically include another function will cause problems for those users who do not want that other function called at all.
 [2018-06-01 10:50 UTC] yohgaki@php.net
-Operating System: +Operating System: Any -PHP Version: 5.5Git-2013-09-23 (Git) +PHP Version: Any
 [2018-06-01 10:50 UTC] yohgaki@php.net
Tony, you are ignoring the fact that there is $delete_old_session option. It exists for security reasons. 

Please stop insisting irrelevant/unrelated opinion for this bug.
 [2018-06-01 11:03 UTC] tony at marston-home dot demon dot co dot uk
I was referring to your post of [2016-10-17 06:38 UTC] in which you said "session_regenerate_id() depreciation is a option. We shouldn't keep security related broken function" which implied that the function was broken and should be deprecated.

The idea that session_regenerate_id() should do anything other than regenerate the id for the current session is totally wrong as that is handled separately by other functions.

I now notice that you have updated this function to delete the current session data as an option, which is acceptable. The idea that it should do so automatically is what I was objecting to.
 [2018-06-02 08:35 UTC] yohgaki@php.net
Your idea is against OWASP guideline, for example.

Timestamps must be managed and these timestamps must be used for precise session life cycle management. Mandatory requirements are better to be managed as default feature.

Current implementation is simply broken because it removes old session data immediately or hopes removal by GC.
 [2018-06-02 08:49 UTC] yohgaki@php.net
It's better to say "session_regenerate_id() is vulnerable" rather than "it is broken".
 
PHP Copyright © 2001-2018 The PHP Group
All rights reserved.
Last updated: Tue Jun 19 23:01:53 2018 UTC