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');
session_cache_limiter('nocache');
session_start();
}
}
or code that generate ID by their own like session_id(md5(uniqid()).
BTW, the default would be use_strict_session=1 for trunk and
use_strict_session=0 for PHP_5_4 (and PHP_5_3?) Current users are not
affected as long as they are using the default. Users should know the
risk of session adoption/fixation, though.
"session_id('contact')" / "session_id(md5(uniqid())" are bad code, but
they have plenty time to fix it. I don't think having session_id() is
bad, but it could be abused. Prohibiting setting session ID under
strict mode would be better for security, IMHO.
Regards,
--
Yasuo Ohgaki
yohg...@ohgaki.net
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
