Hi Stas, On Wed, Jan 27, 2016 at 9:12 AM, Stanislav Malyshev <smalys...@gmail.com> wrote: > > >> CREATED is used to determine if the session should be renewed. >> i.e. session_regenerate_id() > > This looks like something that belongs in userspace.
As my PHP like pseudo code illustrates, this could be done by user script. However, severe issues described in the RFC will happens without proposed change. > >> UPDATED is used to determine if the session is expired or not. >> "UPDATED+ttl < now" is expired. > > We already have GC mechanism that does that, why duplicate it? Keeping timestamp for TTL and timestamp for GC is different thing. TTL is for exact/precise expiration management. GC is for discarding garbage. GC cannot be a replacement of TTL management. > >> 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. > > I'm sorry, I don't understand it - why it's not precise enough? It's a > regular unix timestamp AFAIK, so it's as precise as the rest of > timestamps everywhere. > I am also not sure what you mean by "obsolete session" and why updating > its timestamp is a problem. GC is depended on probability for most save handlers. Relying on probability is not precise by its nature. In addition, garbage sessions are _active_ sessions. Because it is active, garbage session can be exploited by attackers. > >> 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. > > I don't think we should try to fix bad network in PHP. If you didn't get > new session ID, then it's lost and the user will get a new login screen > and will log in again. That's what happens when you have bad network. We > shouldn't add complexity into PHP to fix bad networks. IMHO. It's not a task of user space framework. Any decent session manager must manage sessions in reliable manner. Leaving unreliable PHP session module alone is our fault, IMO. > >> My patch try to resend new session ID once for this case. This will >> reduce number of lost sessions close to zero, hopefully. > > What if you have bad network twice? If it's bad, then it's bad. Can > happen more than once. True. There is a chance that this mitigation fails. However, I observed good enough results from a site processing millions requests per day. > >> 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". > > In this case, we already have gc lifetime parameter. As explained. GC != TTL. > >> 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. > > I think this belongs in userspace. And I don't see it as mandatory - for > some applications, it's perfectly fine to keep long sessions. For > others, it's not. Simple calls of session_regenerate_id() result in lost sessions and/or exploitable sessions. Since there is way to avoid it, why not implement it? Leaving users have hard to debug bugs is not nice approach. > >> 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. > > That seems to be bringing a particular implementation of a particular > software into core. This does not look like a good design. If your CSRF > handling requires keeping old IDs, by all means keep them, but in core > nothing requires that. Precise session management is not done in userspace, nor perfect CSRF. Since there is ways for this, we are better to provide them out of the box. I think this discussion proved that precise session data management is hard enough to be implemented by users. > >>> 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) > > It would still be good to explain why those must be defaults and why > current defaults are not fine. > I can see strict mode by default being ok. httponly cookie is > questionable, as it may break some JS apps that may need access to > session cookie. But we may put it to a vote and see what people think. > For hash function, I don't see how it matters even a little bit, given > that session IDs are random. Httponly is best practice and should be the default. If user code has problem with best practice, they should disable it by their own responsibility. Encouraging best practice by default is our responsibility, IMO. Since I've changed session module to detect collisions, hash function is not too important. However, we have SHA3 already. MD5 is too obsolete. Even SHA1 is obsolete, but it's better than MD5. I don't mind change the default to SHA2, but it could fail because hash module could be disabled. SHA2 also increase size of session ID, too. This may break apps store session ID in a DB table sess_id CHAR(32) > >> If you see the patch, there are many phpt changes, but code and >> logic is rather straight forward. > > Many phpt changes - if those are changes to existing phpt - usually > means a lot of BC breakage. That is exactly what I am concerned with. > There's no point in releasing new version which nobody would use because > it breaks all their applications. Changes in phpt does not mean BC in production code. If you understand changes behind it, you will see it does not affect production code. Tests codes verify internal behaviors/data, immediately removes unneeded sessions, etc. These are the main reasons why many tests are updated. > >> 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. > > In this case, discussing so many unrelated changes in one RFC is worse, > IMO, since it is hugely distracting and very hard to keep track of, and > very easy to forget something. Unless there is a framework that unites > those changes, but so far I didn't see it in the RFC, I just saw a lot > of independent changes. This RFC is for preciseness( ~= better security) Other RFC is not appropriate RFC for changes like INI values. Besides, INI changes are trivial and should not disturb patch review. > >> 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. > > That's why I suggested to do a focused effort instead of doing too many > things at once. This RFC and patch is focus on improving loose session management. I agree it includes minor changes such INI default change, but it should not be obstacles for code review. > >> I'm certain that changes proposed by this RFC are mandatory or almost >> mandatory for better security and session stability. > > Unfortunately, I am not sure yet it was proven that they are mandatory. > Many of them pertain to very specific scenarios of very specific > implementations and as such, would be much better done in userspace. Because we have many users who noticed random lost session even if it happens 1 in a million. I think it's good enough proof. IMHO, it's not a users responsibility session_regenerate_id() results in lost sessions and/or leave exploitable sessions. Anyway, I carefully considered BC impacts. Normal applications should not have problems with this proposal. If any, I'll consider resolution so please try to use it and let me know. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
