Hi Stas, TL;DR; for others. Those who have no idea why this RFC is mandatory and how current session management is broken, please read the RFC's TL;DR;.
On Tue, Jan 26, 2016 at 5:01 PM, Stanislav Malyshev <smalys...@gmail.com> wrote: >> https://wiki.php.net/rfc/precise_session_management >> https://github.com/php/php-src/pull/1734 > > I'm re-reading this RFC and I notice it does quite a lot of things: > - Add five new INI values > - Add two new functions > - Changes behavior of two widely used functions > - Changes four different defaults > - Adds magic data to session > > This alone makes it very hard to figure out what is going on. It looks > like "kitchen sink" RFC instead of targeted one. I appreciate very much > your efforts to make sessions more secure, but it is very hard to review > such big thing with so many things being changed at the same time. And > it looks like not only myself think this way, judging by amount of > feedback this is getting. Maybe I just don't understand bigger picture > and all this is united by some common goal but then I'd like to have > some description of this goal which will enhance my understanding. > > Going through RFC, here are the things that are not clear to me: > > 1. Why we need to change session_regenerate_id() ? It has session delete > option already. You may argue it's not safe to not delete, but then you > can opt to delete. I can even see adding option to expire session after > a short period, but what is proposed looks a bit overcomplicated. Previous proposal was much simpler than this because it addressed only destroy duration issue. It was rejected even if it was simple. I think difficulty is not about proposal volume, but basic understanding about what is broken in current session. Many of issues that are reported could be resolved by this, but it's not complete as this proposal explains. > 2. Why you need to keep session create and update time? IIRC we already > have the time of last use (that's how GC) works, and we could have > expiration timestamp for scenarios like session_regenerate_id() but I > don't see why we need update TS in session, especially one that changes, > and same for session.ttl_update. CREATED is used to determine if the session should be renewed. i.e. session_regenerate_id() UPDATED is used to determine if the session is expired or not. "UPDATED+ttl < now" is expired. UPDATED timestamp is _not_ updated always not to disturb lazy_write. UPDATED is updated when "UPDATED + ttl_update < now" is true. For most save handlers, gc's timestamping is not precise enough for gc. gc timestamp is updated even for obsolete session also. It cannot replace UPDATED timestamp. > 3. Not sure what function NEW_SID serves? Why we need to know new ID in > the old session? Browser may not get new session ID always due to bad network, etc. e.g. Server send new session ID cookie, but connection is lost, then browser will continue to use old session cookie ID. This is one of the cause of random lost sessions. My patch try to resend new session ID once for this case. This will reduce number of lost sessions close to zero, hopefully. > > 4. Not sure how session.ttl would work. Would session expire in that > time regardless of usage? Then it should be unset by default. I > understand there is a use case for hard expiration, but that is separate > functionality and I'm not even sure it should be in the same RFC. It's not hard expiration. I think this should be done in user space. UPDATED will be updated by specified frequency(ttl_update). Session expiration is checked "UPDATED + ttl < now". > > 5. Same for session.regenerate_id - that seems to be a piece of > functionality not related to anything else. How it is connected to the > rest of the RFC? In any case, I don't see why it should be enabled by > default - RFC does not explain it. Same for session.num_sids and SIDS > value - does not explain what is the use case for it and why it is > necessary. Users must regenerate session ID for security reasons. Even if periodic session ID regeneration is mandatory for security, there are many codes that don't do this. Because it is mandatory, the feature should be part of session management. Old session IDs are needed because users may use session IDs for security protections. CSRF protection key is common usage. e.g. $csrf_key = sha1(session_id() . $mysecret) something like that. If user sets session ID regeneration for every 30 minutes, 8 session ID is good enough for 4 hours. BTW, I'm not recommending this CSRF protection method. There is better ways for it. > > 6. Changes like "Use strict mode by default (disallow stealing session > forever). Use httponly cookie by default. Use SHA1 as hash (and use 5 > bits for hashed value string for better compatibility)" are not > described further and not clear how they are connected to the rest - is > it necessary for the rest of the RFC to work? Why? This RFC target is precise (~= secure) session management. This is the reason why these are included. (As well as session_id() accepts any chars, but we agreed to work on this in another RFC) > > Again, myself and I think others appreciate very much your efforts of > making sessions better, but such big changes in a function that > underlies almost every PHP application should be done very carefully and > with full understanding of what is going on, and doing it in a huge > package may be hard. I wonder maybe it'd be better to make RFC do one > necessary thing - e.g. handle session regeneration - and then proceed to > other things? Can we identify one problem we are trying to fix, define > functionality that fixes it, agree on it, and then proceed to the next > problem, etc.? If you see the patch, there are many phpt changes, but code and logic is rather straight forward. I've already have/will have 3 RFC for session. This one, session_id() and user space serialize handler. https://github.com/php/php-src/pull/1732 I would like not to have too many RFCs for session. As you can see from patches, the more I have RFC, the more works for phpt adjustment are required _separately_. Besides writing and managing RFC, it consumes my time a lot. I'm certain that changes proposed by this RFC are mandatory or almost mandatory for better security and session stability. So I hope you feel fine with this RFC by this explanation. If you have further questions, it's welcomed. The logic is written in PHP pseudo code. I'll appreciate if one could find defect if there is. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
