[RFC Discussion] Precise Session Management

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

I would like to write patch for this next week.
If you have comment, please comment this week.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Mike Willbanks — view source

unread

Hello Yasuo,

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

I would like to write patch for this next week.
If you have comment, please comment this week.

This week is hard due to several holidays, I would recommend postponing
discussion until after.

However, I will comment on a few things that I dislike of the RFC:

Exposing the internal state of the session via a key on the session
SESSION_INTERNAL may be dangerous. How are you preventing writes to
this area? Is an exception or error thrown? I also do not feel that it is
worth encoding this directly into the session value but would be of far
greater benefit to expose through functions and ensure it is not touched
and protected from user land. Anything that messes with the $_SESSION can
cause major issues (for instance upload progress did this and can cause
session corruption in certain cases as it manipulates the session state).

I fully agree that session_regenerate_id needs some additional work.
Although. I do not think that the implementation here seems like the
correct path as a general comment.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Mike,

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

I would like to write patch for this next week.
If you have comment, please comment this week.

This week is hard due to several holidays, I would recommend postponing
discussion until after.

Thanks.
I'm working on this for years. I'm not in hurry.

However, I will comment on a few things that I dislike of the RFC:

Exposing the internal state of the session via a key on the session
SESSION_INTERNAL may be dangerous. How are you preventing writes to
this area? Is an exception or error thrown? I also do not feel that it is

The session internal data structure (SESSION_INTERNAL) is removed when
the data is unserialized. It is added back when session data is serialized. So
user cannot see/touch private session data at all.

If user add $_SESSION['SESSION_INTERNAL'] in user script, the module
remove it and raise E_WARNING.

worth encoding this directly into the session value but would be of far
greater benefit to expose through functions and ensure it is not touched and
protected from user land. Anything that messes with the $_SESSION can cause
major issues (for instance upload progress did this and can cause session
corruption in certain cases as it manipulates the session state).

Please file a bug report for this.

As I wrote in previously, $_SESSION['SESSION_INTERNAL'] is completely
hidden and untouchable from users

I fully agree that session_regenerate_id needs some additional work.
Although. I do not think that the implementation here seems like the correct
path as a general comment.

From user point of view, $_SESSION['SESSION_INTERNAL'] is a new reserved/
restricted session key.

We had such restriction before "php_serialize" serialize option. e.g.
$_SESSION['1'] ,
$_SESSION['1abc'], etc are invalid keys and raises confusing error messages. Key
name must be valid as variable name for serialization method other
than "php_serialize".

I could choose to use completely new session serialization method like

$PSEUDO_SESSION_DATA = $SESSION_INTERNAL_DATA[] + $_SESSION[]
where $SESSION_INTERNAL_DATA is $_SESSION['SESSION_INTERNAL'] and
$_SESSION is the current session data array.

No reserved key is required with this. However, this method breaks
almost all 3rd party
session data management libraries that access session data storage directly.
Therefore, I chose to add and hide session internal data structure in $_SESSION.
However, this could be vote option.

Thank you for your comment!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Grzegorz Zdanowski — view source

unread

(…)
From user point of view, $_SESSION['SESSION_INTERNAL'] is a new reserved/
restricted session key.

Personally I think it’s a bad way to handle such thing. Adding yet another „magic“
keyword/reserved field is going to make current situation worse.
I know users should not use names starting with __, but in reality I see them
almost everyday. I even seen SESSION_INTERNAL used once.

—
Grzegorz Zdanowski

8 years ago by Yasuo Ohgaki — view source

unread

Hi Grzegorz,

On Tue, Dec 22, 2015 at 5:42 PM, Grzegorz Zdanowski
grzegorz129@gmail.com wrote:

(…)
From user point of view, $_SESSION['SESSION_INTERNAL'] is a new reserved/
restricted session key.

Personally I think it’s a bad way to handle such thing. Adding yet another „magic“
keyword/reserved field is going to make current situation worse.

Current situation is bad enough already. I've tried to advocate proper session
management including "strict session ID management" for years w/o success.
i.e. session.use_strict_mode=1
I think enough time is gone by already.

The same argument applies to CSRF protection. I'll add automatic and site wide
CSRF protection by using internal session data structure in the
future, hopefully
for 7.1.

I know users should not use names starting with __, but in reality I see them
almost everyday. I even seen SESSION_INTERNAL used once.

Thank you for good feedback.
I may use more cryptic name for it.
Any suggestions?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I know users should not use names starting with __, but in reality I see them
almost everyday. I even seen SESSION_INTERNAL used once.

Thank you for good feedback.
I may use more cryptic name for it.
Any suggestions?

Changed SESSION_INTERNAL to PHP_SESSION.

https://wiki.php.net/rfc/precise_session_management

I guess most users would not use _PHP prefix.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

I've read the RFC and I have some questions and comments:

I do not see why old session being active is a problem when you
regenerate. You write "Attacker may abuse stolen session forever." - I'm
not sure what abuse is meant. If the old session was unauthenticated,
then you can not abuse it in any meaningful way. If the old session was
authenticated, then a) you already lost b) regenerate is not needed,
since regenerate happens when crossing security border and c) sessions
can expire, so it won't be forever anyway, unless you maintain it, but
even then application can easily implement limit on session length in
userspace.
I do not see why you need to know specifically when the session is
deleted. Also, I'm not sure what you mean by "invalid session".
"Stealing session ID is easy regardless of HTTPS" - I certainly
disagree with this. Is that were easy, every application in PHP using
sessions for authentication (which is close to all of them ever using
authentication) would be trivially vulnerable. As I do not see any
reports of such mass security failure happening, it must not be as easy
as you claim. Also, "search the internet to find the proof of what I am
saying" is not a good argument in an RFC. It's your argument, so it's
your job to search the internet and present the proof. If it's very
easy, then it should not even take long.
The bugs you quote are:

69127 - unclear what is going on there, but looks like you understand
what is going on, could you explain on the bug or here?
68063 - easily fixable by rejecting empty ID< but it is a trivial
matter, since practical importance of this seems to be nil
70584 - unclear what is going on here, but doesn't seem to be related
to anything discussed above, at least. Maybe caused by the new code,
according to 70013

They are from 2015 and 2014, so I don't see how any of these are "known
more than 10 years" - could you explain what you mean by that?

I don't like magic session variables too much, though if they will be
implemented transparently from the user, it might be ok.
Session TTLs are very dependent on application nature - for some,
it's ok to have sessions live nearly forever, for others, sessions may
live only minutes. Also, some applications may count it from session
start, others from session last use, others have more complex criteria.
Trying to do one-fits-all solution is not good here. While having the
date when session was initiated may be useful and would not be hurtful
for most scenarios, messing with sessions - such as deleting them - when
app developer does not expect it may not be a good idea.
Not sure I understand what ttl_update is for. If we already have last
access time (which we already do, AFAIK) and session start time (which
is your proposal) - why do we need anything else?
"Therefore, there should be GC API for cron task for instance." - I
am not sure what are you meaning here. Could you describe the scenario
where such function would be useful and how exactly would it be used?
What people that do not have cron functionality or access to it should do?
"Raise error/exception for invalid access" - I do not think it is
clear what is "invalid access". Does it mean access with non-existing
session ID would raise an error? That would definitely break almost
every application using sessions (since now they all rely on PHP
treating expired session the same as if there were no session, thus
triggering standard "not authenticated" workflow).

I think this RFC needs a bit more work now to clarify those points.
Thanks,

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

Hi!

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

I've read the RFC and I have some questions and comments:

I do not see why old session being active is a problem when you
regenerate. You write "Attacker may abuse stolen session forever." - I'm
not sure what abuse is meant. If the old session was unauthenticated,
then you can not abuse it in any meaningful way. If the old session was
authenticated, then a) you already lost b) regenerate is not needed,
since regenerate happens when crossing security border and c) sessions
can expire, so it won't be forever anyway, unless you maintain it, but
even then application can easily implement limit on session length in
userspace.

The old session data may be stolen by suffering/XSS/etc.
session_regenerate_id() is used to mitigate risks of stolen
"valid/authenticated"
session IDs. Good web apps should renew session ID periodically. Therefore,

a) Session could be authenticated/valid.
b) Periodic session ID renewal is needed for better security.
c) Attacker may keep accessing stolen session to avoid GC.

Users can implement the same feature. I've been advocating this for years, but
it's not common even in Japan.

I do not see why you need to know specifically when the session is
deleted. Also, I'm not sure what you mean by "invalid session".

Old session ID is the invalid one. New session ID that is generated by
session_regenerate_id()
is valid one.

Please note that old session ID is valid until TTL also.

"Stealing session ID is easy regardless of HTTPS" - I certainly
disagree with this. Is that were easy, every application in PHP using
sessions for authentication (which is close to all of them ever using
authentication) would be trivially vulnerable. As I do not see any
reports of such mass security failure happening, it must not be as easy
as you claim. Also, "search the internet to find the proof of what I am
saying" is not a good argument in an RFC. It's your argument, so it's
your job to search the internet and present the proof. If it's very
easy, then it should not even take long.

Recent browsers became stricter than before for TLS/SSL connections.
It's not trivial as few years ago, but it's still possible intercept TLS/SSL
connections. e.g. Enterprise network admins sniff their TLS/SSL traffic.

Currently, there is no chance to know if there is stolen sessions. New
session module give a chance to know possible risk by raising error
for invalid/obsolete/old session accesses.

BTW, if attacker such as colleague in office may directly see the cookie
value of a client/browser. If web apps do not renew session ID, the colleague
may abuse the session as long as session ID is valid/authenticated.

The bugs you quote are:

69127 - unclear what is going on there, but looks like you understand
what is going on, could you explain on the bug or here?

It's race in browsers.

Try test code that I've posted. Browser developers might implement proper
locks to avoid race, but they may not yet. Even if firefox/chrome uses proper
locks, we may not rely on them as RFC does not require such cookie
management.

https://bugs.php.net/bug.php?id=69127

[2015-09-19 23:41 UTC] yohgaki@php.net

Procedure to reproduce this issue

test.php

<?php
ob_start();
session_start();
echo "<pre>";

var_dump(session_id(),
$_SESSION['v']++);
session_regenerate_id(true);
var_dump(session_id());
?>

Start CLI server
$ php -S 127.0.0.1:8888

Access test.php and press F5 few minutes
http://127.0.0.1:8888/test.php

You'll see counter value ($_SESSION['v']) is resetted sometimes.

PHP 7.0 git + Fedora 22 + Chrome 45.0.2454.93 (64-bit) : Easy to
reproduce. Thousands of requests are enough.
PHP 7.0 git + Fedora 22 + Firefox 40.0.3 : Very hard to reproduce.
Tens of thousands of requests are required.

CLI server process requests one by one. The reason why there is
difference would be how browser locks cookie data. It seems Chrome
locking is more lazy or no locks at all. (BTW, even if browser locks
cookie strictly, lost packet/etc could cause lost session. Therefore,
"eventually consistent" approach is required for reliable HTTP session
management.)

Let me know if you(anyone) could reproduce the bug or not by this
procedure - PHP version, OS name/version, Browser name/version and
easiness/hardness of reproducibility.

68063 - easily fixable by rejecting empty ID< but it is a trivial
matter, since practical importance of this seems to be nil

https://bugs.php.net/bug.php?id=68063

The root cause of session ID being NULL string is browser race.

When session_regenerate_id(TRUE) is called and session.use_strict_mode=1,
session module may generates new session IDs (multiple IDs since browsers
use multiple requests) in a short period.

This causes browser race condition and results in null session ID cookie
randomly/rarely. My client observed this only a few in a millions of requests.

session.use_strict_mode=0 cannot be a mitigation nor resolution. Allowing
use of uninitialized session will open door for abusing "unchangeable cookie".
i.e. Cookie can be undeletable/unchangeable by combination of path/domain/
httponly/secure attributes. Those who need higher security for web must use
session.use_strict_mode=1. Otherwise, attacker may steal active session
forever by exploiting unchangeable cookie and adoptive session ID management.

70584 - unclear what is going on here, but doesn't seem to be related
to anything discussed above, at least. Maybe caused by the new code,
according to 70013

https://bugs.php.net/bug.php?id=70584

Since the reporter does not give feedback if he/she uses
session_regenerate_id(true)
or not, I cannot tell if it relates to https://bugs.php.net/bug.php?id=70013.

However, if it related to 70013, "we observed the complete loss of all
$_SESSION variables occurring about 1 to 10 times/day" wouldn't be possible.
70013 is a bug that reference to $_SESSION loose the reference always.

They are from 2015 and 2014, so I don't see how any of these are "known
more than 10 years" - could you explain what you mean by that?

Deleting old session data causes problems. I thought it was due to unstable
network used to be and problem wouldn't occur under stable TCP network.
This discussion popped up on occasions and the first one was very long time
ago. However, I cannot be sure when it was. I don't mind at all drop this
phrase. It does not worthwhile researching records. And my memory could
be wrong.

I don't like magic session variables too much, though if they will be
implemented transparently from the user, it might be ok.

Good to hear!

Session TTLs are very dependent on application nature - for some,
it's ok to have sessions live nearly forever, for others, sessions may
live only minutes. Also, some applications may count it from session
start, others from session last use, others have more complex criteria.
Trying to do one-fits-all solution is not good here. While having the
date when session was initiated may be useful and would not be hurtful
for most scenarios, messing with sessions - such as deleting them - when
app developer does not expect it may not be a good idea.

This proposal gives developers full control of session life time, so developers
may choose proper values for their needs.

I think most developers expect old session data will be deleted depending
on session.gc_maxlifetime. However, this assumption is not true. e.g.
attacker may keep session alive by simply accessing stolen session ID.

Not sure I understand what ttl_update is for. If we already have last
access time (which we already do, AFAIK) and session start time (which
is your proposal) - why do we need anything else?

If I update TTL timestamp always, lazy_write would not work at all.
I need to lower write frequency lazy_write to work.

"Therefore, there should be GC API for cron task for instance." - I
am not sure what are you meaning here. Could you describe the scenario
where such function would be useful and how exactly would it be used?
What people that do not have cron functionality or access to it should do?

GC is performed at session_start() by probability. GC could be long and
some user may experience very slow response on occasion. To avoid this,
developers may set up cron task for removing obsolete sessions.

For users do not have access to cron or like, they may leave probability
based GC as it is now.

"Raise error/exception for invalid access" - I do not think it is
clear what is "invalid access". Does it mean access with non-existing
session ID would raise an error? That would definitely break almost
every application using sessions (since now they all rely on PHP
treating expired session the same as if there were no session, thus
triggering standard "not authenticated" workflow).

Does it mean access with non-existing session ID would raise an error?

No. Access to non-existing session ID is handled by session.use_strict_mode=1
already and does not raise error at all. It creates new session ID and
uses it simply.

"Raise error/exception for invalid access"

I mean "Raise error/exception for accessing session data that should not
be accessed anymore." The invalid session is session ID that is obsoleted by
session_regenerate_id(). Access to old/obsolete session 300 seconds after
session_regenerate_id() is invalid in most cases.

However, unstable network (e.g. wireless network) may cause "invalid access"
in rare cases. Example scenario is

Client: Browser accesses to web site
Server: PHP's session_regenerate_id() is called.
2.1. Internal php_session_reset_id() which sends new session ID
cookie is called.
2.2. PHP sends new session ID.
2.3. Network connection is lost (Subway, elevator, etc)
Client: Browser does not get new session ID and keep using old session ID.
Client: Browser sends old session ID which is obsolete in the server.

This proposal allows to resend new session ID once after new session ID is sent.
(60 seconds later up to old session TTL which is 300 seconds, currently) This
mitigates risk of lost session and invalid session access above
scenario or like.
Risks of abuse increase a little in return, though. Even if this
scenario is rare, it
seems it happens.

Thank you for your comment.
I hope I explained well.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

I suppose many of us back from holiday vacation.
If you have any comments/questions/suggestions, I would like to
hear now.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Dan Ackroyd — view source

unread

Hi Yasuo,

I find it hard to give feedback on this RFC as I cannot understand
what it is saying.

In an RFC, defining behaviour just through example like this:

Obsolete session data has NEW_SID and TTL upto session.ttl_destroy.

$_SESSION['PHP_SESSION']['NEW_SID'] = <new session ID>;
$_SESSION['PHP_SESSION']['TTL'] = time() + ini_get('session.ttl_destroy');

doesn't communicate clearly what the behaviour is going to be. There
needs to be a clear description of what is going to happen.

The only feedback I can give is that I think complex session behaviour
need to be managed through objects or functions which can be tested
inside an application. Adding complex behaviour that happens just when
certain elements of a global array is set, is not the right way to add
more complexity to the session management.

My personal belief is that if people want to have more complex session
management, they should do so in userland code. If we do want more
advanced session in core, it should be done as a new extension; one
that doesn't use any ini settings at all...

btw this appears to be a list of the RFCs you currently have open.

Perhaps spending more time polishing one or two ideas would lead to a
better result than spreading your efforts thinly across many ideas?

cheers
Dan

8 years ago by Yasuo Ohgaki — view source

unread

Hi Dan,

I find it hard to give feedback on this RFC as I cannot understand
what it is saying.

In an RFC, defining behaviour just through example like this:

Obsolete session data has NEW_SID and TTL upto session.ttl_destroy.

$_SESSION['PHP_SESSION']['NEW_SID'] = <new session ID>;
$_SESSION['PHP_SESSION']['TTL'] = time() + ini_get('session.ttl_destroy');

doesn't communicate clearly what the behaviour is going to be. There
needs to be a clear description of what is going to happen.

$_SESSION['PHP_SESSION']['NEW_SID'] = <new session ID>;

This is used to re-send new session ID as follows

if (isset($_SESSION['PHP_SESSION']['NEW_SID'])) {
// Must not update obsolete session TTL.
if ($_SESSION['PHP_SESSION']['TTL'] - time() < -60
&& !isset($_SESSION['PHP_SESSION']['NEW_SID_SENT']) {
// Resend new session ID once. This will reduce chance of client
race and lost session by unstable network to acceptable level.
$_SESSION['PHP_SESSION']['NEW_SID_SENT'] = time();
}

In English, it checks obsolete session by
$_SESSION['PHP_SESSION']['NEW_SID']
and if it is older than 60 seconds, re-send new session ID once. The
reason why this
is mandatory is described in the RFC.

$_SESSION['PHP_SESSION']['TTL'] = time() + ini_get('session.ttl_destroy');

This is simply TTL (Tive To Live) for the session.

The only feedback I can give is that I think complex session behaviour
need to be managed through objects or functions which can be tested
inside an application. Adding complex behaviour that happens just when
certain elements of a global array is set, is not the right way to add
more complexity to the session management.

It may sound complex, but what is does is simple and decent session
manager should have. The objectives are

Make sure old/obsolete session is deleted within certain period.
(Currently, it does not)
Make sure session manager will not lost outstanding session.

It should be basic session manager task, IMHO.

Random lost session and use of obsolete session is unacceptable.
Detecting lost session by unit test is impossible also.

My personal belief is that if people want to have more complex session
management, they should do so in userland code. If we do want more
advanced session in core, it should be done as a new extension; one
that doesn't use any ini settings at all...

PHP should be able to build secure/reliable web apps quick and easy.
I don't think many of PHP users know/understand details of precise
session management. It's better to provide precise session manager
out of the box.

btw this appears to be a list of the RFCs you currently have open.

https://wiki.php.net/rfc/allow_url_include
https://wiki.php.net/rfc/consistent-names
https://wiki.php.net/rfc/consistent_function_names
https://wiki.php.net/rfc/dbc2
https://wiki.php.net/rfc/deprecate_ini_set_get_aliases
https://wiki.php.net/rfc/escaper
https://wiki.php.net/rfc/introduce_design_by_contract
https://wiki.php.net/rfc/inconsistent-behaviors
https://wiki.php.net/rfc/introduce-type-affinity
https://wiki.php.net/rfc/precise_float_value
https://wiki.php.net/rfc/script_only_include
https://wiki.php.net/rfc/secure-session-options-by-default
https://wiki.php.net/rfc/session-gc

Perhaps spending more time polishing one or two ideas would lead to a
better result than spreading your efforts thinly across many ideas?

I was proposing this idea over years in fact.
Only relevant RFC is session-gc which I included it into this RFC. This RFC
was part of old declined RFC and I think I polished the idea generic enough.
e.g. Extendable to implement automatic CSRF protection.

Anyway, I don't believe all of PHP users can do the same session management
even if it could be done in user script. Few people were suggested the same, it
should/could be done in userland, for the old RFC, too. How many web
apps/frameworks are adopted the suggested session management since the
last declined RFC?

https://wiki.php.net/rfc/session-lock-ini#proposal_4_-_lazy_destroy

There weren't many. Therefore, providing it out of the box is worth the effort.

Regards,

P.S. Priority of strict_session RFC
https://wiki.php.net/rfc/strict_sessions
was a lot higher than this RFC as loose(adoptive) session manager allows
to steal session permanently. Since it's implemented, this RFC became
1st priority for me. This RFC enables session.use_strict_mode by default
also.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

Although it's still work in progress for
https://wiki.php.net/rfc/precise_session_management
I've added PR for this.

https://github.com/php/php-src/pull/1734

Few things are missing still, but it's good enough to review basic features.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

https://github.com/php/php-src/pull/1734

Few things are missing still, but it's good enough to review basic features.

Please note that if you execute run-tests.php with this patch,
it causes failures on other branches/versions' session tests.

To remove these test errors, you can do

rm -f /tmp/sess_*
rm -f /tmp/u_sess*

This issue will be fixed by the time when patch is completed.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

https://github.com/php/php-src/pull/1734

Few things are missing still, but it's good enough to review basic features.

Please note that if you execute run-tests.php with this patch,
it causes failures on other branches/versions' session tests.

To remove these test errors, you can do

rm -f /tmp/sess_*
rm -f /tmp/u_sess*

This issue will be fixed by the time when patch is completed.

Other than test dependency issue, all issues and proposals are
fixed/implemented. Test dependency will be fixed before merge.

https://wiki.php.net/rfc/precise_session_management
https://github.com/php/php-src/pull/1734

Session will be more reliable and secure with this.
Please review and comment. If there is no issues, I'll start
vote shortly.

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

https://github.com/php/php-src/pull/1734

Few things are missing still, but it's good enough to review basic features.

Please note that if you execute run-tests.php with this patch,
it causes failures on other branches/versions' session tests.

To remove these test errors, you can do

rm -f /tmp/sess_*
rm -f /tmp/u_sess*

This issue will be fixed by the time when patch is completed.

Other than test dependency issue, all issues and proposals are
fixed/implemented. Test dependency will be fixed before merge.

https://wiki.php.net/rfc/precise_session_management
https://github.com/php/php-src/pull/1734

Session will be more reliable and secure with this.
Please review and comment. If there is no issues, I'll start
vote shortly.

I didn't pay attention that current session_id() allows any chars for session ID
while session module and save handlers explicitly disallow characters for
security reasons. e.g. Session module silently removes HTML special chars,
files save handler does not allow PATH special chars.
Current situation is not good at all.

Since this RFC is about preciseness of session management, I would like to
change session_id() validates against default allowed chars as follows.
(As well as enabling already written session_create_id() function)
This patch is against the PR.

Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler,
but it should be checked always by session module. Since the function
only allows
chars used by ID, I would like to add "" a valid char. "" should be
very safe char.

These changes for session ID would be the largest BC of this RFC.
Any comments for this change?

diff --git a/ext/session/session.c b/ext/session/session.c
index 7b3203a..fdc6a91 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -566,7 +566,7 @@ PHPAPI int php_session_valid_key(const char key) / {{{ */

 /* Somewhat arbitrary length limit here, but should be way more than
    anyone needs and avoids file-level warnings later on if we

exceed MAX_PATH */

if (len == 0 || len > 128) {

if (len == 0 || len > PHP_SESSION_KEY_MAX_LEN) {
ret = FAILURE;
}

@@ -2460,31 +2460,35 @@ static PHP_FUNCTION(session_save_path)
Return the current session id. If newid is given, the session id
is replaced with newid */
static PHP_FUNCTION(session_id)
{

zend_string *name = NULL;

zend_string *id = NULL;
int argc = ZEND_NUM_ARGS();

if (zend_parse_parameters(argc, "|S", &name) == FAILURE) {

if (zend_parse_parameters(argc, "|S", &id) == FAILURE) {
return;
}

if (PS(id)) {

   /* keep compatibility for "\0" characters ???

    * see: ext/session/tests/session_id_error3.phpt */

   size_t len = strlen(ZSTR_VAL(PS(id)));

   if (UNEXPECTED(len != ZSTR_LEN(PS(id)))) {

       RETVAL_NEW_STR(zend_string_init(ZSTR_VAL(PS(id)), len, 0));

```
   } else {
```
```
       RETVAL_STR_COPY(PS(id));
```
```
   }
```

```
   RETVAL_STR_COPY(PS(id));
```
} else {
RETVAL_EMPTY_STRING();
}

if (name) {

if (id) {
if (PS(id)) {
zend_string_release(PS(id));
}

```
   PS(id) = zend_string_copy(name);
```

   if (php_session_valid_key(ZSTR_VAL(id)) == FAILURE) {

```
       zend_string *new_id;
```

       /* Set random ID for security reason. */

       php_error_docref(NULL, E_WARNING,

                        "Session ID cannot exceed session max length %d "

                        "and/or contain special characters. Only

aphanumeric, ',', '-' are allowed "

                        "Random session ID is set",

                        PHP_SESSION_KEY_MAX_LEN);

       PS(id) = php_session_create_id(NULL);

```
   } else {
```
```
       PS(id) = zend_string_copy(id);
```
```
   }
```
}
}
/* }}} */
@@ -2519,7 +2523,6 @@ static PHP_FUNCTION(session_regenerate_id)

/* {{{ proto void session_create_id([string prefix])
Generate new session ID. Intended for user save handlers. /
-#if 0
/ This is not used yet */
static PHP_FUNCTION(session_create_id)
{
@@ -2531,34 +2534,34 @@ static PHP_FUNCTION(session_create_id)
}

 if (prefix && ZSTR_LEN(prefix)) {

   if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) {

       /* `E_ERROR` raised for security reason. */

       php_error_docref(NULL, E_WARNING, "Prefix cannot contain

special characters. Only aphanumeric, ',', '-' are allowed");

```
       RETURN_FALSE;
```

   if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE

       || ZSTR_LEN(prefix) > PHP_SESSION_KEY_MAX_LEN/2) {

       /* Do not use prefix for security reason. */

       php_error_docref(NULL, E_WARNING,

                        "Prefix cannot exceed session max length %d "

                        "and/or contain special characters. Only

aphanumeric, ',', '-' are allowed",

                         PHP_SESSION_KEY_MAX_LEN/2);
   } else {
       smart_str_append(&id, prefix);
   }

}

if (PS(session_status) == php_session_active) {

   new_id = PS(mod)->s_create_sid(&PS(mod_data));

} else {

   new_id = php_session_create_id(NULL);

}

/* Should call the default always to avoid problems with user handler */
new_id = php_session_create_id(NULL);

if (new_id) {
smart_str_append(&id, new_id);
zend_string_release(new_id);
} else {
smart_str_free(&id);

   php_error_docref(NULL, E_WARNING, "Failed to create new ID");

   /* E_RECOVERBELE_ERROR raised for security reason. */

   php_error_docref(NULL, E_RECOVERABLE_ERROR, "Failed to create new ID");
   RETURN_FALSE;

}
smart_str_0(&id);
RETVAL_NEW_STR(id.s);
smart_str_free(&id);
}
-#endif
/* }}} */

/* {{{ proto string session_cache_limiter([string new_cache_limiter])
@@ -2901,6 +2904,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_session_id, 0, 0, 0)
ZEND_ARG_INFO(0, id)
ZEND_END_ARG_INFO()

+ZEND_BEGIN_ARG_INFO_EX(arginfo_session_create_id, 0, 0, 0)

ZEND_ARG_INFO(0, prefix)
+ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO_EX(arginfo_session_regenerate_id, 0, 0, 0)
ZEND_ARG_INFO(0, delete_old_session)
ZEND_END_ARG_INFO()
@@ -2989,6 +2996,7 @@ static const zend_function_entry session_functions[] = {
PHP_FE(session_module_name, arginfo_session_module_name)
PHP_FE(session_save_path, arginfo_session_save_path)
PHP_FE(session_id, arginfo_session_id)

PHP_FE(session_create_id, arginfo_session_create_id)
PHP_FE(session_regenerate_id, arginfo_session_regenerate_id)
PHP_FE(session_decode, arginfo_session_decode)
PHP_FE(session_encode, arginfo_session_void)

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler,
but it should be checked always by session module. Since the function
only allows

Oops, sorry

s/PHPAPI php_session_valid_chars()/PHPAPI php_session_valid_key()/

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

Since the function only allows chars used by ID, I would like to add "" a
valid char. "" should be very safe char.

I think about possible attack/misuse scenario a little more and come
up with following.

"_" is wild card char of SQL's LIKE query. Although, it should be rare to use
session ID string for LIKE query, one may do

SELECT * FROM my_sess_table WHERE sess_id LIKE '$id';
where $id is '______________________'.

This may allow to fetch all session IDs in DB. Users will likely write
such query with prefixed session ID, so I don't think allowing "_" is
not good idea after all. I'll keep as it is now, but if you have good
option. Please let me know.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

Since this RFC is about preciseness of session management, I would like to
change session_id() validates against default allowed chars as follows.
(As well as enabling already written session_create_id() function)
This patch is against the PR.

I would strongly advise not to add more things into this RFC (see my
other email). If you want to change which chars are allowed in session
ID, fine, but let's discuss it in separate topic.

However, I would proceed very carefully here, as there are apps that
produce their own session IDs, and breaking them does not help anybody.

About, since session_id() is a user function, what do we gain by
limiting what it does?

For session_create_id(), don't we already have
SessionHandler::create_sid()?

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

Since this RFC is about preciseness of session management, I would like to
change session_id() validates against default allowed chars as follows.
(As well as enabling already written session_create_id() function)
This patch is against the PR.

I would strongly advise not to add more things into this RFC (see my
other email). If you want to change which chars are allowed in session
ID, fine, but let's discuss it in separate topic.

Fine with me.

However, I would proceed very carefully here, as there are apps that
produce their own session IDs, and breaking them does not help anybody.

Sounds good. As I wrote in previous mail this is going to be largest
BC impact of changes I proposed.

About, since session_id() is a user function, what do we gain by
limiting what it does?

Prefix is a part of session ID and it should have the same requirement
as session ID for security reasons.

For session_create_id(), don't we already have
SessionHandler::create_sid()?

There is SessionHandler::create_sid(), but there isn't a function that
creates secure session ID. We may do sha1(random_bytes(32)), but
it's better to have function that uses specified hash/data by
session.hash_function/hash_bits_per_characters.

So "sha1(random_bytes(32))" and "session_create_id()" is not equal,
for example.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

About, since session_id() is a user function, what do we gain by
limiting what it does?

Prefix is a part of session ID and it should have the same requirement
as session ID for security reasons.

I'm not sure why you're talking about prefix. I thought that the issue
was that user can supply session_id() with the ID that is not good for
some reason and you want to filter it on session_id level. Am I wrong?

There is SessionHandler::create_sid(), but there isn't a function that
creates secure session ID.

Why not? The ID created now is not secure? Why? I see it uses
php_session_create_id(), do you mean this function is insecure too? Why?
In any case, if you think it is insecure, why not fix it?

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

About, since session_id() is a user function, what do we gain by
limiting what it does?

Prefix is a part of session ID and it should have the same requirement
as session ID for security reasons.

I'm not sure why you're talking about prefix. I thought that the issue
was that user can supply session_id() with the ID that is not good for
some reason and you want to filter it on session_id level. Am I wrong?

Oops, sorry. Too many lines to reply, I misread session_id()/session_create_id()

session_id() sets session ID. Invalid char that cannot be accepted should be
rejected. Otherwise, user will have lost sessions without errors.

There is SessionHandler::create_sid(), but there isn't a function that
creates secure session ID.

Why not? The ID created now is not secure? Why? I see it uses
php_session_create_id(), do you mean this function is insecure too? Why?
In any case, if you think it is insecure, why not fix it?

SessionHandler::create_sid() is for creating user own ID. Generating ID with
certain prefix.

SessionHandler::create_sid() may call parent create ID function,
though. If parent is session module's save handler, it will call
php_session_create_id().

Currently, there is no simple way to generate session ID with the form
of session module generates. i.e. hash_bits_per_characters=5/6. There
should be an API for it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

Oops, sorry. Too many lines to reply, I misread session_id()/session_create_id()

session_id() sets session ID. Invalid char that cannot be accepted should be
rejected. Otherwise, user will have lost sessions without errors.

As far as I know, handlers already reject characters that are not OK
with them. So what is missing there?

SessionHandler::create_sid() is for creating user own ID. Generating ID with
certain prefix.

Not sure what you mean. The code here:
https://github.com/php/php-src/blob/master/ext/session/mod_user_class.c#L175
is clearly generating an ID. Is this not secure enough?

Currently, there is no simple way to generate session ID with the form
of session module generates. i.e. hash_bits_per_characters=5/6. There
should be an API for it.

Wait, so which ID the SessionHandler::create_sid() generates? Isn't
that the same function? Which function you plan to use instead?

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Wed, Jan 27, 2016 at 10:22 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:

Oops, sorry. Too many lines to reply, I misread session_id()/session_create_id()

session_id() sets session ID. Invalid char that cannot be accepted should be
rejected. Otherwise, user will have lost sessions without errors.

As far as I know, handlers already reject characters that are not OK
with them. So what is missing there?

Session module/save handlers removes invalid chars silently.
This changes user defined session ID, thus session is lost without
apparent errors.

SessionHandler::create_sid() is for creating user own ID. Generating ID with
certain prefix.

Not sure what you mean. The code here:
https://github.com/php/php-src/blob/master/ext/session/mod_user_class.c#L175
is clearly generating an ID. Is this not secure enough?

If php_session_create_id() which is session module function, it's
secure. Users may create whatever session IDs, though.

Currently, there is no simple way to generate session ID with the form
of session module generates. i.e. hash_bits_per_characters=5/6. There
should be an API for it.

Wait, so which ID the SessionHandler::create_sid() generates? Isn't
that the same function? Which function you plan to use instead?

I mean there is no way to call php_session_create_id() without user
defined save handler.

Main use case of session_create_id() and session_id() would be
prefixed session like

session_id(session_create_id('MY-PREFIX-'));

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

https://wiki.php.net/rfc/precise_session_management
https://github.com/php/php-src/pull/1734

I'm re-reading this RFC and I notice it does quite a lot of things:

Add five new INI values
Add two new functions
Changes behavior of two widely used functions
Changes four different defaults
Adds magic data to session

This alone makes it very hard to figure out what is going on. It looks
like "kitchen sink" RFC instead of targeted one. I appreciate very much
your efforts to make sessions more secure, but it is very hard to review
such big thing with so many things being changed at the same time. And
it looks like not only myself think this way, judging by amount of
feedback this is getting. Maybe I just don't understand bigger picture
and all this is united by some common goal but then I'd like to have
some description of this goal which will enhance my understanding.

Going through RFC, here are the things that are not clear to me:

Why we need to change session_regenerate_id() ? It has session delete
option already. You may argue it's not safe to not delete, but then you
can opt to delete. I can even see adding option to expire session after
a short period, but what is proposed looks a bit overcomplicated.
Why you need to keep session create and update time? IIRC we already
have the time of last use (that's how GC) works, and we could have
expiration timestamp for scenarios like session_regenerate_id() but I
don't see why we need update TS in session, especially one that changes,
and same for session.ttl_update.
Not sure what function NEW_SID serves? Why we need to know new ID in
the old session?
Not sure how session.ttl would work. Would session expire in that
time regardless of usage? Then it should be unset by default. I
understand there is a use case for hard expiration, but that is separate
functionality and I'm not even sure it should be in the same RFC.
Same for session.regenerate_id - that seems to be a piece of
functionality not related to anything else. How it is connected to the
rest of the RFC? In any case, I don't see why it should be enabled by
default - RFC does not explain it. Same for session.num_sids and SIDS
value - does not explain what is the use case for it and why it is
necessary.
Changes like "Use strict mode by default (disallow stealing session
forever). Use httponly cookie by default. Use SHA1 as hash (and use 5
bits for hashed value string for better compatibility)" are not
described further and not clear how they are connected to the rest - is
it necessary for the rest of the RFC to work? Why?

Again, myself and I think others appreciate very much your efforts of
making sessions better, but such big changes in a function that
underlies almost every PHP application should be done very carefully and
with full understanding of what is going on, and doing it in a huge
package may be hard. I wonder maybe it'd be better to make RFC do one
necessary thing - e.g. handle session regeneration - and then proceed to
other things? Can we identify one problem we are trying to fix, define
functionality that fixes it, agree on it, and then proceed to the next
problem, etc.?

Thanks,

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

TL;DR; for others.
Those who have no idea why this RFC is mandatory and how current
session management is broken, please read the RFC's TL;DR;.

https://wiki.php.net/rfc/precise_session_management
https://github.com/php/php-src/pull/1734

I'm re-reading this RFC and I notice it does quite a lot of things:

Add five new INI values

Add two new functions

Changes behavior of two widely used functions

Changes four different defaults

Adds magic data to session

This alone makes it very hard to figure out what is going on. It looks
like "kitchen sink" RFC instead of targeted one. I appreciate very much
your efforts to make sessions more secure, but it is very hard to review
such big thing with so many things being changed at the same time. And
it looks like not only myself think this way, judging by amount of
feedback this is getting. Maybe I just don't understand bigger picture
and all this is united by some common goal but then I'd like to have
some description of this goal which will enhance my understanding.

Going through RFC, here are the things that are not clear to me:

Why we need to change session_regenerate_id() ? It has session delete
option already. You may argue it's not safe to not delete, but then you
can opt to delete. I can even see adding option to expire session after
a short period, but what is proposed looks a bit overcomplicated.

Previous proposal was much simpler than this because it addressed
only destroy duration issue. It was rejected even if it was simple. I
think difficulty is not about proposal volume, but basic understanding
about what is broken in current session.

Many of issues that are reported could be resolved by this, but it's not
complete as this proposal explains.

Why you need to keep session create and update time? IIRC we already
have the time of last use (that's how GC) works, and we could have
expiration timestamp for scenarios like session_regenerate_id() but I
don't see why we need update TS in session, especially one that changes,
and same for session.ttl_update.

CREATED is used to determine if the session should be renewed.
i.e. session_regenerate_id()

UPDATED is used to determine if the session is expired or not.
"UPDATED+ttl < now" is expired.

UPDATED timestamp is not updated always not to disturb lazy_write.
UPDATED is updated when "UPDATED + ttl_update < now" is true.

For most save handlers, gc's timestamping is not precise enough for
gc. gc timestamp is updated even for obsolete session also. It cannot
replace UPDATED timestamp.

Not sure what function NEW_SID serves? Why we need to know new ID in
the old session?

Browser may not get new session ID always due to bad network, etc.
e.g. Server send new session ID cookie, but connection is lost, then
browser will continue to use old session cookie ID. This is one of the
cause of random lost sessions.

My patch try to resend new session ID once for this case. This will
reduce number of lost sessions close to zero, hopefully.

Not sure how session.ttl would work. Would session expire in that
time regardless of usage? Then it should be unset by default. I
understand there is a use case for hard expiration, but that is separate
functionality and I'm not even sure it should be in the same RFC.

It's not hard expiration. I think this should be done in user space.

UPDATED will be updated by specified frequency(ttl_update).
Session expiration is checked "UPDATED + ttl < now".

Same for session.regenerate_id - that seems to be a piece of
functionality not related to anything else. How it is connected to the
rest of the RFC? In any case, I don't see why it should be enabled by
default - RFC does not explain it. Same for session.num_sids and SIDS
value - does not explain what is the use case for it and why it is
necessary.

Users must regenerate session ID for security reasons. Even if
periodic session ID regeneration is mandatory for security, there
are many codes that don't do this.

Because it is mandatory, the feature should be part of session
management.

Old session IDs are needed because users may use session IDs
for security protections. CSRF protection key is common usage.
e.g. $csrf_key = sha1(session_id() . $mysecret) something like
that.

If user sets session ID regeneration for every 30 minutes, 8
session ID is good enough for 4 hours.

BTW, I'm not recommending this CSRF protection method.
There is better ways for it.

Changes like "Use strict mode by default (disallow stealing session
forever). Use httponly cookie by default. Use SHA1 as hash (and use 5
bits for hashed value string for better compatibility)" are not
described further and not clear how they are connected to the rest - is
it necessary for the rest of the RFC to work? Why?

This RFC target is precise (~= secure) session management. This
is the reason why these are included. (As well as session_id()
accepts any chars, but we agreed to work on this in another RFC)

Again, myself and I think others appreciate very much your efforts of
making sessions better, but such big changes in a function that
underlies almost every PHP application should be done very carefully and
with full understanding of what is going on, and doing it in a huge
package may be hard. I wonder maybe it'd be better to make RFC do one
necessary thing - e.g. handle session regeneration - and then proceed to
other things? Can we identify one problem we are trying to fix, define
functionality that fixes it, agree on it, and then proceed to the next
problem, etc.?

If you see the patch, there are many phpt changes, but code and
logic is rather straight forward.

I've already have/will have 3 RFC for session.
This one, session_id() and user space serialize handler.
https://github.com/php/php-src/pull/1732
I would like not to have too many RFCs for session.

As you can see from patches, the more I have RFC, the more works for
phpt adjustment are required separately. Besides writing and managing
RFC, it consumes my time a lot.

I'm certain that changes proposed by this RFC are mandatory or almost
mandatory for better security and session stability.

So I hope you feel fine with this RFC by this explanation.
If you have further questions, it's welcomed. The logic is written in PHP
pseudo code. I'll appreciate if one could find defect if there is.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

I've already have/will have 3 RFC for session.
This one, session_id() and user space serialize handler.
https://github.com/php/php-src/pull/1732
I would like not to have too many RFCs for session.

I would like to make PHP more secure. Session is only a part of it.

I have
https://wiki.php.net/rfc/dbc2
https://wiki.php.net/rfc/secure_serialization
https://wiki.php.net/rfc/introduce-type-affinity
https://wiki.php.net/rfc/script_only_include
and so on. Even there will new RFCs such as
automatic CSRF protection when this RFC is finished
and URL rewriter bug is fixed.

I also have
https://bugs.php.net/bug.php?id=68599
https://bugs.php.net/bug.php?id=55391
https://bugs.php.net/bug.php?id=68728
https://bugs.php.net/bug.php?id=69791
and so on

Three RFCs for session is just too much for me already...

Anyway, we may be better to talk about how it should be.
For this thread, how session management should be.
It's just not good enough currently. Besides exploiting
PHP session is too easy, random lost sessions is not
acceptable. Weak defaults are not acceptable also. Let's
talk about what's missing still even with this RFC to make
session secure/stable if any.

Better ideas are welcomed. If there is, I'll implement it.
Let's finish mandatory work and move on.

Thanks,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Dan Ackroyd — view source

unread

I'll appreciate if one could find defect if there is.

The problem is that you're trying to build on a foundation of sand.
The session handling works but is incredibly fragile.

Or to put it more colloquially this is a "how to get to Dublin
problem". To get to having a more secure and reliable session
handling, we need to start from some where else, not just keep
building on top of the current session handler code.

The proposed RFC is not a good way to proceed. Having functionality
depend on setting values in an array like this (as far a I've
undertood the RFC):

// Resend new session ID once. This will reduce chance of client
// race and lost session by unstable network to acceptable level.
$_SESSION['PHP_SESSION']['NEW_SID_SENT'] = time();

is not a sensible way of programming imo. Additionally, changing code
that is apparently very fragile and so breaking people's applications
in subtle ways, is not something we should be doing.

Anyway, we may be better to talk about how it should be.
For this thread, how session management should be.
It's just not good enough currently.

To me, there are two good ways to proceed:

i) Develop a new session extension, that doesn't depend on magic
behaviour of globals and leave the current session handler as it is.
The new session extension could be shipped as a 'work in progres' when
it's good enough, before PHP 8. Then when it's stable, we could figure
out how to transition users from the old extension.

ii) Develop a session handler in userland code only. PHP is powerful
enough to support this. Although obviously there are big benefits to
shipping a session handler with PHP, I don't see any need for it to be
done internally other than we don't currently have a way of shipping
userland code with an extension. I'm hoping that before PHP 8, the
ability to ship PHP code as part of extensions would be in place.

Either of those two would be feasible. The proposed RFC doesn't appear
feasible to me.

cheers
Dan

8 years ago by Yasuo Ohgaki — view source

unread

Hi Dan,

I'll appreciate if one could find defect if there is.

The problem is that you're trying to build on a foundation of sand.
The session handling works but is incredibly fragile.

Or to put it more colloquially this is a "how to get to Dublin
problem". To get to having a more secure and reliable session
handling, we need to start from some where else, not just keep
building on top of the current session handler code.

The proposed RFC is not a good way to proceed. Having functionality
depend on setting values in an array like this (as far a I've
undertood the RFC):

Why depending setting values (timestamp) is not good way.
If you know, please let me know how to manage expiration w/o timestamp.

// Resend new session ID once. This will reduce chance of client
// race and lost session by unstable network to acceptable level.
$_SESSION['PHP_SESSION']['NEW_SID_SENT'] = time();

is not a sensible way of programming imo. Additionally, changing code
that is apparently very fragile and so breaking people's applications
in subtle ways, is not something we should be doing.

I agree that it is difficult for people who don't know PHP session
module code. Function dependency and state management may
seem like maze. However, once you understand, it's not that hard.
It's only a few thousands lines.

Anyway, we may be better to talk about how it should be.
For this thread, how session management should be.
It's just not good enough currently.

To me, there are two good ways to proceed:

i) Develop a new session extension, that doesn't depend on magic
behaviour of globals and leave the current session handler as it is.
The new session extension could be shipped as a 'work in progres' when
it's good enough, before PHP 8. Then when it's stable, we could figure
out how to transition users from the old extension.

ii) Develop a session handler in userland code only. PHP is powerful
enough to support this. Although obviously there are big benefits to
shipping a session handler with PHP, I don't see any need for it to be
done internally other than we don't currently have a way of shipping
userland code with an extension. I'm hoping that before PHP 8, the
ability to ship PHP code as part of extensions would be in place.

Either of those two would be feasible. The proposed RFC doesn't appear
feasible to me.

Why? it's already there and it works.
You are proposing

Leave random lost session bug
Leave session exploit easy

I think we must fix these.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

I have
https://wiki.php.net/rfc/dbc2

This doesn't seem to do anything with security. It's just a way of doing
asserts, which we already have.

https://wiki.php.net/rfc/secure_serialization

This may be a viable extension of somebody really is going to use it. I
would suggest making a PECL extension.

https://wiki.php.net/rfc/introduce-type-affinity

This also has nothing to do with security, and also looks unfinished,
not clear what is sought and what is the benefit.

https://wiki.php.net/rfc/script_only_include

This one we already discussed at length, and the vote result is clear.

Three RFCs for session is just too much for me already...

Maybe we should concentrate on doing one specific thing, bring it to
conclusion and then get to the next one?

Anyway, we may be better to talk about how it should be.
For this thread, how session management should be.
It's just not good enough currently. Besides exploiting
PHP session is too easy, random lost sessions is not

I am sorry, but I still see no base to this claim. You seem to be under
impression that stealing cookies from HTTPS connection is trivial, but I
do not see how it is. Moreover, since virtually all secure communication
over the web relies on this, if it were trivial, no secure communication
would be possible, and I don't see this happening.
If, however, HTTPS/cookies are secure, then I don't see how exploiting
PHP session in current state is easy. There may be some browser bugs
that make some scenarios not work sometimes, but so far I don't see any
problem that were bigger than light annoyance.

acceptable. Weak defaults are not acceptable also. Let's

There are a lot of different use cases for PHP and session, and the same
requirements do not apply to all of them. So in my opinion, the defaults
should cover the most common cases, and avoid breaking applications as
much as possible.

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

I have
https://wiki.php.net/rfc/dbc2

This doesn't seem to do anything with security. It's just a way of doing
asserts, which we already have.

It's a framework for secure programming.
Using it is up to users, but DbC is proven method for writing
sold(secure) code while maintaining the code execution performance in
production. Assert is a weak tool for it.

https://wiki.php.net/rfc/secure_serialization

This may be a viable extension of somebody really is going to use it. I
would suggest making a PECL extension.

MHAC is also a proven method for message validation. Unvalidated
inputs for serialize are source of vulnerabilities for a long time. I
think it worth to provide it for all users. I see work in progress
phpt for serialize vulnerability, why not provide secure way out of
the box?

https://wiki.php.net/rfc/introduce-type-affinity

This also has nothing to do with security, and also looks unfinished,
not clear what is sought and what is the benefit.

It related to secure programming. Validating inputs and use of proper
types is basic thing in secure programming. Type affinity helps this.

https://wiki.php.net/rfc/script_only_include

This one we already discussed at length, and the vote result is clear.

It may be. However, easiness of code inclusion in PHP is one of the
weakest point that is unique to PHP. I'm not sure how it goes, but I
shall try.

Three RFCs for session is just too much for me already...

Maybe we should concentrate on doing one specific thing, bring it to
conclusion and then get to the next one?

I would like not to start discussions for these RFCs now. Fine for me.

Anyway, we may be better to talk about how it should be.
For this thread, how session management should be.
It's just not good enough currently. Besides exploiting
PHP session is too easy, random lost sessions is not

I am sorry, but I still see no base to this claim. You seem to be under
impression that stealing cookies from HTTPS connection is trivial, but I
do not see how it is. Moreover, since virtually all secure communication
over the web relies on this, if it were trivial, no secure communication
would be possible, and I don't see this happening.

I'm not saying stealing session cookie from HTTPS is trivial. It could
be trivial with certain environment, though.

Other than HTTPS, setting unremovable cookies is easy with JavaScript
vulnerability. Currently, we only has session.use_strict_mode=1. This
is not good enough because attacker can generate valid session ID by
themselves, and set it to victims cookie. Without session ID
regeneration with precise expiration, attacker may keep logged in
session forever.

If, however, HTTPS/cookies are secure, then I don't see how exploiting
PHP session in current state is easy. There may be some browser bugs
that make some scenarios not work sometimes, but so far I don't see any
problem that were bigger than light annoyance.

Described how easy to steal session permanently above.
Without this proposal, user/web site owner will never know possible
security breach. With this proposal, user/web site owner can detect it
by raised errors for access to expired sessions.

acceptable. Weak defaults are not acceptable also. Let's

There are a lot of different use cases for PHP and session, and the same
requirements do not apply to all of them. So in my opinion, the defaults
should cover the most common cases, and avoid breaking applications as
much as possible.

Other than restricting chars for session_id(), this proposal will not
break application. Other possible minor BC happens when apps rely on
constant session ID. Relying on constant session ID is bad practice,
but the patch allows to disable automatic session ID regeneration for
better compatibility. Tests for session behavior may be affected, but
it will not affect production code.

Other than these, user should not have problems in their production
codes. If any, please let me know.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

Other than HTTPS, setting unremovable cookies is easy with JavaScript
vulnerability. Currently, we only has session.use_strict_mode=1. This
is not good enough because attacker can generate valid session ID by

That sound unlikely, given how much space needs to be searched. By the
same vein, you could say one could defeat encryption by just guessing
the key. Which, if you are unbelievably lucky, can be true, but nobody
is that lucky. I don't see how guessing session ID is easier than
guessing encryption key.

themselves, and set it to victims cookie. Without session ID
regeneration with precise expiration, attacker may keep logged in
session forever.

That's not true, as many application provide session expiration in
userspace.

Described how easy to steal session permanently above.

It was claimed, but not described how that could actually happen.

Without this proposal, user/web site owner will never know possible
security breach. With this proposal, user/web site owner can detect it
by raised errors for access to expired sessions.

I'm not sure what would be the use of it. Suppose you detected somebody
is trying to use old sessions to breach your security. Then what?
I am also not sure which proposal you refer to that helps with access to
expired sessions - aren't they just deleted? If so, you can't
distinguish access to expired session from access to one that never
existed.

Other than restricting chars for session_id(), this proposal will not
break application. Other possible minor BC happens when apps rely on
constant session ID. Relying on constant session ID is bad practice,
but the patch allows to disable automatic session ID regeneration for
better compatibility. Tests for session behavior may be affected, but
it will not affect production code.

Still do not see how restricting chars is beneficial (handlers already
restrict what is unacceptable for them).
Also, as I said, as a particular implementation detail, I don't think
specific session regeneration pattern belongs in core - not all
applications would want it, and not all in the same way. Some
applications would rather prefer to drop the session and force the user
to re-login, for example.

Other than these, user should not have problems in their production
codes. If any, please let me know.

Changing defaults and functionality always brings possibility for
breakage, and the fact that so many tests needed to be changed also
suggests that the applications that would use the same code unmodified
would be broken.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Wed, Jan 27, 2016 at 10:25 AM, Stanislav Malyshev
smalyshev@gmail.com wrote:

Other than HTTPS, setting unremovable cookies is easy with JavaScript
vulnerability. Currently, we only has session.use_strict_mode=1. This
is not good enough because attacker can generate valid session ID by

That sound unlikely, given how much space needs to be searched. By the
same vein, you could say one could defeat encryption by just guessing
the key. Which, if you are unbelievably lucky, can be true, but nobody
is that lucky. I don't see how guessing session ID is easier than
guessing encryption key.

I should have been precise. JavaScript vulnerability is "JavaScript
injection" vulnerability. It's most common problem of web apps as we
know.

themselves, and set it to victims cookie. Without session ID
regeneration with precise expiration, attacker may keep logged in
session forever.

That's not true, as many application provide session expiration in
userspace.

User may have timestamp for expiration. Problem is it's not a part of
session module management and many codes just let GC to expire session
data as you've mentioned before.

Described how easy to steal session permanently above.

It was claimed, but not described how that could actually happen.

There are 2 ways to keep/generate stolen session

Set undeletable cookie to browser
Get active session via exploit and access it before GC

As I have already explained, getting active session ID is trivial with
access to psychical device. e.g. Steal colleges' session ID while they
are leaving desk. It's just a matter of displaying session ID cookie
and take picture of it.

Without this proposal, user/web site owner will never know possible
security breach. With this proposal, user/web site owner can detect it
by raised errors for access to expired sessions.

I'm not sure what would be the use of it. Suppose you detected somebody
is trying to use old sessions to breach your security. Then what?
I am also not sure which proposal you refer to that helps with access to
expired sessions - aren't they just deleted? If so, you can't
distinguish access to expired session from access to one that never
existed.

Expired sessions are not deleted silently when there is access to it.
Expired sessions will stay in session storage until GCed. Thus, if
there is access to expired sessions, session module detects it and
raise error.

Access to expired session should not happen under normal circumstance.
It's likely a some kind of attacks.

As you've mentioned, my patch resends new session ID only once in case
session ID is lost. It is possible that raised error is due to bad
connections, but it should be very rare. If we get a lot of false
positive, we may resend new session ID more than once.

Other than restricting chars for session_id(), this proposal will not
break application. Other possible minor BC happens when apps rely on
constant session ID. Relying on constant session ID is bad practice,
but the patch allows to disable automatic session ID regeneration for
better compatibility. Tests for session behavior may be affected, but
it will not affect production code.

Still do not see how restricting chars is beneficial (handlers already
restrict what is unacceptable for them).
Also, as I said, as a particular implementation detail, I don't think
specific session regeneration pattern belongs in core - not all
applications would want it, and not all in the same way. Some
applications would rather prefer to drop the session and force the user
to re-login, for example.

I don't think it's a good way to allow invalid session key for
initialization, but silently changes key and continues.

Silent removal is done because session module has transid feature.
Users can send any session ID easily. Bad person may set bad chars in
session ID and set it to their web page to give impression that web
apps are broken, etc.

Other than these, user should not have problems in their production
codes. If any, please let me know.

Changing defaults and functionality always brings possibility for
breakage, and the fact that so many tests needed to be changed also
suggests that the applications that would use the same code unmodified
would be broken.

I agree that. That's the reason why I stopped working a year ago to
concentrate PHP7 development. We have almost a year to PHP7.1 release.
It's good timing for this kind of changes, IMHO.

I carefully considered BC impacts and implemented "should/must be"
feature/behavior. I wouldn't say there is no BC, but almost all
production code should not be affected. I'm sure there will be some
broken application unit tests, though.

Many lines of changes in phpt does not mean production code BC because
session's phpts test internal behavior/data, require immediate session
destroy. There is new data, tests should see it and verify it.
session_destroy() requires -1 parameter for immediate destroy for
cleanup. You can verify what I'm saying in phpt diffs.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi!

There are 2 ways to keep/generate stolen session

Set undeletable cookie to browser

Get active session via exploit and access it before GC

As I have already explained, getting active session ID is trivial with
access to psychical device. e.g. Steal colleges' session ID while they
are leaving desk. It's just a matter of displaying session ID cookie
and take picture of it.

Set undeletable cookie to browser

this is

Set unchangable cookie to browser

to be precise.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

CREATED is used to determine if the session should be renewed.
i.e. session_regenerate_id()

This looks like something that belongs in userspace.

UPDATED is used to determine if the session is expired or not.
"UPDATED+ttl < now" is expired.

We already have GC mechanism that does that, why duplicate it?

For most save handlers, gc's timestamping is not precise enough for
gc. gc timestamp is updated even for obsolete session also. It cannot
replace UPDATED timestamp.

I'm sorry, I don't understand it - why it's not precise enough? It's a
regular unix timestamp AFAIK, so it's as precise as the rest of
timestamps everywhere.
I am also not sure what you mean by "obsolete session" and why updating
its timestamp is a problem.

Browser may not get new session ID always due to bad network, etc.
e.g. Server send new session ID cookie, but connection is lost, then
browser will continue to use old session cookie ID. This is one of the
cause of random lost sessions.

I don't think we should try to fix bad network in PHP. If you didn't get
new session ID, then it's lost and the user will get a new login screen
and will log in again. That's what happens when you have bad network. We
shouldn't add complexity into PHP to fix bad networks.

My patch try to resend new session ID once for this case. This will
reduce number of lost sessions close to zero, hopefully.

What if you have bad network twice? If it's bad, then it's bad. Can
happen more than once.

It's not hard expiration. I think this should be done in user space.

UPDATED will be updated by specified frequency(ttl_update).
Session expiration is checked "UPDATED + ttl < now".

In this case, we already have gc lifetime parameter.

Users must regenerate session ID for security reasons. Even if
periodic session ID regeneration is mandatory for security, there
are many codes that don't do this.

I think this belongs in userspace. And I don't see it as mandatory - for
some applications, it's perfectly fine to keep long sessions. For
others, it's not.

Old session IDs are needed because users may use session IDs
for security protections. CSRF protection key is common usage.
e.g. $csrf_key = sha1(session_id() . $mysecret) something like
that.

That seems to be bringing a particular implementation of a particular
software into core. This does not look like a good design. If your CSRF
handling requires keeping old IDs, by all means keep them, but in core
nothing requires that.

Changes like "Use strict mode by default (disallow stealing session
forever). Use httponly cookie by default. Use SHA1 as hash (and use 5
bits for hashed value string for better compatibility)" are not
described further and not clear how they are connected to the rest - is
it necessary for the rest of the RFC to work? Why?

This RFC target is precise (~= secure) session management. This
is the reason why these are included. (As well as session_id()
accepts any chars, but we agreed to work on this in another RFC)

It would still be good to explain why those must be defaults and why
current defaults are not fine.
I can see strict mode by default being ok. httponly cookie is
questionable, as it may break some JS apps that may need access to
session cookie. But we may put it to a vote and see what people think.
For hash function, I don't see how it matters even a little bit, given
that session IDs are random.

If you see the patch, there are many phpt changes, but code and
logic is rather straight forward.

Many phpt changes - if those are changes to existing phpt - usually
means a lot of BC breakage. That is exactly what I am concerned with.
There's no point in releasing new version which nobody would use because
it breaks all their applications.

I've already have/will have 3 RFC for session.
This one, session_id() and user space serialize handler.
https://github.com/php/php-src/pull/1732
I would like not to have too many RFCs for session.

In this case, discussing so many unrelated changes in one RFC is worse,
IMO, since it is hugely distracting and very hard to keep track of, and
very easy to forget something. Unless there is a framework that unites
those changes, but so far I didn't see it in the RFC, I just saw a lot
of independent changes.

As you can see from patches, the more I have RFC, the more works for
phpt adjustment are required separately. Besides writing and managing
RFC, it consumes my time a lot.

That's why I suggested to do a focused effort instead of doing too many
things at once.

I'm certain that changes proposed by this RFC are mandatory or almost
mandatory for better security and session stability.

Unfortunately, I am not sure yet it was proven that they are mandatory.
Many of them pertain to very specific scenarios of very specific
implementations and as such, would be much better done in userspace.

Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

CREATED is used to determine if the session should be renewed.
i.e. session_regenerate_id()

This looks like something that belongs in userspace.

As my PHP like pseudo code illustrates, this could be done by user
script. However, severe issues described in the RFC will happens
without proposed change.

UPDATED is used to determine if the session is expired or not.
"UPDATED+ttl < now" is expired.

We already have GC mechanism that does that, why duplicate it?

Keeping timestamp for TTL and timestamp for GC is different thing.

TTL is for exact/precise expiration management.
GC is for discarding garbage.

GC cannot be a replacement of TTL management.

For most save handlers, gc's timestamping is not precise enough for
gc. gc timestamp is updated even for obsolete session also. It cannot
replace UPDATED timestamp.

I'm sorry, I don't understand it - why it's not precise enough? It's a
regular unix timestamp AFAIK, so it's as precise as the rest of
timestamps everywhere.
I am also not sure what you mean by "obsolete session" and why updating
its timestamp is a problem.

GC is depended on probability for most save handlers. Relying on
probability is not precise by its nature.

In addition, garbage sessions are active sessions. Because it is
active, garbage session can be exploited by attackers.

Browser may not get new session ID always due to bad network, etc.
e.g. Server send new session ID cookie, but connection is lost, then
browser will continue to use old session cookie ID. This is one of the
cause of random lost sessions.

I don't think we should try to fix bad network in PHP. If you didn't get
new session ID, then it's lost and the user will get a new login screen
and will log in again. That's what happens when you have bad network. We
shouldn't add complexity into PHP to fix bad networks.

IMHO. It's not a task of user space framework. Any decent session
manager must manage sessions in reliable manner.

Leaving unreliable PHP session module alone is our fault, IMO.

My patch try to resend new session ID once for this case. This will
reduce number of lost sessions close to zero, hopefully.

What if you have bad network twice? If it's bad, then it's bad. Can
happen more than once.

True. There is a chance that this mitigation fails.
However, I observed good enough results from a site processing
millions requests per day.

It's not hard expiration. I think this should be done in user space.

UPDATED will be updated by specified frequency(ttl_update).
Session expiration is checked "UPDATED + ttl < now".

In this case, we already have gc lifetime parameter.

As explained. GC != TTL.

Users must regenerate session ID for security reasons. Even if
periodic session ID regeneration is mandatory for security, there
are many codes that don't do this.

I think this belongs in userspace. And I don't see it as mandatory - for
some applications, it's perfectly fine to keep long sessions. For
others, it's not.

Simple calls of session_regenerate_id() result in lost sessions and/or
exploitable sessions. Since there is way to avoid it, why not
implement it? Leaving users have hard to debug bugs is not nice
approach.

Old session IDs are needed because users may use session IDs
for security protections. CSRF protection key is common usage.
e.g. $csrf_key = sha1(session_id() . $mysecret) something like
that.

That seems to be bringing a particular implementation of a particular
software into core. This does not look like a good design. If your CSRF
handling requires keeping old IDs, by all means keep them, but in core
nothing requires that.

Precise session management is not done in userspace, nor perfect CSRF.
Since there is ways for this, we are better to provide them out of the
box.

I think this discussion proved that precise session data management is
hard enough to be implemented by users.

Changes like "Use strict mode by default (disallow stealing session
forever). Use httponly cookie by default. Use SHA1 as hash (and use 5
bits for hashed value string for better compatibility)" are not
described further and not clear how they are connected to the rest - is
it necessary for the rest of the RFC to work? Why?

This RFC target is precise (~= secure) session management. This
is the reason why these are included. (As well as session_id()
accepts any chars, but we agreed to work on this in another RFC)

It would still be good to explain why those must be defaults and why
current defaults are not fine.
I can see strict mode by default being ok. httponly cookie is
questionable, as it may break some JS apps that may need access to
session cookie. But we may put it to a vote and see what people think.
For hash function, I don't see how it matters even a little bit, given
that session IDs are random.

Httponly is best practice and should be the default. If user code has
problem with best practice, they should disable it by their own
responsibility. Encouraging best practice by default is our
responsibility, IMO.

Since I've changed session module to detect collisions, hash function
is not too important. However, we have SHA3 already. MD5 is too
obsolete. Even SHA1 is obsolete, but it's better than MD5. I don't
mind change the default to SHA2, but it could fail because hash module
could be disabled. SHA2 also increase size of session ID, too. This may
break apps store session ID in a DB table

sess_id CHAR(32)

If you see the patch, there are many phpt changes, but code and
logic is rather straight forward.

Many phpt changes - if those are changes to existing phpt - usually
means a lot of BC breakage. That is exactly what I am concerned with.
There's no point in releasing new version which nobody would use because
it breaks all their applications.

Changes in phpt does not mean BC in production code.
If you understand changes behind it, you will see it does not affect
production code.

Tests codes verify internal behaviors/data, immediately removes unneeded
sessions, etc. These are the main reasons why many tests are updated.

I've already have/will have 3 RFC for session.
This one, session_id() and user space serialize handler.
https://github.com/php/php-src/pull/1732
I would like not to have too many RFCs for session.

In this case, discussing so many unrelated changes in one RFC is worse,
IMO, since it is hugely distracting and very hard to keep track of, and
very easy to forget something. Unless there is a framework that unites
those changes, but so far I didn't see it in the RFC, I just saw a lot
of independent changes.

This RFC is for preciseness( ~= better security)
Other RFC is not appropriate RFC for changes like INI values.

Besides, INI changes are trivial and should not disturb patch review.

As you can see from patches, the more I have RFC, the more works for
phpt adjustment are required separately. Besides writing and managing
RFC, it consumes my time a lot.

That's why I suggested to do a focused effort instead of doing too many
things at once.

This RFC and patch is focus on improving loose session management.
I agree it includes minor changes such INI default change, but it should not
be obstacles for code review.

I'm certain that changes proposed by this RFC are mandatory or almost
mandatory for better security and session stability.

Unfortunately, I am not sure yet it was proven that they are mandatory.
Many of them pertain to very specific scenarios of very specific
implementations and as such, would be much better done in userspace.

Because we have many users who noticed random lost session even if it
happens 1 in a million. I think it's good enough proof.

IMHO, it's not a users responsibility session_regenerate_id() results in
lost sessions and/or leave exploitable sessions.

Anyway, I carefully considered BC impacts. Normal applications should
not have problems with this proposal. If any, I'll consider resolution
so please try to use it and let me know.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I would like to restart better session management for PHP 7.1.

https://wiki.php.net/rfc/precise_session_management

Although this RFC targets PHP 7.1, new session management
could be applied to older releases also if majority of us agree.
Please comment.

The patch is stable enough now.

https://github.com/php/php-src/pull/1734

I made patch a little simpler by removing per session basis timestamp
handling. It's system wide now. i.e. Cannot control timestamps per
session. If it is needed, it may be added later.

Since session expiration is managed by timestamp in session data, GC
may be performed when session closes. GC may take time and prevents
server outputs because session_start() is called at the beginning of
script execution usually. If GC is done at the end of
session(rshutdown), user gets the page content just like GC is not
performed while browser looks it's still loading page. I would like to
perform GC only when session is still active when request is shutting
down.

Consequences

If script executes session_commit()/session_destroy()/use read only
option, PHP will not perform GC. i.e. GC function is not called at
all. Therefore, probability of GC decreases and GC becomes less
likely.
If script uses multiple session storage, only the last session
storage performs GC. i.e. Previously used session storage may not call
GC function at all if this is done for site wide.
User will not see occasional page rendering delay by GC. (This is
the main reason of this change)

This behavior is preferred than current one. IMO.
Does anyone have problem with this?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

Consequences

If script executes session_commit()/session_destroy()/use read only
option, PHP will not perform GC. i.e. GC function is not called at
all. Therefore, probability of GC decreases and GC becomes less
likely.

If script uses multiple session storage, only the last session
storage performs GC. i.e. Previously used session storage may not call
GC function at all if this is done for site wide.

User will not see occasional page rendering delay by GC. (This is
the main reason of this change)

This doesn't look very good. This has potential to break GC for some
applications, especially high-performance ones which are likely to use
read-only mode, and thus for them GC is needed.

I also don't understand why you need this to avoid delay. Right now GC
is performed on shutdown, doesn't it? If it's performed on shutdown, it
would not delay rendering. I'm not sure I understand why you need to
condition this on session being active or not active. Please explain.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

Consequences

If script executes session_commit()/session_destroy()/use read only
option, PHP will not perform GC. i.e. GC function is not called at
all. Therefore, probability of GC decreases and GC becomes less
likely.

If script uses multiple session storage, only the last session
storage performs GC. i.e. Previously used session storage may not call
GC function at all if this is done for site wide.

User will not see occasional page rendering delay by GC. (This is
the main reason of this change)

This doesn't look very good. This has potential to break GC for some
applications, especially high-performance ones which are likely to use
read-only mode, and thus for them GC is needed.

I also don't understand why you need this to avoid delay. Right now GC
is performed on shutdown, doesn't it? If it's performed on shutdown, it
would not delay rendering. I'm not sure I understand why you need to
condition this on session being active or not active. Please explain.

As far as I remember, session module performed GC at session_start()
from the beginning it is introduced because module cannot know used
save handlers, save path, session name (this could affect GC on some
save handlers) at shutdown.

So when GC is performed, user experiences delayed response due to GC.
This could be few seconds to minute for files handler.

For normal setups, apps use single configuration for session, GC is
done. If there is multiple session config, module cannot issue GC
except the last one used with this change. We may save used config for
GC. I'm not sure if it's worth doing it.

This RFC uses timestamp for expiration, if GC is performed or not does
not matter. The only problem is growing number of garbage in session
storage with multiple session config in a application. This could be
documentation issue. Users are better to use cron task for GC for
better user experience anyway.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

[RFC Discussion] Precise Session Management

I think this RFC needs a bit more work now to clarify those points. Thanks,

test.php

var_dump(session_id(), $_SESSION['v']++); session_regenerate_id(true); var_dump(session_id()); ?>

Let me know if you(anyone) could reproduce the bug or not by this procedure - PHP version, OS name/version, Browser name/version and easiness/hardness of reproducibility.

Why not? The ID created now is not secure? Why? I see it uses php_session_create_id(), do you mean this function is insecure too? Why? In any case, if you think it is insecure, why not fix it?

Wait, so which ID the SessionHandler::create_sid() generates? Isn't that the same function? Which function you plan to use instead?

Thanks,

There are a lot of different use cases for PHP and session, and the same requirements do not apply to all of them. So in my opinion, the defaults should cover the most common cases, and avoid breaking applications as much as possible.

Unfortunately, I am not sure yet it was proven that they are mandatory. Many of them pertain to very specific scenarios of very specific implementations and as such, would be much better done in userspace.

I think this RFC needs a bit more work now to clarify those points.
Thanks,

var_dump(session_id(),
$_SESSION['v']++);
session_regenerate_id(true);
var_dump(session_id());
?>

Let me know if you(anyone) could reproduce the bug or not by this
procedure - PHP version, OS name/version, Browser name/version and
easiness/hardness of reproducibility.

Why not? The ID created now is not secure? Why? I see it uses
php_session_create_id(), do you mean this function is insecure too? Why?
In any case, if you think it is insecure, why not fix it?

Wait, so which ID the SessionHandler::create_sid() generates? Isn't
that the same function? Which function you plan to use instead?

There are a lot of different use cases for PHP and session, and the same
requirements do not apply to all of them. So in my opinion, the defaults
should cover the most common cases, and avoid breaking applications as
much as possible.

Unfortunately, I am not sure yet it was proven that they are mandatory.
Many of them pertain to very specific scenarios of very specific
implementations and as such, would be much better done in userspace.