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, \
> ps_delete_##x, ps_gc_##x, php_session_create_id
>
>
> #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
>
>> - session ID string is checked so that chars are alphanumeric + ','
>> '-' when use_strict_session=On.
>
>
> php_session_initialize() checks for some additional invalid characters as
> well. Why be strict and allow only [0-9a-zA-Z,-], different storage backends
> have different requirements.
It already explained before. It's for precaution. Is there any good
for allowing useless chars for hashes?
It checks within ps_module, so ps_module writer can use any chars other than
/* check session name for invalid characters */
if (PS(id) && strpbrk(PS(id), "\r\n\t <>'\"\\")) {
>
>
>
>> (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)
>
>
> Why is it better? And if it is, could the function be made generic instead
> of it being copied around, bugs and all?
It's possible to restrict valid chars. But as you said, different
ps_module may have different needs.
>
>
>
>> - session read failure is not rescued to eliminate possible infinite
>> loops. Bundled ps modules were not using this at least.
>
>
> More on loops below.
I could not understand what you mean.
New code removes possible infinite loop.
>> 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.
>
>
> Why not at the same time? It seems appropriate to prove that you have tested
> the new features when you commit them?
It's easier to understand that is going on with diffs, isn't it?
Simply adding bunch of INI entries is enough to make failing tests work.
>
>
>
>> 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)
>
> This could never work with:
>
> session_id("foo");
> session_start();
>
> could it?
Of course it will, if not let me know.
Did you disabled strict session?
session.use_strict_session=0
>
> Have you checked that this still works as advertised with the patch applied?
Few tests has modified INI section so that they use
session.use_strict_session=0
>
>
> I may have additional comments if I can find time to test it.
Thanks.
Please let me know if you find any.
Regards,
--
Yasuo Ohgaki
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
