Re: Solution for session_regenerate_id() issues

From: Date: Fri, 14 Mar 2014 10:37:04 +0000
Subject: Re: Solution for session_regenerate_id() issues
References: 1 2 3  Groups: php.internals 
Request: Send a blank email to internals+get-73156@lists.php.net to get a copy of this message
On 14 March 2014 09:37, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > > Hi Mateusz, > > On Fri, Mar 14, 2014 at 4:41 PM, Mateusz Kocielski <shm@digitalsun.pl>wrote: > > > I'm not sure if we should handle that in PHP, application usually > > regenerates > > session on important events (i.e. on user login/logout etc.), so any > > requests > > with old session should be denied, and this can be achieved using > > session_regenerate_id(TRUE). Wouldn't it be better to write a security > > note in the documentation rather than making whole thing more complex? > > > > The issue is that session_regenerate_id(TRUE) is unreliable. It could cause > race condition since requests from client are not synchronized. There is no > way to force synchronized access resources to clients. Out of sync issue > would be more noticeable under mobile environment, networks with poor > congestion control and/or the same web app in multiple tabs. > > Users may set timeout flag in $_SESSION array and check the value if > session could be active or not. In order to do that in user land, one may do > > > session_start() > // ** check timeout flag ** > if (!empty($_SESSION['VALID_UNTIL']) && $_SESSION['VALID_UNTIL'] > < time()) { > session_destroy(); > trigger_error('Possible session hijack attack detected'); > die('Possible session hijack attack detected'); > } > > // ** set timeout flag ** > if ($_SESSION['LAST_REGENERATE'] < time() + 600) { > $_SESSION['VALID_UNTIL'] = time() + 60; // Shorter is better, but rather > large value is set for lost radio/hand over/etc. Old session is allowed to > use as valid session for 60 seconds. > session_commit(); // Need to save above data in old session. > session_start(); > $_SESSION['LAST_REGENERATE'] = time(); // Update regenerate time here. > session_regenerate_id(); // New session ID and old session data with old > session ID is left > unset($_SESSION['VALID_UNTIL']; // This session should not be deleted > later. > } > > > Something like this should be done for reliable session_regenerate_id(). I > think not many apps do this. > > Above example is allowing 60 seconds window for legitimate user and > attacker. If session is hijacked, > - User could know attack if session ID is regenerated by attacker. > - Attacker could know there is hijack protection if session ID is > regenerated by user. > > Without code like above, both attacker and user may use the session as long > as web app allows. User has no chance to know if he/she is under session > hijack attack or not. Attacker feels safe to enjoy stolen session. > > This should be session manager task and calling session_regenerate_id() > should be enough for PHP users, IMO. > > I didn't include automatic session ID regeneration in the RFC, but it could > be handled by session manager also. It's just a matter of storing/checking > last regenerate time. If users are following security best practices, they > should renew session ID periodically even when HTTPS is used. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net For me, an arbitrary delay time is not a solution to this. The point of this (unless I misunderstand) is to prevent session hijacking with the old session ID, and simply reducing the the time window where this can happen doesn't really solve anything, as I see it. The only flow that makes sense to me (aside from simply nuking the old session data immediately when the ID is regenerated) goes as follows: - A request regenerates the session ID - The old session data is somehow marked as read-only. - All outstanding requests that the server has *already received* that are blocked waiting for a lock on the session are able to access the old data on a read-only basis. - When the last one of these requests has been processed, destroy the old session data. I realise this is likely impractical to implement in such a way that it works reliably (or even at all) with every SAPI. Hence I believe the only *practical* solution here is simply to educate users and encourage the use of session_regenerate_id(TRUE) in the documentation.

« previous php.internals (#73156) next »