Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96164
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <c90779f0-0034-b3fc-e2fe-e19503a196ce@gmail.com>
References: <CAGa2bXaX1eZS=_Xg3JjCVZRBSruMy6yUk-PTrKoGOwW5Tg=0_Q@mail.gmail.com>
 <c90779f0-0034-b3fc-e2fe-e19503a196ce@gmail.com>
Date: Tue, 27 Sep 2016 07:52:09 +0900
Message-ID: <CAGa2bXYQweCOE4G7P-rr1-VyN7Vj36NcOL9MXZaoN=xqeN1ptg@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Fixing halfway implemented session management -
 timestamp based session management OR remove session_regenerate_id()
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Stas,

On Mon, Sep 26, 2016 at 3:02 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> Timestamp based session management is required to manage session as it
>> should. I've updated the session manual pages a while a ago to explain
>> why.
>
> Could you explain what you mean here? "As it should" is kind of broad :)
>
>> http://php.net/manual/en/session.security.php
>> http://php.net/manual/en/function.session-regenerate-id.php
>
> It looks like those need some polishing. If somebody with native English
> volunteers it'd be great, otherwise I'll do it a bit later. It'd be also

Thank you. Please revise, but please do it after realizing underlying issues.

> nice to avoid claims of "mandatory" and instead just put explanations of
> what each option is doing and what is the recommended setting. Nothing
> that is controlled by an option is "mandatory" by definition.

Timestamp is mandatory to manage session precisely and securely.
Current session management is _not_ precise/secure as it does not
control session data expiration and splitted session. i.e. It does not
do anything for legitimate and malicious  user's session when there is
hijack attack.

Mandatoriness of timestamp based session management is obvious to me.
If you have question, I'll try to explain why.

>
> I also see http://php.net/manual/en/function.session-regenerate-id.php
> "warning" which I assume was added by you claims "Immediate session data
> deletion disables session hijack attack detection and prevention also."
> - I do not think this is accurate. Or at least it's not clear enough if
> I can't properly understand what it means :)

OK. I'm not sure how to improve

"Immediate session data deletion disables session hijack attack
detection and prevention also."

If session data removed immediately, not only it causes client/network
races, but also make session hijack detection impossible.

BTW, current implementation does not do anything when a session is
splitted into 2 sessions, i.e. legitimate and cracker session due to
session hijack. Both sessions work as authentic session.
session_regenerate_id() must result in only one active session
eventually.

>
>> Although session module has over 10 years of history, session module
>> lacks basic feature and is not implemented fully yet. As I mentioned
>> in above manual pages, it does not have _mandatory_ timestamp based
>> session management.
>>
>> I proposed implementation [1], but it was declined even if it is
>> mandatory for session module to manage session data correctly and
>> precisely.
>
> Obviously, other people disagree about how mandatory it is. :)

So you do not think timestamp based session management is not
mandatory. Then how do you accomplish securing session data?

 - How to prevent race conditions by network and client?
   - How to recover from lost sessions? (Network race condition cannot be fixed)
   - How to prevent race conditions in browsers? (We cannot fix browsers)
 - How to detect possible attacks?

If there is nothing we can do, we may leave problem as it is. However,
timestamp can resolve almost all issues. (My proposal attempt
reconnect session only once, so it could lose session but will be rare
compare to do nothing)

>
>> Some may think "timestamp management should be part of user task", but
>> even simple basic feature like session_regenerate_id() can NOT work as
>> it supposed without timestamp based management. (Other mandatory tasks
>> have problems also, but I ignore them for now)
>
> I was under impression it already works as it supposed to, but obviously

Sudden loss of session is not something user expect as a normal behavior.

> you are of different opinion, so I'd like to hear details about why. If
> you're talking about the case where network connection drops in the
> middle of reading the new session ID, I still think it is a very exotic
> and rare use case and claiming this function needs to be deleted because
> it doesn't cover this case makes little sense to me.

We have several bug reports for this. I personally experienced this
issue with very large web service also. It's not that rare. I observed
handful races per day for the application. I don't consider it rare
and ignorable.

>
> I've been using PHP servers with session_regenerate_id() for many years
> and never heard complaints about this. Of course, your user base may be
> different, but I still question that we need to implement a complex
> system and overhaul most of the session management to serve this rare case.

This is hard to notice, but it happens for sure. For example, session
ID being empty (this is client side race) had 17 votes.

https://bugs.php.net/bug.php?id=68063

I fixed this by rejecting and resetting empty session ID.
We have sudden session loss bug reports also. Following bug report has 8 votes.

https://bugs.php.net/bug.php?id=70584

This one cannot be fixed without timestamping. (Otherwise, use very
loose and insecure session management is required)

Session module should do better job and it can.

>
>> There is userland workaround as described. User can implement their
>> own session_regenerate_id() as described in the manual page.
>>
>> Since session management is very important feature for web apps, we
>> shouldn't keep providing halfway implemented API forever.
>> Implementation or removal is required.
>>   timestamp based (precise) session management again.
>
> What substantial changes do you propose to make since the previous RFC?
> We can't just keep voting on the same thing until we get result that you
> like, that's not how it works.

I'm proposing 2 choices now.

 - Adopting timestamp in session
 - Remove features that cannot work correctly without timestamp.

>
>>   session_regenerate_id() deprecation now and removal in future version.
>
> This makes no sense to me, at least without a lot of additional
> explanation.

Current session_regenerate_id() cannot do the optimal job for the
task. i.e. It results in empty session ID (my fix is merely a
workaround. The problem is still there), lost session ID, session
hijack attack is undetectable/cannot prevent. These issues are fatal.
IMHO. Therefore, timestamp is mandatory.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net