Everything

php

[VOTE] RFC: Introduce session_start() options - read_only, unsafe_lock, lazy_write and lazy_destroy

php.internals

Yasuo Ohgaki

12 years ago

Hi all,

I almost forgot to start vote for this RFC.
This RFC is to introduce options to session_start().
Options are read_only, lazy_write, unsafe_lock and lazy_destroy.
lazy_destroy is bug fix in fact.

https://wiki.php.net/rfc/session-lock-ini
Vote: 2014/02/13 - 2014/02/20

There is no new INI. Session operations become faster and/or more reliable
in general. Exception is unsafe_lock option. It could be used at users own
risk in return of better performance. There is a little benchmark result in
the RFC to see the benefit.

The URL to the patch is mostly complete patch. I noticed there are INI
changes that should be in different RFC. I'll fix it later. If you find
something else, please let me know.

Thank you for voting!

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

12 years ago

Hi Stas,

On Thu, Feb 13, 2014 at 12:39 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I almost forgot to start vote for this RFC.
> This RFC is to introduce options to session_start().
> Options are read_only, lazy_write, unsafe_lock and lazy_destroy.
> lazy_destroy is bug fix in fact.
>

I think there is some misunderstanding. lazy_destroy is design bug fix
indeed. Without it, we cannot make sure that old sessions are deleted when
session_regenerate_id() is called, or unwanted race condition if it is
deleted. Leaving old session alive works, but it opens attack vector
unnecessarily.

I thought "deleted session data should be deleted" like you at first. You
can see that in this thread.

http://marc.info/?l=php-internals&m=138242492914526&w=2

However, conclusion is "Session data cannot be deleted immediately".
Accesses from a client are not serialized. Therefore, immediate session
data deletion results in unwanted behavior. Application that manages
personal photos is good example.

- User can only display photos when he/she is logged in.
- There are private photos in user's personal page. i.e. All photos are
protected by authenticated session.

When browser accesses to the personal page, application checks
authentication status and returns HTML page for it if user's session is
authenticated. Browser tries to load images which require authenticated
session. If session_regenerate_id() is called (timeout, etc) while loading
images, what happens? If old session data is deleted, other images cannot
be loaded because requests are done by old session ID. This scenario valid
since current browser uses multiple connections to load resources of a web
page.

Currently, session_regenerate_id() does not delete session by default. It
leaves old one. It is made not to delete old session data by default so
that it will work under such scenario.

What happens to  "should be deleted" old session data? It keeps working as
valid session until it is deleted by GC. This behavior is not acceptable.
If user's session is stolen by JavaScript injection/sniffering/etc,
attacker may abuse stolen session forever. All attacker has to do is access
web server so that GC will not delete it. session_regenerate_id() does not
help because attacker has valid authenticated session already.

The solution for this is delayed deletion when session should be deleted.
lazy_destroy (name could be changed) allows to access deleted session data
specified amount of time. No more than that. (90 sec by default).
Therefore, attackers cannot abuse stolen sessions forever and
session_regenerate_id() works as it should. i.e. Create new session and
delete old one to mitigate risk of stolen session.

By the way, session_destroy() could be made to delete session data
immediately. It is made not to delete immediately to keep consistent
destroy behavior with session_regenerate_id(). Unless user use
session_destroy() strange way, immediate deletion should not be a problem.

I hope I explained well enough to understand what is the
session_regenerate_id() bug.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Stas Malyshev

12 years ago

Hi!

> When browser accesses to the personal page, application checks
> authentication status and returns HTML page for it if user's session is
> authenticated. Browser tries to load images which require authenticated
> session. If session_regenerate_id() is called (timeout, etc) while
> loading images, what happens? If old session data is deleted, other
> images cannot be loaded because requests are done by old session ID.
> This scenario valid since current browser uses multiple connections to
> load resources of a web page.

If anything was called that makes old session invalid, any further
access to this session should result in failure. How the app does it,
does not matter really. Doing otherwise would be a huge security problem
- you removed the session, but you still can access it. Timing does not
matter - milliseconds of unauthorized access may be enough to compromise
an account. So I don't see any use in "delete, but only with delay"
option - if the data still valid, no reason to delete, if not valid -
should be deleted immediately.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Yasuo Ohgaki

12 years ago

Hi Stas,

On Fri, Feb 14, 2014 at 5:22 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> > When browser accesses to the personal page, application checks
> > authentication status and returns HTML page for it if user's session is
> > authenticated. Browser tries to load images which require authenticated
> > session. If session_regenerate_id() is called (timeout, etc) while
> > loading images, what happens? If old session data is deleted, other
> > images cannot be loaded because requests are done by old session ID.
> > This scenario valid since current browser uses multiple connections to
> > load resources of a web page.
>
> If anything was called that makes old session invalid, any further
> access to this session should result in failure. How the app does it,
> does not matter really. Doing otherwise would be a huge security problem
> - you removed the session, but you still can access it. Timing does not
> matter - milliseconds of unauthorized access may be enough to compromise
> an account. So I don't see any use in "delete, but only with delay"
> option - if the data still valid, no reason to delete, if not valid -
> should be deleted immediately.

Ideally, it is better to be deleted immediately.
Due to the limitation of web technology, it is not possible without
unwanted behaviors.

- Limitation: It is impossible to force browsers access resources one by
one.
- Unwanted behaviors: Some accesses may be denied, image/iframe/js
content/etc not accessible by vanished session.

As I explained, current work round for the limitation (keep "must be
deleted" session) allows access to attackers indefinite period of time. It
will be reduced from indefinite to seconds with this RFC.

When ideal solution cannot be chosen, we should choose better.
Allow attackers seconds or indefinite attack time?
Users may set as short as 1 second with this RFC.

Implementation/setting of delayed deletion is debatable, but choice is
obvious to me...

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Peter Cowburn

12 years ago

On 13 February 2014 03:39, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi all,
>
> I almost forgot to start vote for this RFC.
> This RFC is to introduce options to session_start().
> Options are read_only, lazy_write, unsafe_lock and lazy_destroy.
> lazy_destroy is bug fix in fact.
>
> https://wiki.php.net/rfc/session-lock-ini
> Vote: 2014/02/13 - 2014/02/20
>

Is this vote still in-progress? The RFC page says yes, but the closing date
has long-since passed.

Yasuo Ohgaki

12 years ago

Hi Peter,

On Mon, Mar 3, 2014 at 7:56 PM, Peter Cowburn <petercowburn@gmail.com>wrote:

> Is this vote still in-progress? The RFC page says yes, but the closing
> date has long-since passed.

Thank you for reminding.
Proposal 1 is passed 9 vs 1.
Proposal 2 and 3 is declined 1 vs 7 and 1 vs 6.

Lazy deletion is design bug fix. This issue cannot be solved without
delayed deletion due to technical reason of current web technology. This
also involves session security. Current implementation allows attackers to
exploit stolen session as long as they want also.
I'll come back on this issue later.

Thank you for voting all!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

12 years ago

Hi all,

On Fri, Mar 7, 2014 at 5:34 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Peter,
>
> On Mon, Mar 3, 2014 at 7:56 PM, Peter Cowburn <petercowburn@gmail.com>wrote:
>
>> Is this vote still in-progress? The RFC page says yes, but the closing
>> date has long-since passed.
>
>
> Thank you for reminding.
> Proposal 1 is passed 9 vs 1.
> Proposal 2 and 3 is declined 1 vs 7 and 1 vs 6.
>
> Lazy deletion is design bug fix. This issue cannot be solved without
> delayed deletion due to technical reason of current web technology. This
> also involves session security. Current implementation allows attackers to
> exploit stolen session as long as they want also.
> I'll come back on this issue later.
>
> Thank you for voting all!
>
>

Modified patch for this RFC is here

https://github.com/yohgaki/php-src/compare/PHP-5.6-rfc-session-lock

There may be leftover still. I'll check it again later, but it's
appreciated if you find any.

Someone asked if I'm going to allow to change all of session INIs by
session_start(),
I think it's good to have.

I would like to implement this as hash of INI options and handlers like

"option_name" => function_of_INI_modify_handler;

This way, I can iterate parameter array easily/efficiently, can change INI
values
easily/efficiently and raise appropriate errors.

Any comments for this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Sun, Mar 16, 2014 at 3:11 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Modified patch for this RFC is here
>
> https://github.com/yohgaki/php-src/compare/PHP-5.6-rfc-session-lock
>
> There may be leftover still. I'll check it again later, but it's
> appreciated if you find any.
>
> Someone asked if I'm going to allow to change all of session INIs by
> session_start(),
> I think it's good to have.
>
> I would like to implement this as hash of INI options and handlers like
>
>   "option_name" => function_of_INI_modify_handler;
>
> This way, I can iterate parameter array easily/efficiently, can change INI
> values
> easily/efficiently and raise appropriate errors.
>
> Any comments for this?
>

Sorry that the patch didn't included in PHP 5.6.
I've made patch for master since PHP 5.6 is released already.

https://wiki.php.net/rfc/session-lock-ini
https://github.com/php/php-src/pull/1016

Except comments, changes are almost minimal, but includes a few bug fixes
that tests equality of PS(session_status) against "php_session_none". The
comparison must be "PS(session_status) != php_session_active" as it has
php_session_disabled. I also removed 2 needless session globals.

Comments are appreciated.
This patch boosts PHP application performance a lot when session data
have not changed. It's faster than benchmark in the wiki because hashing
has removed.

If I don't any comment in a few days, I'll merge it to master.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Thu, Jan 22, 2015 at 2:05 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> This patch boosts PHP application performance a lot when session data
> have not changed. It's faster than benchmark in the wiki because hashing
> has removed.
>

Since it should be faster, I tried to took some benchmarks. Then I found
extremely slow
session (files handler) performance with CLI built in web server regardless
of may patch.

The script I used is

<?php
session_start();
$_SESSION['test'] = 1234;
var_dump(session_id());
?>

Fedora 21 PHP 5.6 (RPM package)
Requests per second:    5916.41 [#/sec] (mean)

Fedora 21 Master (no debug, no zts)
Requests per second:    949.56 [#/sec] (mean)

Simple script like "echo 1234;" seems working well.

Does anyone have any insight on this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Pierre Joye

11 years ago

Single query/answer at a time?
On Jan 22, 2015 1:44 PM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:

Yasuo Ohgaki

11 years ago

Hi all,

On Thu, Jan 22, 2015 at 3:42 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Since it should be faster, I tried to took some benchmarks. Then I found
> extremely slow
> session (files handler) performance with CLI built in web server
> regardless of may patch.
>
> The script I used is
>
> <?php
> session_start();
> $_SESSION['test'] = 1234;
> var_dump(session_id());
> ?>
>
> Fedora 21 PHP 5.6 (RPM package)
> Requests per second:    5916.41 [#/sec] (mean)
>
> Fedora 21 Master (no debug, no zts)
> Requests per second:    949.56 [#/sec] (mean)
>
> Simple script like "echo 1234;" seems working well.
>
> Does anyone have any insight on this?
>

I think I found what's wrong.

I've switched to btrfs recently. It seems btrfs performs very poor with
large directory entry
compared to ext4. With ext4, I think I didn't observe much performance
drop, but btrfs
drops performance a _lot_ with large directory entry. I didn't expect this.

It seems I need ext4 (or reiser4/reiserfs/xfs)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Thu, Jan 22, 2015 at 2:05 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> I've made patch for master since PHP 5.6 is released already.
>
> https://wiki.php.net/rfc/session-lock-ini
> https://github.com/php/php-src/pull/1016
>
> Except comments, changes are almost minimal, but includes a few bug fixes
> that tests equality of PS(session_status) against "php_session_none". The
> comparison must be "PS(session_status) != php_session_active" as it has
> php_session_disabled. I also removed 2 needless session globals.
>
> Comments are appreciated.
> This patch boosts PHP application performance a lot when session data
> have not changed. It's faster than benchmark in the wiki because hashing
> has removed.
>
> If I don't have any comment in a few days, I'll merge it to master.
>

I've updated UPGRADING and UPGRADING.INTERNALS and ready to merge.
I'll wait a day more. Please comment on github if you have.

Thank you.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Yasuo Ohgaki

11 years ago

Hi all,

On Mon, Jan 26, 2015 at 6:42 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> On Thu, Jan 22, 2015 at 2:05 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> I've made patch for master since PHP 5.6 is released already.
>>
>> https://wiki.php.net/rfc/session-lock-ini
>> https://github.com/php/php-src/pull/1016
>>
>> Except comments, changes are almost minimal, but includes a few bug fixes
>> that tests equality of PS(session_status) against "php_session_none". The
>> comparison must be "PS(session_status) != php_session_active" as it has
>> php_session_disabled. I also removed 2 needless session globals.
>>
>> Comments are appreciated.
>> This patch boosts PHP application performance a lot when session data
>> have not changed. It's faster than benchmark in the wiki because hashing
>> has removed.
>>
>> If I don't have any comment in a few days, I'll merge it to master.
>>
>
> I've updated UPGRADING and UPGRADING.INTERNALS and ready to merge.
> I'll wait a day more. Please comment on github if you have.
>

I've tried to get some benchmarks. It seems current system is too fast to
get obvious performance difference.

Test command: ab -c 7 -n 500000 http://localhost:8888/session.php

Test script:
<?php
ini_set('session.save_path', '/home/tmp');
ini_set('session.lazy_write', 1); // Change mode here
ini_set('session.use_strict_mode', 0);

session_id('testid');
session_start(['read_and_close'=>0]); // Change mode here
//$_SESSION['test'] = ++$_SESSION['test'];
$_SESSION['a'] = str_repeat('a', 102400);
echo '<pre>';
var_dump(session_id(), $_SESSION['test']);
?>


Old behavior was around 15000 reqs/sec.

"read_and_close" improved it to about 20000 reqs/sec. i.e 33% faster.

"lazy_write" did not improve # of reqs, but per process httpd disk writes
was reduced from 100 MB/s to 5 MB/s. i.e. There were 12 httpd processes,
1200 MB/s writes was reduced to 60 MB/s writes. Note: Linux kernel(btrfs)
does not actually write data to disk when the data is the same.

I think this would be good enough benchmark for merging.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net