Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73156
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.177 as permitted sender)
MIME-Version: 1.0
Sender: are.you.winning@gmail.com
In-Reply-To: <CAGa2bXYQXgJi2jxLxR4iCKb79LwQREUHLZ0z-+D5c-8fd6P1Kw@mail.gmail.com>
References: <CAGa2bXZT8w9=usaPcJzs8NaiGfCpqvw3NVOBvTuGmSAs3vwxEQ@mail.gmail.com>
	<20140314074112.GB26909@mail>
	<CAGa2bXYQXgJi2jxLxR4iCKb79LwQREUHLZ0z-+D5c-8fd6P1Kw@mail.gmail.com>
Date: Fri, 14 Mar 2014 10:37:04 +0000
Message-ID: <CAGAGxbaCq92+-jheNeRC033S8NUrYd_3cjiwOZcgGzbgt89QJQ@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Mateusz Kocielski <shm@digitalsun.pl>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] Solution for session_regenerate_id() issues
From: daverandom@php.net (Chris Wright)

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.