Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:56718 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 4740 invoked from network); 2 Dec 2011 09:38:57 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 2 Dec 2011 09:38:57 -0000 Authentication-Results: pb1.pair.com smtp.mail=tyra3l@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=tyra3l@gmail.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.42 as permitted sender) X-PHP-List-Original-Sender: tyra3l@gmail.com X-Host-Fingerprint: 209.85.216.42 mail-qw0-f42.google.com Received: from [209.85.216.42] ([209.85.216.42:42627] helo=mail-qw0-f42.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id D5/C0-33712-EAC98DE4 for ; Fri, 02 Dec 2011 04:38:55 -0500 Received: by qabg40 with SMTP id g40so278773qab.8 for ; Fri, 02 Dec 2011 01:38:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=LSWyC1C/1swKu8J3WjQjJzCEuAZgMbXPl8paXSY7Jz4=; b=DIza7Thz4mj52BXIOJRG+9unSEqhcii8hQQXKeekV1uSzs2rFHhVepUSET9mhn/NUI 69DtKpM9yAdIjwKfBsHHC+MnDpt7j8AMNkp6VkrY03vSVY+GLwEH1S4Cx/O90Fqxkh0t LQJ9c6fScb8s5HGRLl9fz9edM7HTk+v3ilJ4Q= MIME-Version: 1.0 Received: by 10.224.181.141 with SMTP id by13mr2303807qab.1.1322818732150; Fri, 02 Dec 2011 01:38:52 -0800 (PST) Received: by 10.229.38.134 with HTTP; Fri, 2 Dec 2011 01:38:52 -0800 (PST) In-Reply-To: References: <4EBDC283.3040700@yahoo.co.jp> <4ED727FB.7030001@uw.no> <4ED76013.50601@lerdorf.com> <4ED85560.6040101@uw.no> <4ED8805B.7070308@uw.no> Date: Fri, 2 Dec 2011 10:38:52 +0100 Message-ID: To: Yasuo Ohgaki Cc: "Daniel K." , Rasmus Lerdorf , internals@lists.php.net Content-Type: multipart/alternative; boundary=20cf30334c87a1d08a04b318bc8b Subject: Re: [PHP-DEV] Strict session? From: tyra3l@gmail.com (Ferenc Kovacs) --20cf30334c87a1d08a04b318bc8b Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Fri, Dec 2, 2011 at 10:31 AM, Yasuo Ohgaki wrote: > Hi Daniel, > > 2011/12/2 Daniel K. : > > Yasuo Ohgaki wrote: > >> > >> 2011/12/2 Daniel K. : > >>> > >>> Yasuo Ohgaki wrote: > >>>> > >>>> 2011/12/2 Yasuo Ohgaki : > >>> > >>> 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 handler= s > >>>>> may be using this to prevent adoption(?) > >>> > >>> > >>> I do not believe this could have been used as you speculate. The retr= y > >>> 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 b= e > > 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 val= id > >>> 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 o= wn > > 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=3D0. > > > > > > 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 =3D=3D 'contact')) { > if (session_id() !=3D '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=3D1 for trunk and > use_strict_session=3D0 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 > yohgaki@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. --=20 Ferenc Kov=C3=A1cs @Tyr43l - http://tyrael.hu --20cf30334c87a1d08a04b318bc8b--