[RFC Discussion] Precise Session Management

Personally I think it’s a bad way to handle such thing. Adding yet another „magic“ keyword/reserved field is going to make current situation worse. I know users should not use names starting with __, but in reality I see them almost everyday. I even seen __SESSION_INTERNAL__ used once. — Grzegorz Zdanowski

Hi all, On Tue, Jan 26, 2016 at 11:22 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

I didn't pay attention that current session_id() allows any chars for session ID while session module and save handlers explicitly disallow characters for security reasons. e.g. Session module silently removes HTML special chars, files save handler does not allow PATH special chars. Current situation is not good at all. Since this RFC is about preciseness of session management, I would like to change session_id() validates against default allowed chars as follows. (As well as enabling already written session_create_id() function) This patch is against the PR. Currently, the use of "PHPAPI php_session_valid_chars()" is up to save handler, but it should be checked always by session module. Since the function only allows chars used by ID, I would like to add "_" a valid char. "_" should be very safe char. These changes for session ID would be the largest BC of this RFC. Any comments for this change? diff --git a/ext/session/session.c b/ext/session/session.c index 7b3203a..fdc6a91 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -566,7 +566,7 @@ PHPAPI int php_session_valid_key(const char *key) /* {{{ */ /* Somewhat arbitrary length limit here, but should be way more than anyone needs and avoids file-level warnings later on if we exceed MAX_PATH */ - if (len == 0 || len > 128) { + if (len == 0 || len > PHP_SESSION_KEY_MAX_LEN) { ret = FAILURE; } @@ -2460,31 +2460,35 @@ static PHP_FUNCTION(session_save_path) Return the current session id. If newid is given, the session id is replaced with newid */ static PHP_FUNCTION(session_id) { - zend_string *name = NULL; + zend_string *id = NULL; int argc = ZEND_NUM_ARGS(); - if (zend_parse_parameters(argc, "|S", &name) == FAILURE) { + if (zend_parse_parameters(argc, "|S", &id) == FAILURE) { return; } if (PS(id)) { - /* keep compatibility for "\0" characters ??? - * see: ext/session/tests/session_id_error3.phpt */ - size_t len = strlen(ZSTR_VAL(PS(id))); - if (UNEXPECTED(len != ZSTR_LEN(PS(id)))) { - RETVAL_NEW_STR(zend_string_init(ZSTR_VAL(PS(id)), len, 0)); - } else { - RETVAL_STR_COPY(PS(id)); - } + RETVAL_STR_COPY(PS(id)); } else { RETVAL_EMPTY_STRING(); } - if (name) { + if (id) { if (PS(id)) { zend_string_release(PS(id)); } - PS(id) = zend_string_copy(name); + if (php_session_valid_key(ZSTR_VAL(id)) == FAILURE) { + zend_string *new_id; + /* Set random ID for security reason. */ + php_error_docref(NULL, E_WARNING, + "Session ID cannot exceed session max length %d " + "and/or contain special characters. Only aphanumeric, ',', '-' are allowed " + "Random session ID is set", + PHP_SESSION_KEY_MAX_LEN); + PS(id) = php_session_create_id(NULL); + } else { + PS(id) = zend_string_copy(id); + } } } /* }}} */ @@ -2519,7 +2523,6 @@ static PHP_FUNCTION(session_regenerate_id) /* {{{ proto void session_create_id([string prefix]) Generate new session ID. Intended for user save handlers. */ -#if 0 /* This is not used yet */ static PHP_FUNCTION(session_create_id) { @@ -2531,34 +2534,34 @@ static PHP_FUNCTION(session_create_id) } if (prefix && ZSTR_LEN(prefix)) { - if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) { - /* E_ERROR raised for security reason. */ - php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only aphanumeric, ',', '-' are allowed"); - RETURN_FALSE; + if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE + || ZSTR_LEN(prefix) > PHP_SESSION_KEY_MAX_LEN/2) { + /* Do not use prefix for security reason. */ + php_error_docref(NULL, E_WARNING, + "Prefix cannot exceed session max length %d " + "and/or contain special characters. Only aphanumeric, ',', '-' are allowed", + PHP_SESSION_KEY_MAX_LEN/2); } else { smart_str_append(&id, prefix); } } - if (PS(session_status) == php_session_active) { - new_id = PS(mod)->s_create_sid(&PS(mod_data)); - } else { - new_id = php_session_create_id(NULL); - } + /* Should call the default always to avoid problems with user handler */ + new_id = php_session_create_id(NULL); if (new_id) { smart_str_append(&id, new_id); zend_string_release(new_id); } else { smart_str_free(&id); - php_error_docref(NULL, E_WARNING, "Failed to create new ID"); + /* E_RECOVERBELE_ERROR raised for security reason. */ + php_error_docref(NULL, E_RECOVERABLE_ERROR, "Failed to create new ID"); RETURN_FALSE; } smart_str_0(&id); RETVAL_NEW_STR(id.s); smart_str_free(&id); } -#endif /* }}} */ /* {{{ proto string session_cache_limiter([string new_cache_limiter]) @@ -2901,6 +2904,10 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_session_id, 0, 0, 0) ZEND_ARG_INFO(0, id) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_INFO_EX(arginfo_session_create_id, 0, 0, 0) + ZEND_ARG_INFO(0, prefix) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_INFO_EX(arginfo_session_regenerate_id, 0, 0, 0) ZEND_ARG_INFO(0, delete_old_session) ZEND_END_ARG_INFO() @@ -2989,6 +2996,7 @@ static const zend_function_entry session_functions[] = { PHP_FE(session_module_name, arginfo_session_module_name) PHP_FE(session_save_path, arginfo_session_save_path) PHP_FE(session_id, arginfo_session_id) + PHP_FE(session_create_id, arginfo_session_create_id) PHP_FE(session_regenerate_id, arginfo_session_regenerate_id) PHP_FE(session_decode, arginfo_session_decode) PHP_FE(session_encode, arginfo_session_void)

Hi!

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. 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. 3. Not sure what function NEW_SID serves? Why we need to know new ID in the old session? 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. 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. 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? 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.? Thanks,