Session Id Collisions

12 years ago by Raymond Irving — view source

unread

Hello Everyone,

I've been reading that it's possible to encounter session id collisions
with the default php configuration. It's also been said that PHP utilizes a
cryptographically weak random number generator to
produce session ID information.

I know it's possible to change the hash function and entropy used in the
generation of the id but after looking at the php_session_create_id()
function in the source code, I am wondering if adding the User Agent
string to the default setup would improve the uniqueness of the id.

What do you think?

__
Raymond

12 years ago by Rasmus Lerdorf — view source

unread

Hello Everyone,

I've been reading that it's possible to encounter session id collisions
with the default php configuration. It's also been said that PHP utilizes a
cryptographically weak random number generator to
produce session ID information.

I know it's possible to change the hash function and entropy used in the
generation of the id but after looking at the php_session_create_id()
function in the source code, I am wondering if adding the User Agent
string to the default setup would improve the uniqueness of the id.

What do you think?

Adding a widely-known string adds very little entropy. As of PHP 5.4 we
default to using session.entropy_file set to /dev/urandom or /dev/random
(if it is available) so there is no entropy issue with the default
config as of 5.4. Before 5.4 users had to be aware enough to add that to
their php.ini themselves.

-Rasmus

12 years ago by Raymond Irving — view source

unread

Hi Rasmus,

Many thanks for the information.

It would be great if this information can be added to the docs:

http://www.php.net/manual/en/session.configuration.php#ini.session.entropy-file

__
Raymond

Hello Everyone,

I've been reading that it's possible to encounter session id collisions
with the default php configuration. It's also been said that PHP
utilizes a
cryptographically weak random number generator to
produce session ID information.

I know it's possible to change the hash function and entropy used in the
generation of the id but after looking at the php_session_create_id()
function in the source code, I am wondering if adding the User Agent
string to the default setup would improve the uniqueness of the id.

What do you think?

Adding a widely-known string adds very little entropy. As of PHP 5.4 we
default to using session.entropy_file set to /dev/urandom or /dev/random
(if it is available) so there is no entropy issue with the default
config as of 5.4. Before 5.4 users had to be aware enough to add that to
their php.ini themselves.

-Rasmus

12 years ago by Sherif Ramadan — view source

unread

Hi Rasmus,

Many thanks for the information.

It would be great if this information can be added to the docs:

http://www.php.net/manual/en/session.configuration.php#ini.session.entropy-file

Please open a documentation bug at https://bugs.php.net/ for this so
that we have a record of the issue and I'll be sure to follow up on
the ticket.

I can confirm the default configuration directives from 5.4:

http://lxr.php.net/xref/PHP_5_4/ext/session/session.c#725

and 5.3:

http://lxr.php.net/xref/PHP_5_3/ext/session/session.c#807

So I will update the docs accordingly to reflect the change.

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

I was willing to add collision detection to session module
after session adoption patch is merged.

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2012/8/24 Sherif Ramadan theanomaly.is@gmail.com:

Hi Rasmus,

Many thanks for the information.

It would be great if this information can be added to the docs:

http://www.php.net/manual/en/session.configuration.php#ini.session.entropy-file

Please open a documentation bug at https://bugs.php.net/ for this so
that we have a record of the issue and I'll be sure to follow up on
the ticket.

I can confirm the default configuration directives from 5.4:

http://lxr.php.net/xref/PHP_5_4/ext/session/session.c#725

and 5.3:

http://lxr.php.net/xref/PHP_5_3/ext/session/session.c#807

So I will update the docs accordingly to reflect the change.

12 years ago by Ferenc Kovacs — view source

unread

Hi,

I was willing to add collision detection to session module
after session adoption patch is merged.

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

Please don't top post.
What is this session adoption patch?
Is it (part of) the Strict session rfc/patch from you?

Ferenc Kovács
@Tyr43l - http://tyrael.hu

12 years ago by Yasuo Ohgaki — view source

unread

2012/8/26 Ferenc Kovacs tyra3l@gmail.com:

Hi,

I was willing to add collision detection to session module
after session adoption patch is merged.

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

Please don't top post.
What is this session adoption patch?
Is it (part of) the Strict session rfc/patch from you?

Yes.
Strict session patch reject uninitialized session ID, thus it
prevents session adoption/fixation.

I know session ID collision will not happen most likely, but
there are few people who worries collision. We can check
session ID collision when it is generated.

It's easy patch, but I didn't include the patch to focus on
adoption.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Stas Malyshev — view source

unread

Hi!

I know session ID collision will not happen most likely, but
there are few people who worries collision. We can check
session ID collision when it is generated.

You mean two randomly generated session IDs colliding? I think the
probability of it is pretty low. I mean it'd take PHP's random number
generator function to generate two equal random numbers in the same
microsecond. And these are random 64-bit numbers, so unless you're
generating billions of sessions per microsecond I don't think it's a
very real concern.

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

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

2012/8/26 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

I know session ID collision will not happen most likely, but
there are few people who worries collision. We can check
session ID collision when it is generated.

You mean two randomly generated session IDs colliding? I think the
probability of it is pretty low. I mean it'd take PHP's random number
generator function to generate two equal random numbers in the same
microsecond. And these are random 64-bit numbers, so unless you're
generating billions of sessions per microsecond I don't think it's a
very real concern.

Right,

Statistically, session ID collision will not happen.
It's very low even when we consider birthday paradox.
We could also use stronger hash (SHA-1 etc) rather than MD5.

However, collision detection is easy and makes collision impossible.
That's the whole point of the patch. Collision is most unlikely, but
unique session ID the basis of security. It's worth to have, IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Stas Malyshev — view source

unread

Hi!

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

As far as I remember there were some things that needed to be
refactored/changed and I didn't see the updates since then, but if you
could point me to the latest version I could check it out.

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

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

2012/8/26 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

As far as I remember there were some things that needed to be
refactored/changed and I didn't see the updates since then, but if you
could point me to the latest version I could check it out.

We agreed not to introduce new session save handler API so that
there would be no barrier for adoption.

I've done it months ago, I guess you missed a mail.
These are the patch.

master
https://gist.github.com/1379668

5.4
https://gist.github.com/2224196

5.3
https://gist.github.com/2224360

There might be conflict since it's made at 3/28, I'll
fix it if there is. (I'll check it myself within a few days)

if you
could point me to the latest version I could check it out.

Thank you for your time.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Stas Malyshev — view source

unread

Hi!

Thank you for the links!

master
https://gist.github.com/1379668

5.4
https://gist.github.com/2224196

5.3
https://gist.github.com/2224360

I think patch of this magnitude is not a good idea for 5.3. As for the
rest, it'd be much easier to track and comment on if you could create a
pull request. I'll read it through and see if it's OK for 5.4, but
either 5.4 or master pull would be great.

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

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

2012/8/27 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

Thank you for the links!

master
https://gist.github.com/1379668

5.4
https://gist.github.com/2224196

5.3
https://gist.github.com/2224360

I think patch of this magnitude is not a good idea for 5.3. As for the
rest, it'd be much easier to track and comment on if you could create a
pull request. I'll read it through and see if it's OK for 5.4, but
either 5.4 or master pull would be great.

OK, I'll review my patch again and send pull request for master
and 5.4 soon.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

2012/8/27 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi,

2012/8/26 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

What's the status of session adoption patch?
I've created patches for all 3 versions and I think Stats
is going to merge it to master and PHP 5.4.

As far as I remember there were some things that needed to be
refactored/changed and I didn't see the updates since then, but if you
could point me to the latest version I could check it out.

We agreed not to introduce new session save handler API so that
there would be no barrier for adoption.

I've done it months ago, I guess you missed a mail.
These are the patch.

master
https://gist.github.com/1379668

5.4
https://gist.github.com/2224196

5.3
https://gist.github.com/2224360

There might be conflict since it's made at 3/28, I'll
fix it if there is. (I'll check it myself within a few days)

if you
could point me to the latest version I could check it out.

Thank you for your time.

Regards,

It seems I've already added collision detection when I
last updated :)

It tries to generate new session ID a few times and
if it fails, it does not initialize session.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

12 years ago by Stas Malyshev — view source

unread

Hi!

It seems I've already added collision detection when I
last updated :)

It tries to generate new session ID a few times and
if it fails, it does not initialize session.

It'd be nice if we could keep it separate. Could you create a pull that
includes only the strict session functionality? You could add the
collision part in separate pull and we could discuss it separately but
it'd be nice to not mix these things.

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

12 years ago by Yasuo Ohgaki — view source

unread

Hi,

2012/8/27 Stas Malyshev smalyshev@sugarcrm.com:

Hi!

It seems I've already added collision detection when I
last updated :)

It tries to generate new session ID a few times and
if it fails, it does not initialize session.

It'd be nice if we could keep it separate. Could you create a pull that
includes only the strict session functionality? You could add the
collision part in separate pull and we could discuss it separately but
it'd be nice to not mix these things.

No problem.
I'll make separate requests for it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi stats and others,

Sorry for the delay.
I've finally updated the strict session patch.

Following diff is against PHP-5.3, but 5.4 and others will be mostly the same.

https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef

I was promised to create separate patch for session id collision detection, but
the patch is also in the diff. I left comments for the "if" statement handles
collision. If you still prefer to have separate patch, I'll do it when
I send pull requests.

If nobody has comment, I'll create patch for 5.4.

If you read this patch closely, you'll see that this patch accesses
PS(id) global directly due to limitation of current API. It would be
good to have new API for 5.5, I think.

BTW, this patch fixes bug #60634 as a side effect also.
https://bugs.php.net/bug.php?id=60634

Comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi,

Sorry for the long delay, I've sent pull requests

https://github.com/php/php-src/pull/368
https://github.com/php/php-src/pull/367
https://github.com/php/php-src/pull/366

Thank you for your time.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2012/12/24 Yasuo Ohgaki yohgaki@ohgaki.net

Hi stats and others,

Sorry for the delay.
I've finally updated the strict session patch.

Following diff is against PHP-5.3, but 5.4 and others will be mostly the
same.

https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef

I was promised to create separate patch for session id collision
detection, but
the patch is also in the diff. I left comments for the "if" statement
handles
collision. If you still prefer to have separate patch, I'll do it when
I send pull requests.

If nobody has comment, I'll create patch for 5.4.

If you read this patch closely, you'll see that this patch accesses
PS(id) global directly due to limitation of current API. It would be
good to have new API for 5.5, I think.

BTW, this patch fixes bug #60634 as a side effect also.
https://bugs.php.net/bug.php?id=60634

Comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi,

Sorry for the long delay, I've sent pull requests

https://github.com/php/php-src/pull/368
https://github.com/php/php-src/pull/367
https://github.com/php/php-src/pull/366

Hi,

I see the strict mode check is now implemented in the handlers and not
session.c, presumably to keep ABI, but this means code is duplicated and
the setting only actually works if the handler supports it. It's
unfortunate timing since 5.5 has just gone, but I think it would make much
more sense to have a new function in the structure (as in your original
patch) and do this only in PHP.next.

Having such an ini setting which quietly fails if using an unsupported
handler is not good. I guess you could keep a whitelist of supported
handlers but that's also obviously far from ideal.

Regards,

Arpad

Thank you for your time.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2012/12/24 Yasuo Ohgaki yohgaki@ohgaki.net

Hi stats and others,

Sorry for the delay.
I've finally updated the strict session patch.

Following diff is against PHP-5.3, but 5.4 and others will be mostly the
same.

https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef

I was promised to create separate patch for session id collision
detection, but
the patch is also in the diff. I left comments for the "if" statement
handles
collision. If you still prefer to have separate patch, I'll do it when
I send pull requests.

If nobody has comment, I'll create patch for 5.4.

If you read this patch closely, you'll see that this patch accesses
PS(id) global directly due to limitation of current API. It would be
good to have new API for 5.5, I think.

BTW, this patch fixes bug #60634 as a side effect also.
https://bugs.php.net/bug.php?id=60634

Comments are appreciated.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

2013/6/27 Arpad Ray arraypad@gmail.com

I see the strict mode check is now implemented in the handlers and not
session.c, presumably to keep ABI, but this means code is duplicated and
the setting only actually works if the handler supports it. It's
unfortunate timing since 5.5 has just gone, but I think it would make much
more sense to have a new function in the structure (as in your original
patch) and do this only in PHP.next.

Having such an ini setting which quietly fails if using an unsupported
handler is not good. I guess you could keep a whitelist of supported
handlers but that's also obviously far from ideal.

Thank you for comment.

There are plenty of time before 5.6 release, I would like to fix ps_module
API also. Current implementation requires ps_modules to access PS(id) to
prevent rare case of crash. I'll try to implement a little better API for
ps_modules.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Sorry for the long delay, I've sent pull requests

https://github.com/php/php-src/pull/368
https://github.com/php/php-src/pull/367
https://github.com/php/php-src/pull/366

I'm looking at the pulls, and I see these issues:

session_id is still banned in strict mode. Can we not ban it? I think
it detracts significantly from usefulness of the patch, as it would
break a lot of legit code that takes session IDs not from cookies but
from other sources and thus have to use session_id.
I got segfault when running it on test
session_set_save_handler_class_005. Here's the backtrace:

0x003a4c6d in ps_files_path_create (buf=0xbfffdae4 "", buflen=1024,
data=0x0, key=0x286f95c "ba3c966548c65fb9dd0ad4d30972f2fa") at
/Users/smalyshev/php-5.5/ext/session/mod_files.c:76
76 if (key_len <= data->dirdepth ||
(gdb) bt
#0 0x003a4c6d in ps_files_path_create (buf=0xbfffdae4 "", buflen=1024,
data=0x0, key=0x286f95c "ba3c966548c65fb9dd0ad4d30972f2fa") at
/Users/smalyshev/php-5.5/ext/session/mod_files.c:76
#1 0x003a47f9 in ps_files_key_exists (data=0x0, key=0x286f95c
"ba3c966548c65fb9dd0ad4d30972f2fa") at
/Users/smalyshev/php-5.5/ext/session/mod_files.c:230
#2 0x003a46c8 in ps_create_sid_files (mod_data=0xc9ae94, newlen=0x0) at
/Users/smalyshev/php-5.5/ext/session/mod_files.c:468
#3 0x003960ef in zim_SessionHandler_create_sid (ht=0,
return_value=0x286f880, return_value_ptr=0xbfffe228, this_ptr=0x2868260,
return_value_used=1) at
/Users/smalyshev/php-5.5/ext/session/mod_user_class.c:155

Looks like module data is not initialized properly (which is the point
of the test) but the checks on ps_create_sid_files do not check it.

Also got some other test failures:
session_save_path_variation5
session_set_save_handler_class_012
session_set_save_handler_class_016

Didn't look into those yet.

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

11 years ago by Stas Malyshev — view source

unread

Hi!

https://github.com/php/php-src/pull/368
https://github.com/php/php-src/pull/367
https://github.com/php/php-src/pull/366

I've amended your patch slightly:
https://github.com/php/php-src/pull/401

and it looks fine to me now. What do you think, is it OK with you?

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

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Mon, Aug 5, 2013 at 9:45 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

https://github.com/php/php-src/pull/368
https://github.com/php/php-src/pull/367
https://github.com/php/php-src/pull/366

I've amended your patch slightly:
https://github.com/php/php-src/pull/401

and it looks fine to me now. What do you think, is it OK with you?

Thank you for noticing crash. Data can be null, so the fix is OK.
Removing the limitation that prohibits setting session ID is fine for me,
too.

Please, apply your patch.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi,

Thank you for noticing crash. Data can be null, so the fix is OK.
Removing the limitation that prohibits setting session ID is fine for me,
too.

Please, apply your patch.

I thought we were in agreement about doing this properly in PHP.next? My
arguments against this version of the patch still stand:

Hi Arpad,

2013/6/27 Arpad Ray arraypad@gmail.com

I see the strict mode check is now implemented in the handlers and not
session.c, presumably to keep ABI, but this means code is duplicated and
the setting only actually works if the handler supports it. It's
unfortunate timing since 5.5 has just gone, but I think it would make much
more sense to have a new function in the structure (as in your original
patch) and do this only in PHP.next.

Having such an ini setting which quietly fails if using an unsupported
handler is not good. I guess you could keep a whitelist of supported
handlers but that's also obviously far from ideal.

Thank you for comment.

There are plenty of time before 5.6 release, I would like to fix ps_module
API also. Current implementation requires ps_modules to access PS(id) to
prevent rare case of crash. I'll try to implement a little better API for
ps_modules.

Arpad

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

I thought we were in agreement about doing this properly in PHP.next? My
arguments against this version of the patch still stand:

We had long discussion and decided to apply maintained branches
as security enhancement more than a year ago. We also planned to
apply the patch into 5.3 originally, but 5.3 is security fix only now.

Anyway, if users are resetting session id properly, they are protected
against session adoption attacks. However, users are not protect their
apps properly, then they are at the risk of session adoption. This fix is
rather important for PHP, since there are many setups that share
PHP with many apps. That's the reason why we decided to apply
this patch into maintained branches.

PHP web server admins should feel much safer than before with this
feature.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi Yasuo,

I thought we were in agreement about doing this properly in PHP.next? My
arguments against this version of the patch still stand:

We had long discussion and decided to apply maintained branches
as security enhancement more than a year ago. We also planned to
apply the patch into 5.3 originally, but 5.3 is security fix only now.

Anyway, if users are resetting session id properly, they are protected
against session adoption attacks. However, users are not protect their
apps properly, then they are at the risk of session adoption. This fix is
rather important for PHP, since there are many setups that share
PHP with many apps. That's the reason why we decided to apply
this patch into maintained branches.

PHP web server admins should feel much safer than before with this
feature.

I'm not against the idea in principle but still think having a security
feature which just quietly fails if you're not using one of two modified
handlers is really not good.

I also think there's no great rush to add this, because as you say, it can
be protected against in userland too.

I would much rather have a robust, clean solution even if we have to wait
until php.next for it.

Arpad

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

I'm not against the idea in principle but still think having a security
feature which just quietly fails if you're not using one of two modified
handlers is really not good.

I also think there's no great rush to add this, because as you say, it can
be protected against in userland too.

I would much rather have a robust, clean solution even if we have to wait
until php.next for it.

As I wrote, we already had long discussion a year ago and decided to
include maintained branches.

This issue should be fixed 8 years ago. It's already too late to adopt IMHO.

If you would like to know the details of risks, please refer to the RFC.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi Yasuo,

I'm not against the idea in principle but still think having a security
feature which just quietly fails if you're not using one of two modified
handlers is really not good.

I also think there's no great rush to add this, because as you say, it
can be protected against in userland too.

I would much rather have a robust, clean solution even if we have to wait
until php.next for it.

As I wrote, we already had long discussion a year ago and decided to
include maintained branches.

Could you point me to where this was decided please? I don't see a vote or
anything like a consensus in the previous threads.

This issue should be fixed 8 years ago. It's already too late to adopt
IMHO.

If you would like to know the details of risks, please refer to the RFC.

I understand the risk and your solution, I just think there's a better
solution (more like your original patch if I recall correctly) and that we
shouldn't rush to apply an inferior solution just because the issue is so
old.

Arpad

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

Could you point me to where this was decided please? I don't see a vote or
anything like a consensus in the previous threads.

There isn't vote for this RFC since this is security.
It's also a consensus.

The main thread is this. It's been 2 years ago, not a year.
Message-ID: <CAGa2bXYDbDnTi1aF4Ts9oShyy=
2NKPaTc34_+Hg7ZN0v+_MGSQ@mail.gmail.com>

You will see a few other threads around.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi Yasuo,

Could you point me to where this was decided please? I don't see a vote
or anything like a consensus in the previous threads.

There isn't vote for this RFC since this is security.
It's also a consensus.

While this is a security concern, it's not a straightforward bug fix. When
there's contention in how to fix it, I think there really should be a vote.

I've read the other threads and I don't think has been any clear consensus
about this issue and I, for one, am not happy to have what I feel is an
inferior solution committed while it's still being discussed.

To reiterate: this ini setting will quietly fail when using a handler which
hasn't been patched, like memcached, or a custom handler. That's arguably
worse than not having the setting at all since it could give people a false
sense of security.

Arpad

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

I think there really should be a vote.

This means you don't really understand the true risk of this vulnerability.
It allows permanent session ID fixation. This is CVE assigned vulnerability.
Details are explained in the RFC and I don't want to explain fully in ML
again.
(We might discussed the details in security@php.net, but I think I wrote
enough info)

Please refer to the RFC.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Arpad Ray — view source

unread

Hi Yasuo,

I think there really should be a vote.

This means you don't really understand the true risk of this vulnerability.
It allows permanent session ID fixation. This is CVE assigned
vulnerability.
Details are explained in the RFC and I don't want to explain fully in ML
again.
(We might discussed the details in security@php.net, but I think I wrote
enough info)

Please refer to the RFC.

I do really understand the risk... I'm saying there should be a vote not on
whether or not to fix it, but on how to fix it. Ideally we can figure out
something we're all happy with and don't need to vote, but while we so
evidently disagree, I think we do.

I'm not going to repeat my arguments against the committed solution yet
again, but I really think we need a better one.

Arpad

11 years ago by Stas Malyshev — view source

unread

Hi!

I'm not going to repeat my arguments against the committed solution yet
again, but I really think we need a better one.

You are free to propose a better one. Since this topic is being
discussed for almost 2 years and nobody came with anything better, as
far as I know, I think it is reasonable on this stage to go with what we
have. If you have something better that is not BC - you're welcome to
make a pull against master, if you have something that is better and is
BC - that's excellent, let's see it and if it works better, no problem
getting it into 5.5.
But as far as I see now, that is the only viable patch that we had
during pretty long time, so sitting and waiting that something better
comes along doesn't look like the best course of action. I think we
waited enough so that anybody who had better solution had a chance to
propose it and develop it, and given it is a real problem, I think at
least solution that works for now is a good thing to have.

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

11 years ago by Arpad Ray — view source

unread

Hi Stas,

On Mon, Aug 5, 2013 at 8:23 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

I'm not going to repeat my arguments against the committed solution yet
again, but I really think we need a better one.

You are free to propose a better one. Since this topic is being
discussed for almost 2 years and nobody came with anything better, as
far as I know, I think it is reasonable on this stage to go with what we
have. If you have something better that is not BC - you're welcome to
make a pull against master, if you have something that is better and is
BC - that's excellent, let's see it and if it works better, no problem
getting it into 5.5.
But as far as I see now, that is the only viable patch that we had
during pretty long time, so sitting and waiting that something better
comes along doesn't look like the best course of action. I think we
waited enough so that anybody who had better solution had a chance to
propose it and develop it, and given it is a real problem, I think at
least solution that works for now is a good thing to have.

As I've said I actually think Yasuo's original patch was a better approach,
tackling the issue in session.c instead of leaving it up to all the
handlers to implement. This would break BC but solves the major flaw of the
ini setting working with some handlers and silently failing with others. I
think it's also a cleaner approach in general.

It's a real pity that missed the 5.5 boat.

I'll have a think if there's a way to do this with BC, or at least to fail
better.

Arpad

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

Hi Stas,

On Mon, Aug 5, 2013 at 8:23 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

I'm not going to repeat my arguments against the committed solution yet
again, but I really think we need a better one.

You are free to propose a better one. Since this topic is being
discussed for almost 2 years and nobody came with anything better, as
far as I know, I think it is reasonable on this stage to go with what we
have. If you have something better that is not BC - you're welcome to
make a pull against master, if you have something that is better and is
BC - that's excellent, let's see it and if it works better, no problem
getting it into 5.5.
But as far as I see now, that is the only viable patch that we had
during pretty long time, so sitting and waiting that something better
comes along doesn't look like the best course of action. I think we
waited enough so that anybody who had better solution had a chance to
propose it and develop it, and given it is a real problem, I think at
least solution that works for now is a good thing to have.

As I've said I actually think Yasuo's original patch was a better
approach, tackling the issue in session.c instead of leaving it up to all
the handlers to implement. This would break BC but solves the major flaw of
the ini setting working with some handlers and silently failing with
others. I think it's also a cleaner approach in general.

It's a real pity that missed the 5.5 boat.

I'll have a think if there's a way to do this with BC, or at least to fail
better.

There is "reason" that we agreed that we do not have vote for this patch.
I'll write up full description after the release, since not many people
understand true risk of session adoption vulnerable session management.
(I have to check browser behaviors again, though.)

Please wait.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi Arpad,

I think there really should be a vote.

This means you don't really understand the true risk of this
vulnerability.
It allows permanent session ID fixation. This is CVE assigned
vulnerability.
Details are explained in the RFC and I don't want to explain fully in ML
again.
(We might discussed the details in security@php.net, but I think I wrote
enough info)

Please refer to the RFC.

I do really understand the risk...

It allows "permanent" session ID fixation due to browser implementations.
To make matter worse than old days, recent browsers only send one
outstanding cookie. This made attack detection impossible at server side.
(i.e. bad countermeasure(?) took by browser developers)

If you curious about this vulnerability fix still, please read the RFC and
do a little experiments. I did the experiment 2 years ago (and even 10 years
ago). I suppose things are not changed.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Sorry for the delay.
I've finally updated the strict session patch.

I'll review it ASAP, probably on the weekend. Unfortunately, we've
missed the window for 5.5 API changes, if there's any API change, but we
can still do it in master.

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

Session Id Collisions

Please don't top post. What is this session adoption patch? Is it (part of) the Strict session rfc/patch from you?

As far as I remember there were some things that needed to be refactored/changed and I didn't see the updates since then, but if you could point me to the latest version I could check it out.

I think patch of this magnitude is not a good idea for 5.3. As for the rest, it'd be much easier to track and comment on if you could create a pull request. I'll read it through and see if it's OK for 5.4, but either 5.4 or master pull would be great.

Didn't look into those yet.

Please don't top post.
What is this session adoption patch?
Is it (part of) the Strict session rfc/patch from you?

As far as I remember there were some things that needed to be
refactored/changed and I didn't see the updates since then, but if you
could point me to the latest version I could check it out.

I think patch of this magnitude is not a good idea for 5.3. As for the
rest, it'd be much easier to track and comment on if you could create a
pull request. I'll read it through and see if it's OK for 5.4, but
either 5.4 or master pull would be great.