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 (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
>
>
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.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
