Re: [PHP-DEV] Strict session?
Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: If user really want to set session ID, they can explicitly disable use_strict_mode. For almost all application, setting static ID is bad code. There are some applications that exploit adoptive session, but they can live with new code also. I'm not sure I understand - why prohibiting the setting of the session ID is necessary? I understand the idea of the original patch - if somebody could set your session ID for you, without knowledge of either the user or the app, it is bad since third party knows this ID and thus can use it. However, I do not understand why it is bad for the app to set the session ID - after all, session ID comes from the app anyway, what's the problem here? Why we have to deny benefits of the protection that this patch claims to provide against injecting session by the third party to the applications that set the session from inside? I do not understand the link between one and the other, can you please explain? For example, it is easy to find cases with google code search, that users are setting ID while they really should do is session_regenerate_id(). These kind of mistakes would be better to be prevented under strict mode, IMHO. We can say, shooting your own foot is your responsibility, but I think raising error would be more user friendly. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Stats, I updated the patch to address discussion. https://gist.github.com/1379668 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: Hi! My main concern with this change is that it is binary incompatible with existing session implementation, which means it would be hard to get it into 5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in this case I'm not sure why it is required. So far the only reason I understand was If we do this, users cannot know if ps_modules is new one that prevents adoption or not, especially for third party ps_modules. but this is definitely a documentation issue and even with new API you can not really know that - you just know there's a handler but you can not know the module implements it properly - maybe it just always returns OK. I updated the patch and fixed them all. There is no API incompatibility also, since I added PS_MOD_SID2/PS_MOD_FUNCS_SID2. Thanks Daniel K. I also still not sure why default SID creation function was changed to 3 identical cloned functions. I do not think it's the right way - if some macro is not allowing us to leave it as is, we can have another macro, but I do not see how splitting one standard SID creation function into a number of identical clones is beneficial. I think it needs to be changed. I consolidated key validation function into session.c. Session ID was created by php_session_create_id() function by default. So there weren't 3 creation function, but key validation functions. I hope you feel comfortable with new patch. For user module, with the patch if you add validation handler you also have to add create_sid handler - which most users don't even want to change. We should allow for using standard SID creation functions there (maybe by passing NULL there? or by having some function that implements php_session_create_id, maybe even better), and we shouldn't check validate_sid when we mean create_sid - if we allow to set create_sid, we should also support case where we set create_sid but not validate_sid. Also, all other modules when SID creation fails set invalid_sid and return NULL, while new user module SID creation returns and doesn't set invalid_sid. Why is that? We also probably need to deal then with the question what should be done when create_sid fails (I'd say we can not do much but issue fatal error but maybe there are any better ideas) if we allow it to fail. We don't seem to check invalid_sid now after create_sid, so if we're setting it, we must check it. Thank you for catching this. I added fallback to php_session_create_id() with SHA1 hash as hash function. Even if users/ps_module writers did something wrong in their code, it always has valid PS(id). It may be better to raise notice or warning error, but I don't add it. Any comments? I think the patch is much better than the original thanks to discussion. Any comments are appreciated. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: If we care about binary API compatibility, how about make PS_MOD_SID2/PS_MOD_FUNCS_SID2 macros? Then we can forget about ABI. I'm sorry, I don't understand how any macros would help anything. Adding stuff to the structure would break binary compatibility, how the macros would help it? Since it is just adding new separate structure to session module, but PHP itself checks API version number. So users cannot use the same binary module anyway. I should have said API, not ABI. Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
2011/12/4 Yasuo Ohgaki yohg...@ohgaki.net: Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: If we care about binary API compatibility, how about make PS_MOD_SID2/PS_MOD_FUNCS_SID2 macros? Then we can forget about ABI. I'm sorry, I don't understand how any macros would help anything. Adding stuff to the structure would break binary compatibility, how the macros would help it? Since it is just adding new separate structure to session module, but PHP itself checks API version number. So users cannot use the same binary module anyway. I should have said API, not ABI. Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID. Oops, I was thinking to add new structure, but I forget to do. Anyway, if we care about ABI, we should add new structure or should have other solutions. -- Yasuo Ohgaki -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi! Since it is just adding new separate structure to session module, but PHP itself checks API version number. So users cannot use the same binary module anyway. I should have said API, not ABI. Now it should compile with msession which uses PHP_MOD_SID/PS_MOD_FUNCS_SID. That's the problem. All versions in 5.3.x and 5.4.x lines should be binary compatible, means no changes that aren't binary compatible can go into 5.3 or 5.4. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi! For example, it is easy to find cases with google code search, that users are setting ID while they really should do is session_regenerate_id(). These kind of mistakes would be better to be prevented under strict mode, IMHO. I'm not sure how that would help in this case - so the set would be rejected, then the users will turn the strict mode off to make their code work and thus lose the protection it provides. How that improves anything? Setting session ID and protection against adoption are two different things, why you need to turn off the latter to get the former working? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: Hi! For example, it is easy to find cases with google code search, that users are setting ID while they really should do is session_regenerate_id(). These kind of mistakes would be better to be prevented under strict mode, IMHO. I'm not sure how that would help in this case - so the set would be rejected, then the users will turn the strict mode off to make their code work and thus lose the protection it provides. How that improves anything? Setting session ID and protection against adoption are two different things, why you need to turn off the latter to get the former working? Since the patch sets INI_ALL for session.use_strict_mode, users may disable strict_mode for specific code. They don't have to disable strict mode for whole application. It's possible allow user defined session id, but as far as I searched on google, users are just misused or abused session_id($newid). Since there are many places that users could shooting their own foot, I don't mind to allow session_id($newid). It's far more important provide protection for decent code. Should I go ahead to change this? Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: Hi! If we care about binary API compatibility, how about make PS_MOD_SID2/PS_MOD_FUNCS_SID2 macros? Then we can forget about ABI. I'm sorry, I don't understand how any macros would help anything. Adding stuff to the structure would break binary compatibility, how the macros would help it? Macro is for source level compatibility for msession or like, not for binary level. AFAIK, if we just add entry at end of structure and there is no alignment issue, then we can keep binary compatibility. There may be platforms that may causes problems, though. It there is, please let me know. I'll think about ABI for PHP 5.3/5.4, but isn't better to have current patch for PHP 5.4.0? Then we don't need additional memory initialization/checks for 5.4. I compiled sqilte module to test it, but valgrind reports memory leaks :( I guess I should use memcache for testing. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi! If user really want to set session ID, they can explicitly disable use_strict_mode. For almost all application, setting static ID is bad code. There are some applications that exploit adoptive session, but they can live with new code also. I'm not sure I understand - why prohibiting the setting of the session ID is necessary? I understand the idea of the original patch - if somebody could set your session ID for you, without knowledge of either the user or the app, it is bad since third party knows this ID and thus can use it. However, I do not understand why it is bad for the app to set the session ID - after all, session ID comes from the app anyway, what's the problem here? Why we have to deny benefits of the protection that this patch claims to provide against injecting session by the third party to the applications that set the session from inside? I do not understand the link between one and the other, can you please explain? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi! My main concern with this change is that it is binary incompatible with existing session implementation, which means it would be hard to get it into 5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in this case I'm not sure why it is required. So far the only reason I understand was If we do this, users cannot know if ps_modules is new one that prevents adoption or not, especially for third party ps_modules. but this is definitely a documentation issue and even with new API you can not really know that - you just know there's a handler but you can not know the module implements it properly - maybe it just always returns OK. Maybe we can make separate binary-compatible patch for 5.3/5.4 branches and patch with new APIs for trunk? The user-space handlers for 5.3/5.4 would still have to do the checks (though stock modules will have the checks built-in) but for trunk they could do it by defining validation callbacks. I also still not sure why default SID creation function was changed to 3 identical cloned functions. I do not think it's the right way - if some macro is not allowing us to leave it as is, we can have another macro, but I do not see how splitting one standard SID creation function into a number of identical clones is beneficial. I think it needs to be changed. For user module, with the patch if you add validation handler you also have to add create_sid handler - which most users don't even want to change. We should allow for using standard SID creation functions there (maybe by passing NULL there? or by having some function that implements php_session_create_id, maybe even better), and we shouldn't check validate_sid when we mean create_sid - if we allow to set create_sid, we should also support case where we set create_sid but not validate_sid. Also, all other modules when SID creation fails set invalid_sid and return NULL, while new user module SID creation returns and doesn't set invalid_sid. Why is that? We also probably need to deal then with the question what should be done when create_sid fails (I'd say we can not do much but issue fatal error but maybe there are any better ideas) if we allow it to fail. We don't seem to check invalid_sid now after create_sid, so if we're setting it, we must check it. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Stats, 2011/12/4 Stas Malyshev smalys...@sugarcrm.com: Hi! My main concern with this change is that it is binary incompatible with existing session implementation, which means it would be hard to get it into 5.3 and 5.4. While I understand sometimes adding handlers is inevitable, in this case I'm not sure why it is required. So far the only reason I understand was If we do this, users cannot know if ps_modules is new one that prevents adoption or not, especially for third party ps_modules. but this is definitely a documentation issue and even with new API you can not really know that - you just know there's a handler but you can not know the module implements it properly - maybe it just always returns OK. Programmer has freedom to do bad thing, but it's important for better security to have dedicated API for validation. From my experience, it's much easier let programmers to use API, but to code properly. Programmers may use strict session using methods that are written in wiki page. However, most of applications don't have proper protection. One reason would be lack of API, I suppose. Maybe we can make separate binary-compatible patch for 5.3/5.4 branches and patch with new APIs for trunk? The user-space handlers for 5.3/5.4 would If we care about binary API compatibility, how about make PS_MOD_SID2/PS_MOD_FUNCS_SID2 macros? Then we can forget about ABI. still have to do the checks (though stock modules will have the checks built-in) but for trunk they could do it by defining validation callbacks. I also still not sure why default SID creation function was changed to 3 identical cloned functions. I do not think it's the right way - if some macro is not allowing us to leave it as is, we can have another macro, but I do not see how splitting one standard SID creation function into a number of identical clones is beneficial. I think it needs to be changed. Ok. I'll make a default char validation function for this and make it call from mod_*.c If ps_module writers would like to use special chars for whatever reason, they may use their own function. For user module, with the patch if you add validation handler you also have to add create_sid handler - which most users don't even want to change. We should allow for using standard SID creation functions there (maybe by passing NULL there? or by having some function that implements php_session_create_id, maybe even better), and we shouldn't check validate_sid when we mean create_sid - if we allow to set create_sid, we should also support case where we set create_sid but not validate_sid. Also, all other modules when SID creation fails set invalid_sid and return NULL, while new user module SID creation returns and doesn't set invalid_sid. Why is that? We also probably need to deal then with the question what should be done when create_sid fails (I'd say we can not do much but issue fatal error but maybe there are any better ideas) if we allow it to fail. We don't seem to check invalid_sid now after create_sid, so if we're setting it, we must check it. Thank you. I'll take care of this. If script or ps_module returns empty SID, it should generate SID by using default SID creation function. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Daniel,
2011/12/2 Daniel K. d...@uw.no:
Yasuo Ohgaki wrote:
2011/12/2 Daniel K. d...@uw.no:
Yasuo Ohgaki wrote:
2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net:
Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.
Does CODING_STANDARDS mention this?
I hope not, this should be obvious.
Depends on editor or IDE setting/behavior, but I don't have problem with this.
Since Daniel mentioned that he cannot disable strict session,
I did no such thing. from where did you get that idea?
Because you wrote this.
You cut out the essential lines before:
It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)
I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails _and_ additionally sets
PS(invalid_session_id)
Where I proposition that the existing logic (which you removed in your
patch) could not have done any looping. Because, then the code below would
not have worked.
This could never work with:
session_id(foo);
session_start();
could it?
No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this must
have been added for the benefit of external modules once upon a time.
However, the current code seems to never have worked for anyone.
If we suppose that it exists an external session module which sets
PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of
session_id(foo); session_start(); would have left you with a NULL
session_id, then a new one would have been created after goto new_session.
I hope someone would have discovered this had they actually implemented such
a module.
I take this as a sign that no such module exists, but I may of course be
wrong in that.
It could be possible that uses current code to prevent session
adoption. To do that, ps_module should keep state variable to avoid
infinite goto loops.
Current code (+ PECL's sqlite/memcache/msession/session_pgsql and
memcached ps_module) is not written to use this goto loop such way.
When read failure happens, these ps_module simply goes to infinate
loop. Therefore, new code does not hart any.
However, I found that msession is using PS_MOD_SID macro. It should
add validate_id() to work, but it's just getting ID using msession
function and use php_session_create_id() when it fails. So using
PS_MOD macro is also ok for msession.
I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod
may
be in for a nasty surpris because of this.
Remember, this is not just about the return value of hash functions, as
this
is used to validate session_ids set with session_id() as well.
With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.
If by 'user' you mean any PHP script, I disagree.
If user really want to set session ID, they can explicitly disable
use_strict_mode.
For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.
BTW, one module that is exploiting adoptive session is session_pgsql
which is written by me.
The purported goal of the patch was to thwart attempts to choose ones own
session_id by sending fake/stale cookies. To also make session_id() unusable
from PHP scripts, or to thwart the script-writers ability to choose the
session_id seems like a serious bug.
Almost all application shouldn't do this, unless they really
understand what they are doing.
If users (script writers) really wants to create session ID by their
own, they can use user save handlers or they can simply turn it off by
ini_set('session.use_strict_mode', false).
If user would like to use adoptive session, user may set
session.use_strict_mode=0.
Of course, but I think we need to find out the semantics, and the expected
use case / protection level this new feature is supposed to have.
With my current understanding of the patch, and my use of PHP, it seems
mostly useless. But, I stress again that I have just read the code, I have
not actually used it yet.
Please try to use it. There shouldn't be any problems for almost all
applications. I haven't check Shuhosin and it's patch, but I think it
has same code. Applications that requires some kind of static or
somewhat static session ID should be many.
According to google code search, there are some code that actually
using session_id() to set ID.
e.g.
if ((isset($s)) ($s == 'contact')) {
if (session_id() != 'contact') {
/* Retrieve the value of the hidden field in
the contact module */
session_id('contact');
Re: [PHP-DEV] Strict session?
On Fri, Dec 2, 2011 at 10:31 AM, Yasuo Ohgaki yohg...@ohgaki.net wrote:
Hi Daniel,
2011/12/2 Daniel K. d...@uw.no:
Yasuo Ohgaki wrote:
2011/12/2 Daniel K. d...@uw.no:
Yasuo Ohgaki wrote:
2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net:
Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.
Does CODING_STANDARDS mention this?
I hope not, this should be obvious.
Depends on editor or IDE setting/behavior, but I don't have problem with
this.
Since Daniel mentioned that he cannot disable strict session,
I did no such thing. from where did you get that idea?
Because you wrote this.
You cut out the essential lines before:
It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?)
I do not believe this could have been used as you speculate. The retry
logic kicks in when PS_READ_FUNC() fails _and_ additionally sets
PS(invalid_session_id)
Where I proposition that the existing logic (which you removed in your
patch) could not have done any looping. Because, then the code below
would
not have worked.
This could never work with:
session_id(foo);
session_start();
could it?
No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this
must
have been added for the benefit of external modules once upon a time.
However, the current code seems to never have worked for anyone.
If we suppose that it exists an external session module which sets
PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of
session_id(foo); session_start(); would have left you with a NULL
session_id, then a new one would have been created after goto
new_session.
I hope someone would have discovered this had they actually implemented
such
a module.
I take this as a sign that no such module exists, but I may of course be
wrong in that.
It could be possible that uses current code to prevent session
adoption. To do that, ps_module should keep state variable to avoid
infinite goto loops.
Current code (+ PECL's sqlite/memcache/msession/session_pgsql and
memcached ps_module) is not written to use this goto loop such way.
When read failure happens, these ps_module simply goes to infinate
loop. Therefore, new code does not hart any.
However, I found that msession is using PS_MOD_SID macro. It should
add validate_id() to work, but it's just getting ID using msession
function and use php_session_create_id() when it fails. So using
PS_MOD macro is also ok for msession.
I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor
sod
may
be in for a nasty surpris because of this.
Remember, this is not just about the return value of hash functions, as
this
is used to validate session_ids set with session_id() as well.
With strict session, user cannot set session ID. If user can, it's not
a strict session, but adoptive.
If by 'user' you mean any PHP script, I disagree.
If user really want to set session ID, they can explicitly disable
use_strict_mode.
For almost all application, setting static ID is bad code. There are
some applications that exploit adoptive session, but they can live
with new code also.
BTW, one module that is exploiting adoptive session is session_pgsql
which is written by me.
The purported goal of the patch was to thwart attempts to choose ones own
session_id by sending fake/stale cookies. To also make session_id()
unusable
from PHP scripts, or to thwart the script-writers ability to choose the
session_id seems like a serious bug.
Almost all application shouldn't do this, unless they really
understand what they are doing.
If users (script writers) really wants to create session ID by their
own, they can use user save handlers or they can simply turn it off by
ini_set('session.use_strict_mode', false).
If user would like to use adoptive session, user may set
session.use_strict_mode=0.
Of course, but I think we need to find out the semantics, and the
expected
use case / protection level this new feature is supposed to have.
With my current understanding of the patch, and my use of PHP, it seems
mostly useless. But, I stress again that I have just read the code, I
have
not actually used it yet.
Please try to use it. There shouldn't be any problems for almost all
applications. I haven't check Shuhosin and it's patch, but I think it
has same code. Applications that requires some kind of static or
somewhat static session ID should be many.
According to google code search, there are some code that actually
using session_id() to set ID.
e.g.
if ((isset($s)) ($s == 'contact')) {
if
Re: [PHP-DEV] Strict session?
Hi all, I haven't mentioned, but I'll file CVE for this because this is exploitable venerability. Unless it is filed by CVE, not many people would know the risk of adoption/fixation. There may be the entry for this, though. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Ferenc,
2011/12/2 Ferenc Kovacs tyr...@gmail.com:
wouldn't it be better if we push the session id validation to the
application level?
we should provide a hook both to the C api and to
the session_set_save_handler.
of course we can additionally change the default range of valid characters
for the default session handler implementation, but it would still possible
for the application developer to change or extend that.
It's possible with session_set_save_handler(), but users should
implement all save handlers. session_set_save_handler() could be
modified just to add validation function and choose any chars except
chars invalidated by php_session_initialize()
/* check session name for invalid characters */
if (PS(id) strpbrk(PS(id), \r\n\t '\\\)) {
efree(PS(id));
PS(id) = NULL;
}
For example, we may do
bool session_set_save_handler(SESSION_SET_VALIDATE_ID,
my_validation_id_function);
There are many possible implementations.
Any comments?
Regards,
--
Yasuo Ohgaki
yohg...@ohgaki.net
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Daniel,
2011/12/1 Daniel K. d...@uw.no:
Yasuo Ohgaki wrote:
I've made the patch for trunk. I checked by using session module
tests using valgrind.
https://gist.github.com/1379668
As a general comment there seems to be lots of extra white space in the
'empty' lines of the patch.
Spaces are left by my emacs.
I don't think there are many lines with extra spaces, but I'll check.
Index: ext/session/php_session.h
===
--- ext/session/php_session.h (revision 319702)
+++ ext/session/php_session.h (working copy)
@@ -69,7 +72,7 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x);
This is bogus, either you meant to do as for PS_FUNCS_SID below;
In this case ; may not be needed.
I'll just remove it.
@@ -83,11 +86,12 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x); \
+ PS_VALIDATE_SID_FUNC(x)
[...]
Index: ext/session/mod_user_class.c
===
--- ext/session/mod_user_class.c (revision 319702)
+++ ext/session/mod_user_class.c (working copy)
@@ -141,4 +141,74 @@
RETVAL_LONG(PS(default_mod)-s_gc(PS(mod_data), maxlifetime,
nrdels TSRMLS_CC));
}
+
+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{
+ size_t len;
+ const char *p;
+ char c;
+ int ret = SUCCESS;
+ + for (p = key; (c = *p); p++) {
+ /* valid characters are a..z,A..Z,0..9 */
and ',' and '-', bug copied from mod_files.c?
No, it's from the original patch that has written by Stefan.
Restricting chars is generally considered better for security.
I'm considering just omitting lower special chars, but also I think
there aren't any point that allowing chars are not needed for the
ps_module. Allowing 'x','y','z',etc for hash function is meaning
less.
+ if (!((c = 'a' c = 'z')
+ || (c = 'A' c = 'Z')
+ || (c = '0' c = '9')
+ || c == ','
+ || c == '-')) {
+ ret = FAILURE;
+ break;
+ }
+ }
Index: ext/session/mod_mm.c
===
--- ext/session/mod_mm.c (revision 319702)
+++ ext/session/mod_mm.c (working copy)
@@ -257,6 +257,38 @@
free(data);
}
+/* If you change the logic here, please also update the error message in
+ * ps_files_open() appropriately */
Really?
Copied from existing files ps_module.
Index: ext/session/session.c
===
--- ext/session/session.c (revision 319702)
+++ ext/session/session.c (working copy)
@@ -1663,7 +1670,10 @@
/* remove shutdown function */
remove_user_shutdown_function(session_shutdown,
sizeof(session_shutdown) TSRMLS_CC);
- for (i = 0; i 6; i++) {
+ for (i = 0; i 8; i++) {
+ if (i = num_args) {
+ break;
+ }
@@ -1677,7 +1687,10 @@
zend_alter_ini_entry(session.save_handler,
sizeof(session.save_handler), user, sizeof(user)-1, PHP_INI_USER,
PHP_INI_STAGE_RUNTIME);
}
- for (i = 0; i 6; i++) {
+ for (i = 0; i 8; i++) {
+ if (i = num_args) {
+ break;
+ }
What about:
for (i = 0, i num_args, i++)
That's better. I just put if when I noticed num_args matters.
I'll change it.
@@ -2218,9 +2231,13 @@
for (i = 0, mod = ps_modules; i MAX_MODULES; i++, mod++) {
if (*mod (*mod)-s_name) {
smart_str_appends(save_handlers, (*mod)-s_name);
- smart_str_appendc(save_handlers, ' ');
+ if ((*mod)-s_validate_sid) {
+ smart_str_appends(save_handlers,
(strict) );
+ } else {
+ smart_str_appends(save_handlers,
(adaptive) );
}
}
+ }
Funky indentation and brace placement.
I though tabfied, but it seem there are spaces.
What do you mean brace placement?
These ps modules should implement validate_sid(). Modules that are
using PS_MOD/PS_FUNCS are not affected, they just marked as adaptive
module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())
NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but
it was the same as PS_MOD()/PS_MOD_FUNCS().
Well, not exactly the same.
I forgot that.
Thanks.
#define PS_MOD(x) \
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \
Re: [PHP-DEV] Strict session?
On 12/01/2011 02:38 AM, Yasuo Ohgaki wrote: Hi Daniel, 2011/12/1 Daniel K. d...@uw.no: Yasuo Ohgaki wrote: I've made the patch for trunk. I checked by using session module tests using valgrind. https://gist.github.com/1379668 As a general comment there seems to be lots of extra white space in the 'empty' lines of the patch. Spaces are left by my emacs. I don't think there are many lines with extra spaces, but I'll check. Please just tell your diff to ignore whitespace when you create the patch. It is annoying to have to sift through extra irrelevant lines. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Rasmus, 2011/12/1 Rasmus Lerdorf ras...@lerdorf.com: Please just tell your diff to ignore whitespace when you create the patch. It is annoying to have to sift through extra irrelevant lines. I think Daniel mean there are extra spaces for indent. I'll fix it. Since Daniel mentioned that he cannot disable strict session, I compiled against current head. According to run-tests.php, it seems there is something wrong that prevents disabling strict session. I'll fix it, then notify updated patch. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: I think Daniel mean there are extra spaces for indent. I'll fix it. Done. Since Daniel mentioned that he cannot disable strict session, I compiled against current head. According to run-tests.php, it seems there is something wrong that prevents disabling strict session. I'll fix it, then notify updated patch. Sorry Daniel. You should do session.use_strict_mode=0 NOT session.use_strict_session=0 I also updated tests that are exploiting adoptive sessions. https://gist.github.com/1379668 Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi all, According to comments, I updated the patch. Test files are added. Entry added to php.ini-* https://gist.github.com/1379668 -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Yasuo Ohgaki wrote:
2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net:
I think Daniel mean there are extra spaces for indent.
I'll fix it.
That's exactly it, however the updated patch still has problems.
Search for a + followed by only tabs or spaces. Empty lines should be
just that, empty.
Since Daniel mentioned that he cannot disable strict session,
I did no such thing. from where did you get that idea?
I mentioned that I had not tested the patch at all, just read it.
I also updated tests that are exploiting adoptive sessions.
https://gist.github.com/1379668
I see you've addressed a few of my comments, but not all.
Specifically, excessive whitespace remains. Both on otherwise empty
lines, and on lines indented with a tab and a space, which just looks
sloppy. See e.g. PS_VALIDATE_SID_FUNC(mm) as an example showing both
defects. You've only fixed it in ext/session/session.c, do the same
to the rest of the patch.
These comments should either be fixed to match the code, or deleted.
+ /* valid characters are a..z,A..Z,0..9 */
+ if (!((c = 'a' c = 'z')
+ || (c = 'A' c = 'Z')
+ || (c = '0' c = '9')
+ || c == ','
+ || c == '-')) {
ps_user_valid_key returns SUCCESS/FAILURE.
ps_mm_valid_key returns 1/0 as does the exsting ps_files_valid_key.
I am in serious doubt as to whether the additonal restrictions on valid
characters in session ids are appropriate, and I fear that some poor sod
may be in for a nasty surpris because of this.
Remember, this is not just about the return value of hash functions, as
this is used to validate session_ids set with session_id() as well.
Daniel K.
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Yasuo Ohgaki wrote: Hi all, According to comments, I updated the patch. Our emails crossed, however my comments still stand. Test files are added. Thanks for that. Entry added to php.ini-* Typo: +session.usr_strict_mode = 1 Daniel K. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
2011/12/2 Daniel K. d...@uw.no: Yasuo Ohgaki wrote: 2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: I think Daniel mean there are extra spaces for indent. I'll fix it. That's exactly it, however the updated patch still has problems. Search for a + followed by only tabs or spaces. Empty lines should be just that, empty. Does CODING_STANDARDS mention this? Since Daniel mentioned that he cannot disable strict session, I did no such thing. from where did you get that idea? Because you wrote this. This could never work with: session_id(foo); session_start(); could it? I think you understands it can be controlled by session.use_strict_mode now. I am in serious doubt as to whether the additonal restrictions on valid characters in session ids are appropriate, and I fear that some poor sod may be in for a nasty surpris because of this. Remember, this is not just about the return value of hash functions, as this is used to validate session_ids set with session_id() as well. With strict session, user cannot set session ID. If user can, it's not a strict session, but adoptive. If user would like to use adoptive session, user may set session.use_strict_mode=0. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi Daniel, 2011/12/2 Daniel K. d...@uw.no: Yasuo Ohgaki wrote: Hi all, According to comments, I updated the patch. Our emails crossed, however my comments still stand. Test files are added. Thanks for that. Entry added to php.ini-* Typo: +session.usr_strict_mode = 1 Thank you. I'll fix it. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi all, 2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: With strict session, user cannot set session ID. If user can, it's not a strict session, but adoptive. If user would like to use adoptive session, user may set session.use_strict_mode=0. When user tried to use session_id() to set their on ID when session.use_strict_mode=1, it would be nicer to emit warning error. Failing silently is not good. Any comments? Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi all, 2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: Hi all, 2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: With strict session, user cannot set session ID. If user can, it's not a strict session, but adoptive. If user would like to use adoptive session, user may set session.use_strict_mode=0. When user tried to use session_id() to set their on ID when session.use_strict_mode=1, it would be nicer to emit warning error. Failing silently is not good. Any comments? Updated patch so that session_id() raise warning when use_strict_mode is on. I forgot to mention that I've fixed session_set_save_handler() to take care new parameters. If everybody is comfortable with this patch. I would like to write docs for this. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Yasuo Ohgaki wrote: 2011/12/2 Daniel K. d...@uw.no: Yasuo Ohgaki wrote: 2011/12/2 Yasuo Ohgaki yohg...@ohgaki.net: Search for a + followed by only tabs or spaces. Empty lines should be just that, empty. Does CODING_STANDARDS mention this? I hope not, this should be obvious. Since Daniel mentioned that he cannot disable strict session, I did no such thing. from where did you get that idea? Because you wrote this. You cut out the essential lines before: It removes session read failure retry code from php_session_initialize() . The retry code may cause infinite loop depending on save handler implementation. Some session save handlers may be using this to prevent adoption(?) I do not believe this could have been used as you speculate. The retry logic kicks in when PS_READ_FUNC() fails _and_ additionally sets PS(invalid_session_id) Where I proposition that the existing logic (which you removed in your patch) could not have done any looping. Because, then the code below would not have worked. This could never work with: session_id(foo); session_start(); could it? No code in PHP sets PS(invalid_session_id) in PS_READ_FUNC(), so this must have been added for the benefit of external modules once upon a time. However, the current code seems to never have worked for anyone. If we suppose that it exists an external session module which sets PS(invalid_session_id) in PS_READ_FUNC(), the above sequence of session_id(foo); session_start(); would have left you with a NULL session_id, then a new one would have been created after goto new_session. I hope someone would have discovered this had they actually implemented such a module. I take this as a sign that no such module exists, but I may of course be wrong in that. I am in serious doubt as to whether the additonal restrictions on valid characters in session ids are appropriate, and I fear that some poor sod may be in for a nasty surpris because of this. Remember, this is not just about the return value of hash functions, as this is used to validate session_ids set with session_id() as well. With strict session, user cannot set session ID. If user can, it's not a strict session, but adoptive. If by 'user' you mean any PHP script, I disagree. The purported goal of the patch was to thwart attempts to choose ones own session_id by sending fake/stale cookies. To also make session_id() unusable from PHP scripts, or to thwart the script-writers ability to choose the session_id seems like a serious bug. If user would like to use adoptive session, user may set session.use_strict_mode=0. Of course, but I think we need to find out the semantics, and the expected use case / protection level this new feature is supposed to have. With my current understanding of the patch, and my use of PHP, it seems mostly useless. But, I stress again that I have just read the code, I have not actually used it yet. Daniel K. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Yasuo Ohgaki wrote:
I've made the patch for trunk. I checked by using session module
tests using valgrind.
https://gist.github.com/1379668
As a general comment there seems to be lots of extra white space
in the 'empty' lines of the patch.
Index: ext/session/php_session.h
===
--- ext/session/php_session.h (revision 319702)
+++ ext/session/php_session.h (working copy)
@@ -69,7 +72,7 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x);
This is bogus, either you meant to do as for PS_FUNCS_SID below;
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x); \
+ PS_VALIDATE_SID_FUNC(x)
or you should leave it alone. Your comments below suggests the latter.
@@ -83,11 +86,12 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x); \
+ PS_VALIDATE_SID_FUNC(x)
[...]
Index: ext/session/mod_user_class.c
===
--- ext/session/mod_user_class.c(revision 319702)
+++ ext/session/mod_user_class.c(working copy)
@@ -141,4 +141,74 @@
RETVAL_LONG(PS(default_mod)-s_gc(PS(mod_data), maxlifetime, nrdels
TSRMLS_CC));
}
+
+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{
+ size_t len;
+ const char *p;
+ char c;
+ int ret = SUCCESS;
+
+ for (p = key; (c = *p); p++) {
+ /* valid characters are a..z,A..Z,0..9 */
and ',' and '-', bug copied from mod_files.c?
+ if (!((c = 'a' c = 'z')
+ || (c = 'A' c = 'Z')
+ || (c = '0' c = '9')
+ || c == ','
+ || c == '-')) {
+ ret = FAILURE;
+ break;
+ }
+ }
Index: ext/session/mod_mm.c
===
--- ext/session/mod_mm.c(revision 319702)
+++ ext/session/mod_mm.c(working copy)
@@ -257,6 +257,38 @@
free(data);
}
+/* If you change the logic here, please also update the error message in
+ * ps_files_open() appropriately */
Really?
+static int ps_mm_valid_key(const char *key)
+{
+ size_t len;
+ const char *p;
+ char c;
+ int ret = 1;
+
+ for (p = key; (c = *p); p++) {
+ /* valid characters are a..z,A..Z,0..9 */
Deja vu.
+ if (!((c = 'a' c = 'z')
+ || (c = 'A' c = 'Z')
+ || (c = '0' c = '9')
+ || c == ','
+ || c == '-')) {
+ ret = 0;
+ break;
+ }
+ }
Index: ext/session/mod_user.c
===
--- ext/session/mod_user.c (revision 319702)
+++ ext/session/mod_user.c (working copy)
@@ -163,6 +163,81 @@
+static int ps_user_valid_key(const char *key TSRMLS_DC)
+{
+ size_t len;
+ const char *p;
+ char c;
+ int ret = SUCCESS;
+
+ for (p = key; (c = *p); p++) {
+ /* valid characters are a..z,A..Z,0..9 */
Again.
+ if (!((c = 'a' c = 'z')
+ || (c = 'A' c = 'Z')
+ || (c = '0' c = '9')
+ || c == ','
+ || c == '-')) {
+ ret = FAILURE;
+ break;
+ }
+ }
Index: ext/session/session.c
===
--- ext/session/session.c (revision 319702)
+++ ext/session/session.c (working copy)
@@ -1663,7 +1670,10 @@
/* remove shutdown function */
remove_user_shutdown_function(session_shutdown,
sizeof(session_shutdown) TSRMLS_CC);
- for (i = 0; i 6; i++) {
+ for (i = 0; i 8; i++) {
+if (i = num_args) {
+break;
+}
@@ -1677,7 +1687,10 @@
zend_alter_ini_entry(session.save_handler, sizeof(session.save_handler),
user, sizeof(user)-1, PHP_INI_USER, PHP_INI_STAGE_RUNTIME);
}
- for (i = 0; i 6; i++) {
+ for (i = 0; i 8; i++) {
+if (i = num_args) {
+break;
+}
What about:
for (i = 0, i num_args, i++)
@@ -2218,9 +2231,13 @@
for (i = 0, mod = ps_modules; i MAX_MODULES; i++, mod++) {
if (*mod (*mod)-s_name) {
smart_str_appends(save_handlers, (*mod)-s_name);
- smart_str_appendc(save_handlers, ' ');
+if ((*mod)-s_validate_sid) {
+smart_str_appends(save_handlers,
Re: [PHP-DEV] Strict session?
Hi folks, 2011/11/29 Yasuo Ohgaki yohg...@ohgaki.net: I updated wiki page to remove draft status. https://wiki.php.net/rfc/strict_sessions Any corrections and comments are appreciated. I think I've wrote everything needed into the Wiki page to understand the issue, so I changed status to Under Discussion. According to https://wiki.php.net/rfc/voting minimum discussion period is 2 weeks and voting is required. How should I proceed? BTW, please note that this patch is must have patch for security season. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi all, Some users that have tested this patch asked me if it's possible deleting offending cookies that enable targeted DoS attack. https://wiki.php.net/rfc/strict_sessions I would like to add patch that deletes offending cookies which may controlled by php.ini setting. I can try to delete possible offending cookies, but recent browsers only sent outstanding cookie. Therefore, it's impossible to know if it deleted all offending cookies was successfully deleted. This feature will be best effort based feature. I think the default setting for deleting cookies should be off by default, so that users could notice configuration problems. (i.e. cookie path/domain, session name) This patch eliminates session adoption/fixation, but introduces targeted DoS as I mentioned already. Even if it may not be possible to delete all malicious cookies, but it is worthwhile to have this feature. Any comments? Hannes, I could edit the page once, but save button is disabled for some reason. Could you check my karma? Thank you. -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hannes, I could edit the page once, but save button is disabled for some reason. Could you check my karma? Thank you. you have to either provide a comment for the wiki change or check the Minor Changes checkbox to be allowed to save your work. -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Strict session?
On Mon, Nov 28, 2011 at 21:51, Yasuo Ohgaki yohg...@ohgaki.net wrote: Hannes, I could edit the page once, but save button is disabled for some reason. Could you check my karma? Thank you. Please fill out the large pink field called Edit summary.. and write something in it. The save button will then be enabled. -Hannes -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Thank you Ferenc and Hannes! -- Yasuo Ohgaki -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi folks, I updated wiki page to remove draft status. https://wiki.php.net/rfc/strict_sessions Any corrections and comments are appreciated. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
He has svn account. If you login with your svn credentials you'll have full write karma to everything on the wiki. If you don't remember your password; https://master.php.net/forgot.php (your username is yohgaki) -Hannes On Wed, Nov 23, 2011 at 02:08, Ferenc Kovacs tyr...@gmail.com wrote: On Wed, Nov 23, 2011 at 1:46 AM, Yasuo Ohgaki yohg...@ohgaki.net wrote: Ferenc, I can login to wiki, but cannot write to the page. https://wiki.php.net/rfc/strict_sessions Could you update the page with attached file? Thank you. -- Yasuo Ohgaki Sure, Hannes, could you please give karma to Yasuo so that he can edit the rfc himself? Thanks! -- Ferenc Kovács @Tyr43l - http://tyrael.hu -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
2011/11/23 Hannes Magnusson bj...@php.net: He has svn account. If you login with your svn credentials you'll have full write karma to everything on the wiki. If you don't remember your password; https://master.php.net/forgot.php (your username is yohgaki) Thank you Hannes, I don't know why I could not press save button, but I can save page now. I've already fixed a few broken English. -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
On Wed, Nov 23, 2011 at 2:53 PM, Yasuo Ohgaki yohg...@ohgaki.net wrote: 2011/11/23 Hannes Magnusson bj...@php.net: He has svn account. If you login with your svn credentials you'll have full write karma to everything on the wiki. If you don't remember your password; https://master.php.net/forgot.php (your username is yohgaki) Thank you Hannes, I don't know why I could not press save button, but I can save page now. I've already fixed a few broken English. -- Yasuo Ohgaki yohg...@ohgaki.net My guess is that you were trying to edit the page while I had it open for editing also, so I think that it was my bad. :) -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Strict session?
Hi Stats, Thanks to Perrie, I realized I should try to this patch to be accepted for 5.4 branch. As you may already knew, session adoption is real threat for PHP applications. Therefore, this patch should be in 5.4.0 for better security. In case of you've missed why session adoption is real threat for PHP applications, please try to set the same cookies to various path and domains. Then check and compare the result with IE and Chrome/Firefox. This patch is really needed for PHP. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Hi all, I've made the patch for trunk. I checked by using session module tests using valgrind. https://gist.github.com/1379668 This patch adds - validate_sid() to ps_modules (Save handlers) - use_strict_session to php.ini (On by default, off for old behavior) - display that save handler supports strict session or not via phpinfo() (So that user could know behavior) - update PHP_SESSION_API version (So that save handler authors could write portable code) Compatibility issues are - save handlers that are currently working should also work with this patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro. These ps modules should implement validate_sid(). Modules that are using PS_MOD/PS_FUNCS are not affected, they just marked as adaptive module. (e.g. pecl sqlite's ps module. You can see it via phpinfo()) NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but it was the same as PS_MOD()/PS_MOD_FUNCS(). If old ps_module does not compile, just remove _SID from PS_MOD_SID()/PS_MOD_FUNCS_SID(). - session ID string is checked so that chars are alphanumeric + ',' '-' when use_strict_session=On. (mod_file.c checks session ID this way w/o this patch to prevent problems. Using restrictive session ID string is better for other ps modules. IMHO) - session read failure is not rescued to eliminate possible infinite loops. Bundled ps modules were not using this at least. You will see some tests are failing since they depend on adaptive session. By looking into failing test results, you can see this patch is generating new session ID if it's not a already initialized session. I'll modify these tests (set use_strict_session=Off) and add some tests for new feature (new validate_sid() function for user and use_class save handler) after commit. It removes session read failure retry code from php_session_initialize() . The retry code may cause infinite loop depending on save handler implementation. Some session save handlers may be using this to prevent adoption(?), but they should implement validate_sid() to prevent adoption from now on. With new code, there will never be infinite loops. Currently bundled save handlers are not using this feature. If there is no objection, I'll commit it to trunk. It will save much time if I don't have to write wiki for rfc. Comments are appreciated. -- Yasuo Ohgaki 2011/11/12 9:50 Rui Hirokawa rui_hirok...@yahoo.co.jp: Hi, I strongly recommend to submit the Strict session patch for php-src(HEAD) because the vulnerability of PHP against the session adoption/fixation attack is annoying issue of the PHP programmers for many years. I also suggest to apply this patch for PHP_5_4 after PHP 5.4.0 is released. For PHP 5.4.1, I suggest that the default value of session.use_strict_mode should be 0 (Off) to maintain the backward compatibility. Rui Yasuo Ohgaki wrote: Hi all, Few years ago, I have proposed strict session. It seems PHP 5.4 and php-src don't have protection against session adoption yet. Since there will be many TLDs, session adoption attack will be very easy for some domains until browsers support them. Even without new TLDs, attacker may place cookie file to attack session adoption or can exploit paths or domains, since there is no standard for precedence. e.g. Domain has more precedence over path on Chrome while path has greater precedence on IE. This is due to the order difference of sent cookies. (If there are multiple cookies are set for domain/path, only one became the outstanding cookie. I think PHP uses first IIRC while other implementation may use the last. Therefore, browser may not able to solve this issue, since it may destroy apps specific to browser) Even if a programmer sets path and domain for PHP session id cookie, attackers may exploit this to fix session id with session managers that are vulnerable to session adoption. If you don't get idea, play with cookie with/without domain/path is set/unset. You'll see how attacker may use session adoption. Default session module's configuration (domain not set, path set to /) is very easy to exploit anyway. I usually set both exact application domain/path, and unset all domain/path cookies for session id to prevent the attack. I guess this is not widely adopted. Even this is not enough. For example, if subdomain is added, Chrome has greater precedence for subdomain and attacker may exploit it. I pasted a patch for PHP 5.2 that rejects uninitialized session id. I think original patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to current PHP. If one would like to old behavior, he/she can just turn off the strict session. There are too many ways to exploit session with session adoption vulnerable session manger. Simple solution is making session manager strict. Any comments? -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP
Re: [PHP-DEV] Strict session?
On 11/22/2011 02:51 PM, Yasuo Ohgaki wrote: It will save much time if I don't have to write wiki for rfc. Please add at least the contents of your email to an RFC so there is some description that doesn't require trawling through mail archives to find. Chris -- Email: christopher.jo...@oracle.com Tel: +1 650 506 8630 Blog: http://blogs.oracle.com/opal/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
On Wed, Nov 23, 2011 at 12:31 AM, Christopher Jones christopher.jo...@oracle.com wrote: On 11/22/2011 02:51 PM, Yasuo Ohgaki wrote: It will save much time if I don't have to write wiki for rfc. Please add at least the contents of your email to an RFC so there is some description that doesn't require trawling through mail archives to find. Chris -- Email: christopher.jo...@oracle.com Tel: +1 650 506 8630 Blog: http://blogs.oracle.com/opal/ I don't know that Yasuo has wiki account or not, so I went ahead and created a draft(just dumping the info there): https://wiki.php.net/rfc/strict_sessions -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Strict session?
2011/11/23 Ferenc Kovacs tyr...@gmail.com: It will save much time if I don't have to write wiki for rfc. Please add at least the contents of your email to an RFC so there is some description that doesn't require trawling through mail archives to find. Thank you. I see you've made new page. I'll add description later. -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Strict session?
Ferenc,
I can login to wiki, but cannot write to the page.
https://wiki.php.net/rfc/strict_sessions
Could you update the page with attached file?
Thank you.
--
Yasuo Ohgaki
== Request for Comments: Strict Sessions ==
* Version: 0.1
* Date: 2011-11-23
* Author: Yasuo Ohgaki yohg...@ohgaki.net
* Status: Draft
* First Published at: https://wiki.php.net/rfc/strict_sessions
= Introduction =
https://gist.github.com/1379668
This patch adds
- validate_sid() to ps_modules (Save handlers)
- use_strict_session to php.ini (On by default, off for old behavior)
- display that save handler supports strict session or not via
phpinfo() (So that user could know behavior)
- update PHP_SESSION_API version (So that save handler authors could
write portable code)
Compatibility issues are
- save handlers that are currently working should also work with this
patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro.
These ps modules should implement validate_sid(). Modules that are
using PS_MOD/PS_FUNCS are not affected, they just marked as adaptive
module. (e.g. pecl sqlite's ps module. You can see it via phpinfo())
NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but
it was the same as PS_MOD()/PS_MOD_FUNCS(). If old ps_module does not
compile, just remove _SID from PS_MOD_SID()/PS_MOD_FUNCS_SID().
- session ID string is checked so that chars are alphanumeric + ','
'-' when use_strict_session=On.
(mod_file.c checks session ID this way w/o this patch to prevent
problems. Using restrictive session ID string is better for other ps
modules. IMHO)
- session read failure is not rescued to eliminate possible infinite
loops. Bundled ps modules were not using this at least.
You will see some tests are failing since they depend on adaptive
session. By looking into failing test results, you can see this patch
is generating new session ID if it's not a already initialized
session. I'll modify these tests (set use_strict_session=Off) and add
some tests for new feature (new validate_sid() function for user and
use_class save handler) after commit.
It removes session read failure retry code from
php_session_initialize() . The retry code may cause infinite loop
depending on save handler implementation. Some session save handlers
may be using this to prevent adoption(?), but they should implement
validate_sid() to prevent adoption from now on. With new code, there
will never be infinite loops. Currently bundled save handlers are not
using this feature.
If there is no objection, I'll commit it to trunk.
= Background =
Session is one of the most important feature to secure Web applications.
However, current PHP session module is weak to session ID adoption, thus it
is weak to session fixation.
Current Solution
Programmer can make adoptive session with user land code as follows.
Login code fragment: Code that adds session ID as validation key.
session_destory();
session_regenerate_id();
$_SESSION['vaild_id'] = session_id();
Validation code: Code other than login. Check if session is properly initilized.
if ($_SESSION['valid_id'] !== session_id()) {
die('Invalid use of session ID');
}
Alternatively, programmers may try to delete all possible cookies by sending
empty
session ID cookie.
Reason why the validation key is required
Cookie that is used for session allows multiple cookies for a single request.
When multiple cookies are set, browsers send multiple cookie header WITHOUT
domain and path
information. Browser just send cookie header and there is no way to know which
cookie is for
which domain or path.
In addtion, there are no standards for sending multiple cookie. For example, IE
has different
order preference for sending cookies than Chrome/Firefox.
This behavior prevents use of session_regenerat_id()'s new cookie in some
cases. PHP may
use invalid session ID to initialize session. Session ID can be fixed (i.e.
session fixation)
without the validation code.
Session fixation can be used to take over session and compromise Web
application security.
= Solution with this patch =
This patch adds validate_id() function to ps_modules (session save handler)
that can check
if the session ID is already initialized one or not. Bundled ps_modules are now
have validate
function and will never accept uninitialized session ID regardless cookie based
or URL rewriter.
It is still possible users may write improper save handler, but it is user's
responsibility that writes
proper save handler that prevents session adoption.
Compatibility
At user land, there will be no compatibility issues by disabling
session.use_strict_mode. (i.e. session.use_strict_mode=0)
There may be compatibility problem For internal session modules, if and only if
it uses PS_MOD_SID()/PS_MOD_FUNCS_SID()
for ps_module definition.
Module writers may use PHP_SESSION_API version id to write portable ps_modules.
=
Re: [PHP-DEV] Strict session?
On Wed, Nov 23, 2011 at 1:46 AM, Yasuo Ohgaki yohg...@ohgaki.net wrote: Ferenc, I can login to wiki, but cannot write to the page. https://wiki.php.net/rfc/strict_sessions Could you update the page with attached file? Thank you. -- Yasuo Ohgaki Sure, Hannes, could you please give karma to Yasuo so that he can edit the rfc himself? Thanks! -- Ferenc Kovács @Tyr43l - http://tyrael.hu
Re: [PHP-DEV] Strict session?
Hi, I strongly recommend to submit the Strict session patch for php-src(HEAD) because the vulnerability of PHP against the session adoption/fixation attack is annoying issue of the PHP programmers for many years. I also suggest to apply this patch for PHP_5_4 after PHP 5.4.0 is released. For PHP 5.4.1, I suggest that the default value of session.use_strict_mode should be 0 (Off) to maintain the backward compatibility. Rui Yasuo Ohgaki wrote: Hi all, Few years ago, I have proposed strict session. It seems PHP 5.4 and php-src don't have protection against session adoption yet. Since there will be many TLDs, session adoption attack will be very easy for some domains until browsers support them. Even without new TLDs, attacker may place cookie file to attack session adoption or can exploit paths or domains, since there is no standard for precedence. e.g. Domain has more precedence over path on Chrome while path has greater precedence on IE. This is due to the order difference of sent cookies. (If there are multiple cookies are set for domain/path, only one became the outstanding cookie. I think PHP uses first IIRC while other implementation may use the last. Therefore, browser may not able to solve this issue, since it may destroy apps specific to browser) Even if a programmer sets path and domain for PHP session id cookie, attackers may exploit this to fix session id with session managers that are vulnerable to session adoption. If you don't get idea, play with cookie with/without domain/path is set/unset. You'll see how attacker may use session adoption. Default session module's configuration (domain not set, path set to /) is very easy to exploit anyway. I usually set both exact application domain/path, and unset all domain/path cookies for session id to prevent the attack. I guess this is not widely adopted. Even this is not enough. For example, if subdomain is added, Chrome has greater precedence for subdomain and attacker may exploit it. I pasted a patch for PHP 5.2 that rejects uninitialized session id. I think original patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to current PHP. If one would like to old behavior, he/she can just turn off the strict session. There are too many ways to exploit session with session adoption vulnerable session manger. Simple solution is making session manager strict. Any comments? -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Strict session?
Hi all,
Few years ago, I have proposed strict session.
It seems PHP 5.4 and php-src don't have protection against session
adoption yet.
Since there will be many TLDs, session adoption attack will be
very easy for some domains until browsers support them.
Even without new TLDs, attacker may place cookie file to attack
session adoption or can exploit paths or domains, since there is no
standard for precedence.
e.g. Domain has more precedence over path on Chrome while
path has greater precedence on IE. This is due to the order difference
of sent cookies. (If there are multiple cookies are set for domain/path, only
one became the outstanding cookie. I think PHP uses first IIRC while other
implementation may use the last. Therefore, browser may not able to
solve this issue, since it may destroy apps specific to browser)
Even if a programmer sets path and domain for PHP session id cookie,
attackers may exploit this to fix session id with session managers that are
vulnerable to session adoption.
If you don't get idea, play with cookie with/without domain/path is set/unset.
You'll see how attacker may use session adoption. Default session module's
configuration (domain not set, path set to /) is very easy to exploit anyway.
I usually set both exact application domain/path, and unset all domain/path
cookies for session id to prevent the attack. I guess this is not
widely adopted.
Even this is not enough. For example, if subdomain is added, Chrome has
greater precedence for subdomain and attacker may exploit it.
I pasted a patch for PHP 5.2 that rejects uninitialized session id. I
think original
patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port to
current PHP. If one would like to old behavior, he/she can just turn off the
strict session.
There are too many ways to exploit session with session adoption vulnerable
session manger. Simple solution is making session manager strict.
Any comments?
--
Yasuo Ohgaki
yohg...@ohgaki.net
Index: ext/session/php_session.h
===
--- ext/session/php_session.h (リビジョン 288932)
+++ ext/session/php_session.h (作業コピー)
@@ -23,7 +23,7 @@
#include ext/standard/php_var.h
-#define PHP_SESSION_API 20020330
+#define PHP_SESSION_API 20051121
#define PS_OPEN_ARGS void **mod_data, const char *save_path, const
char *session_name TSRMLS_DC
#define PS_CLOSE_ARGS void **mod_data TSRMLS_DC
@@ -32,6 +32,7 @@
#define PS_DESTROY_ARGS void **mod_data, const char *key TSRMLS_DC
#define PS_GC_ARGS void **mod_data, int maxlifetime, int *nrdels TSRMLS_DC
#define PS_CREATE_SID_ARGS void **mod_data, int *newlen TSRMLS_DC
+#define PS_VALIDATE_SID_ARGS void **mod_data, const char *key TSRMLS_DC
/* default create id function */
PHPAPI char *php_session_create_id(PS_CREATE_SID_ARGS);
@@ -45,6 +46,7 @@
int (*s_destroy)(PS_DESTROY_ARGS);
int (*s_gc)(PS_GC_ARGS);
char *(*s_create_sid)(PS_CREATE_SID_ARGS);
+int (*s_validate_sid)(PS_VALIDATE_SID_ARGS);
} ps_module;
#define PS_GET_MOD_DATA() *mod_data
@@ -57,6 +59,7 @@
#define PS_DESTROY_FUNC(x) int ps_delete_##x(PS_DESTROY_ARGS)
#define PS_GC_FUNC(x) int ps_gc_##x(PS_GC_ARGS)
#define PS_CREATE_SID_FUNC(x) char *ps_create_sid_##x(PS_CREATE_SID_ARGS)
+#define PS_VALIDATE_SID_FUNC(x)int
ps_validate_sid_##x(PS_VALIDATE_SID_ARGS)
#define PS_FUNCS(x) \
PS_OPEN_FUNC(x); \
@@ -65,11 +68,12 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x); \
+PS_VALIDATE_SID_FUNC(x)
#define PS_MOD(x) \
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \
-ps_delete_##x, ps_gc_##x, php_session_create_id
+ps_delete_##x, ps_gc_##x, php_session_create_id, ps_validate_sid_##x
/* SID enabled module handler definitions */
#define PS_FUNCS_SID(x) \
@@ -79,11 +83,12 @@
PS_WRITE_FUNC(x); \
PS_DESTROY_FUNC(x); \
PS_GC_FUNC(x); \
- PS_CREATE_SID_FUNC(x)
+ PS_CREATE_SID_FUNC(x); \
+PS_VALIDATE_SID(x)
#define PS_MOD_SID(x) \
#x, ps_open_##x, ps_close_##x, ps_read_##x, ps_write_##x, \
-ps_delete_##x, ps_gc_##x, ps_create_sid_##x
+ps_delete_##x, ps_gc_##x, ps_create_sid_##x, ps_validate_sid_##x
typedef enum {
php_session_disabled,
@@ -121,6 +126,7 @@
zend_bool use_only_cookies;
zend_bool use_trans_sid;/* contains the INI value of
whether to use
trans-sid */
zend_bool apply_trans_sid; /* whether or not to enable trans-sid for
the current request */
+zend_bool use_strict_mode; /* whether or not PHP accepts
unknown session ids */
long hash_func;
long hash_bits_per_character;
Index: ext/session/tests/session_commit_variation4.phpt
===
--- ext/session/tests/session_commit_variation4.phpt(リビジョン 288932)
+++
