From b60e2f7c9fa21bd00667223f68e18eee865eab75 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 12 Aug 2022 22:15:02 +0100 Subject: [PATCH 01/10] Convert some Session globals from char* to zend_string* However, the code is somewhat disgusting as I'm hitting Zend/zend_types.h:1222: zend_gc_delref: Assertion (zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7) failed. Failures that I don't understand. --- ext/session/php_session.h | 10 +++---- ext/session/session.c | 55 ++++++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/ext/session/php_session.h b/ext/session/php_session.h index dfa7632e5a447..a532da34ae13a 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -142,14 +142,14 @@ typedef struct _php_ps_globals { char *save_path; char *session_name; zend_string *id; - char *extern_referer_chk; - char *cache_limiter; + zend_string *extern_referer_chk; + zend_string *cache_limiter; zend_long cookie_lifetime; - char *cookie_path; - char *cookie_domain; - char *cookie_samesite; + zend_string *cookie_path; + zend_string *cookie_domain; bool cookie_secure; bool cookie_httponly; + zend_string *cookie_samesite; const ps_module *mod; const ps_module *default_mod; void *mod_data; diff --git a/ext/session/session.c b/ext/session/session.c index 55a2265df7be9..20e89dd3f6775 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -757,11 +757,14 @@ static PHP_INI_MH(OnUpdateSessionLong) /* {{{ */ /* }}} */ -static PHP_INI_MH(OnUpdateSessionString) /* {{{ */ +static PHP_INI_MH(OnUpdateSessionStr) /* {{{ */ { SESSION_CHECK_ACTIVE_STATE; SESSION_CHECK_OUTPUT_STATE; - return OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); + + zend_string **p = (zend_string **) ZEND_INI_GET_ADDR(); + *p = new_value ? new_value : NULL; + return SUCCESS; } /* }}} */ @@ -923,16 +926,16 @@ PHP_INI_BEGIN() STD_PHP_INI_ENTRY("session.gc_maxlifetime", "1440", PHP_INI_ALL, OnUpdateSessionLong, gc_maxlifetime, php_ps_globals, ps_globals) PHP_INI_ENTRY("session.serialize_handler", "php", PHP_INI_ALL, OnUpdateSerializer) STD_PHP_INI_ENTRY("session.cookie_lifetime", "0", PHP_INI_ALL, OnUpdateCookieLifetime,cookie_lifetime, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionString, cookie_path, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionString, cookie_domain, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionStr, cookie_path, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_domain, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_httponly", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionString, cookie_samesite, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_samesite, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateRefererCheck, extern_referer_chk, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cache_limiter", "nocache", PHP_INI_ALL, OnUpdateSessionString, cache_limiter, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateSessionStr, extern_referer_chk, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cache_limiter", "nocache", PHP_INI_ALL, OnUpdateSessionStr, cache_limiter, php_ps_globals, ps_globals) STD_PHP_INI_ENTRY("session.cache_expire", "180", PHP_INI_ALL, OnUpdateSessionLong, cache_expire, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_trans_sid", "0", PHP_INI_ALL, OnUpdateUseTransSid, use_trans_sid, php_ps_globals, ps_globals) PHP_INI_ENTRY("session.sid_length", "32", PHP_INI_ALL, OnUpdateSidLength) @@ -1352,7 +1355,7 @@ static int php_session_cache_limiter(void) /* {{{ */ { const php_session_cache_limiter_t *lim; - if (PS(cache_limiter)[0] == '\0') return 0; + if (!PS(cache_limiter) || ZSTR_LEN(PS(cache_limiter)) == 0) return 0; if (PS(session_status) != php_session_active) return -1; if (SG(headers_sent)) { @@ -1362,7 +1365,8 @@ static int php_session_cache_limiter(void) /* {{{ */ } for (lim = php_session_cache_limiters; lim->name; lim++) { - if (!strcasecmp(lim->name, PS(cache_limiter))) { + // TODO Use zend_string_cmp API? + if (!strcasecmp(lim->name, ZSTR_VAL(PS(cache_limiter)))) { lim->func(); return 0; } @@ -1464,14 +1468,14 @@ static zend_result php_session_send_cookie(void) /* {{{ */ } } - if (PS(cookie_path)[0]) { + if (PS(cookie_path) && ZSTR_LEN(PS(cookie_path)) != 0) { smart_str_appends(&ncookie, COOKIE_PATH); - smart_str_appends(&ncookie, PS(cookie_path)); + smart_str_append(&ncookie, PS(cookie_path)); } - if (PS(cookie_domain)[0]) { + if (PS(cookie_domain) && ZSTR_LEN(PS(cookie_domain)) != 0) { smart_str_appends(&ncookie, COOKIE_DOMAIN); - smart_str_appends(&ncookie, PS(cookie_domain)); + smart_str_append(&ncookie, PS(cookie_domain)); } if (PS(cookie_secure)) { @@ -1482,9 +1486,9 @@ static zend_result php_session_send_cookie(void) /* {{{ */ smart_str_appends(&ncookie, COOKIE_HTTPONLY); } - if (PS(cookie_samesite)[0]) { + if (PS(cookie_samesite) && ZSTR_LEN(PS(cookie_samesite)) != 0) { smart_str_appends(&ncookie, COOKIE_SAMESITE); - smart_str_appends(&ncookie, PS(cookie_samesite)); + smart_str_append(&ncookie, PS(cookie_samesite)); } smart_str_0(&ncookie); @@ -1687,12 +1691,12 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ } /* Check whether the current request was referred to by * an external site which invalidates the previously found id. */ - if (PS(id) && PS(extern_referer_chk)[0] != '\0' && + if (PS(id) && PS(extern_referer_chk) && ZSTR_LEN(PS(extern_referer_chk)) != 0 && !Z_ISUNDEF(PG(http_globals)[TRACK_VARS_SERVER]) && (data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "HTTP_REFERER", sizeof("HTTP_REFERER") - 1)) && Z_TYPE_P(data) == IS_STRING && Z_STRLEN_P(data) != 0 && - strstr(Z_STRVAL_P(data), PS(extern_referer_chk)) == NULL + strstr(Z_STRVAL_P(data), ZSTR_VAL(PS(extern_referer_chk))) == NULL ) { zend_string_release_ex(PS(id), 0); PS(id) = NULL; @@ -1957,11 +1961,15 @@ PHP_FUNCTION(session_get_cookie_params) array_init(return_value); add_assoc_long(return_value, "lifetime", PS(cookie_lifetime)); - add_assoc_string(return_value, "path", PS(cookie_path)); - add_assoc_string(return_value, "domain", PS(cookie_domain)); + // TODO Use add_assoc_str() but figure out why it emits a + // Zend/zend_types.h:1222: zend_gc_delref: Assertion `(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)' failed. + add_assoc_string(return_value, "path", ZSTR_VAL(PS(cookie_path))); + add_assoc_string(return_value, "domain", ZSTR_VAL(PS(cookie_domain))); add_assoc_bool(return_value, "secure", PS(cookie_secure)); add_assoc_bool(return_value, "httponly", PS(cookie_httponly)); - add_assoc_string(return_value, "samesite", PS(cookie_samesite)); + // TODO Use add_assoc_str() but figure out why it emits a + // Zend/zend_types.h:1222: zend_gc_delref: Assertion `(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)' failed. + add_assoc_string(return_value, "samesite", ZSTR_VAL(PS(cookie_samesite))); } /* }}} */ @@ -2536,7 +2544,12 @@ PHP_FUNCTION(session_cache_limiter) RETURN_FALSE; } - RETVAL_STRING(PS(cache_limiter)); + zend_string *result_str = PS(cache_limiter); + if (!result_str) { + result_str = zend_empty_string; + } + // TODO Prevent duplication??? + RETVAL_STR(zend_string_dup(result_str, false)); if (limiter) { ini_name = ZSTR_INIT_LITERAL("session.cache_limiter", 0); From 7c04d833d33a6b6f93d59dd3f4bf10a925c13d21 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 12 Aug 2022 22:56:23 +0100 Subject: [PATCH 02/10] Convert session_name from char to zend_string Also fixed a bug where nul bytes where not properly checked --- ext/session/php_session.h | 2 +- ext/session/session.c | 100 ++++++++++++------ ext/session/tests/bug66481.phpt | 2 +- ext/session/tests/rfc1867_sid_post.phpt | 2 +- .../tests/session_name_variation1.phpt | 4 +- 5 files changed, 71 insertions(+), 39 deletions(-) diff --git a/ext/session/php_session.h b/ext/session/php_session.h index a532da34ae13a..39d5a2f1d7fbe 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -140,7 +140,7 @@ typedef struct _php_session_rfc1867_progress { typedef struct _php_ps_globals { char *save_path; - char *session_name; + zend_string *session_name; zend_string *id; zend_string *extern_referer_chk; zend_string *cache_limiter; diff --git a/ext/session/session.c b/ext/session/session.c index 20e89dd3f6775..73949bac8ae86 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -439,7 +439,7 @@ static zend_result php_session_initialize(void) /* {{{ */ } /* Open session handler first */ - if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE + if (PS(mod)->s_open(&PS(mod_data), PS(save_path), ZSTR_VAL(PS(session_name))) == FAILURE /* || PS(mod_data) == NULL */ /* FIXME: open must set valid PS(mod_data) with success */ ) { php_session_abort(); @@ -704,16 +704,31 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ SESSION_CHECK_ACTIVE_STATE; SESSION_CHECK_OUTPUT_STATE; - /* Numeric session.name won't work at all */ - if ((!ZSTR_LEN(new_value) || is_numeric_string(ZSTR_VAL(new_value), ZSTR_LEN(new_value), NULL, NULL, 0))) { - int err_type; + int err_type; - if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) { - err_type = E_WARNING; - } else { - err_type = E_ERROR; - } + if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) { + err_type = E_WARNING; + } else { + err_type = E_ERROR; + } + if (ZSTR_LEN(new_value) == 0) { + /* Do not output error when restoring ini options. */ + if (stage != ZEND_INI_STAGE_DEACTIVATE) { + php_error_docref(NULL, err_type, "session.name \"%s\" cannot be empty", ZSTR_VAL(new_value)); + } + return FAILURE; + } + /* Nul bytes are not allowed */ + if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { + /* Do not output error when restoring ini options. */ + if (stage != ZEND_INI_STAGE_DEACTIVATE) { + php_error_docref(NULL, err_type, "session.name \"%s\" cannot contain nul bytes", ZSTR_VAL(new_value)); + } + return FAILURE; + } + /* Numeric session.name won't work at all */ + if (is_numeric_str_function(new_value, NULL, NULL)) { /* Do not output error when restoring ini options. */ if (stage != ZEND_INI_STAGE_DEACTIVATE) { php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric or empty", ZSTR_VAL(new_value)); @@ -721,7 +736,9 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ return FAILURE; } - return OnUpdateStringUnempty(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); + zend_string **p = (zend_string **) ZEND_INI_GET_ADDR(); + *p = new_value; + return SUCCESS; } /* }}} */ @@ -1394,9 +1411,10 @@ static void php_session_remove_cookie(void) { size_t session_cookie_len; size_t len = sizeof("Set-Cookie")-1; - ZEND_ASSERT(strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) == NULL); - spprintf(&session_cookie, 0, "Set-Cookie: %s=", PS(session_name)); + ZEND_ASSERT(strpbrk(ZSTR_VAL(PS(session_name)), SESSION_FORBIDDEN_CHARS) == NULL); + spprintf(&session_cookie, 0, "Set-Cookie: %s=", ZSTR_VAL(PS(session_name))); + // TODO Manually compute from known information? session_cookie_len = strlen(session_cookie); current = l->head; while (current) { @@ -1434,9 +1452,10 @@ static zend_result php_session_send_cookie(void) /* {{{ */ return FAILURE; } + // TODO need to Check for nul byte? /* Prevent broken Set-Cookie header, because the session_name might be user supplied */ - if (strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) != NULL) { /* man isspace for \013 and \014 */ - php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,;.[ \\t\\r\\n\\013\\014'"); + if (strpbrk(ZSTR_VAL(PS(session_name)), SESSION_FORBIDDEN_CHARS) != NULL) { /* man isspace for \013 and \014 */ + php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,; \\t\\r\\n\\013\\014'"); return FAILURE; } @@ -1444,7 +1463,7 @@ static zend_result php_session_send_cookie(void) /* {{{ */ e_id = php_url_encode(ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id))); smart_str_appendl(&ncookie, "Set-Cookie: ", sizeof("Set-Cookie: ")-1); - smart_str_appendl(&ncookie, PS(session_name), strlen(PS(session_name))); + smart_str_append(&ncookie, PS(session_name)); smart_str_appendc(&ncookie, '='); smart_str_appendl(&ncookie, ZSTR_VAL(e_id), ZSTR_LEN(e_id)); @@ -1570,7 +1589,7 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */ if (PS(define_sid)) { smart_str var = {0}; - smart_str_appends(&var, PS(session_name)); + smart_str_append(&var, PS(session_name)); smart_str_appendc(&var, '='); smart_str_appends(&var, ZSTR_VAL(PS(id))); smart_str_0(&var); @@ -1598,18 +1617,15 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */ (data = zend_hash_str_find(&EG(symbol_table), "_COOKIE", sizeof("_COOKIE") - 1))) { ZVAL_DEREF(data); if (Z_TYPE_P(data) == IS_ARRAY && - (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), strlen(PS(session_name))))) { + (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) { ZVAL_DEREF(ppid); apply_trans_sid = 0; } } } if (apply_trans_sid) { - zend_string *sname; - sname = zend_string_init(PS(session_name), strlen(PS(session_name)), 0); - php_url_scanner_reset_session_var(sname, 1); /* This may fail when session name has changed */ - zend_string_release_ex(sname, 0); - php_url_scanner_add_session_var(PS(session_name), strlen(PS(session_name)), ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)), 1); + php_url_scanner_reset_session_var(PS(session_name), 1); /* This may fail when session name has changed */ + php_url_scanner_add_session_var(ZSTR_VAL(PS(session_name)), ZSTR_LEN(PS(session_name)), ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)), 1); } return SUCCESS; } @@ -1620,8 +1636,7 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ { zval *ppid; zval *data; - char *value; - size_t lensess; + char *p, *value; switch (PS(session_status)) { case php_session_active: @@ -1656,8 +1671,6 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ PS(send_cookie) = PS(use_cookies) || PS(use_only_cookies); } - lensess = strlen(PS(session_name)); - /* * Cookies are preferred, because initially cookie and get * variables will be available. @@ -1669,7 +1682,7 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ if (!PS(id)) { if (PS(use_cookies) && (data = zend_hash_str_find(&EG(symbol_table), "_COOKIE", sizeof("_COOKIE") - 1))) { ZVAL_DEREF(data); - if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) { + if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) { ppid2sid(ppid); PS(send_cookie) = 0; PS(define_sid) = 0; @@ -1679,16 +1692,31 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ if (!PS(use_only_cookies)) { if (!PS(id) && (data = zend_hash_str_find(&EG(symbol_table), "_GET", sizeof("_GET") - 1))) { ZVAL_DEREF(data); - if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) { + if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) { ppid2sid(ppid); } } if (!PS(id) && (data = zend_hash_str_find(&EG(symbol_table), "_POST", sizeof("_POST") - 1))) { ZVAL_DEREF(data); - if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) { + if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) { ppid2sid(ppid); } } + /* Check the REQUEST_URI symbol for a string of the form + * '=' to allow URLs of the form + * http://yoursite/=/script.php */ + if (!PS(id) && zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER)) == SUCCESS && + (data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "REQUEST_URI", sizeof("REQUEST_URI") - 1)) && + Z_TYPE_P(data) == IS_STRING && + (p = strstr(Z_STRVAL_P(data), ZSTR_VAL(PS(session_name)))) && + p[ZSTR_LEN(PS(session_name))] == '=' + ) { + char *q; + p += ZSTR_LEN(PS(session_name)); + if ((q = strpbrk(p, "/?\\"))) { + PS(id) = zend_string_init(p, q - p, 0); + } + } /* Check whether the current request was referred to by * an external site which invalidates the previously found id. */ if (PS(id) && PS(extern_referer_chk) && ZSTR_LEN(PS(extern_referer_chk)) != 0 && @@ -1771,7 +1799,7 @@ static zend_result php_session_reset(void) /* {{{ */ PHPAPI void session_adapt_url(const char *url, size_t url_len, char **new_url, size_t *new_len) /* {{{ */ { if (APPLY_TRANS_SID && (PS(session_status) == php_session_active)) { - *new_url = php_url_scanner_adapt_single_url(url, url_len, PS(session_name), ZSTR_VAL(PS(id)), new_len, 1); + *new_url = php_url_scanner_adapt_single_url(url, url_len, ZSTR_VAL(PS(session_name)), ZSTR_VAL(PS(id)), new_len, 1); } } /* }}} */ @@ -1993,7 +2021,8 @@ PHP_FUNCTION(session_name) RETURN_FALSE; } - RETVAL_STRING(PS(session_name)); + // TODO Prevent duplication??? + RETVAL_STR(zend_string_dup(PS(session_name), false)); if (name) { ini_name = ZSTR_INIT_LITERAL("session.name", 0); @@ -2409,7 +2438,7 @@ PHP_FUNCTION(session_regenerate_id) zend_string_release_ex(PS(id), 0); PS(id) = NULL; - if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE) { + if (PS(mod)->s_open(&PS(mod_data), PS(save_path), ZSTR_VAL(PS(session_name))) == FAILURE) { PS(session_status) = php_session_none; if (!EG(exception)) { zend_throw_error(NULL, "Failed to open session: %s (path: %s)", PS(mod)->s_name, PS(save_path)); @@ -3125,7 +3154,7 @@ static bool early_find_sid_in(zval *dest, int where, php_session_rfc1867_progres return 0; } - if ((ppid = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[where]), PS(session_name), progress->sname_len)) + if ((ppid = zend_hash_find(Z_ARRVAL(PG(http_globals)[where]), PS(session_name))) && Z_TYPE_P(ppid) == IS_STRING) { zval_ptr_dtor(dest); ZVAL_COPY_DEREF(dest, ppid); @@ -3233,7 +3262,8 @@ static zend_result php_session_rfc1867_callback(unsigned int event, void *event_ multipart_event_start *data = (multipart_event_start *) event_data; progress = ecalloc(1, sizeof(php_session_rfc1867_progress)); progress->content_length = data->content_length; - progress->sname_len = strlen(PS(session_name)); + // TODO Remove field? + progress->sname_len = ZSTR_LEN(PS(session_name)); PS(rfc1867_progress) = progress; } break; @@ -3255,7 +3285,7 @@ static zend_result php_session_rfc1867_callback(unsigned int event, void *event_ if (data->name && data->value && value_len) { size_t name_len = strlen(data->name); - if (name_len == progress->sname_len && memcmp(data->name, PS(session_name), name_len) == 0) { + if (name_len == progress->sname_len && memcmp(data->name, ZSTR_VAL(PS(session_name)), name_len) == 0) { zval_ptr_dtor(&progress->sid); ZVAL_STRINGL(&progress->sid, (*data->value), value_len); } else if (name_len == strlen(PS(rfc1867_name)) && memcmp(data->name, PS(rfc1867_name), name_len + 1) == 0) { diff --git a/ext/session/tests/bug66481.phpt b/ext/session/tests/bug66481.phpt index 88c2e48ed7999..91fd956fde024 100644 --- a/ext/session/tests/bug66481.phpt +++ b/ext/session/tests/bug66481.phpt @@ -15,6 +15,6 @@ var_dump(session_name("foo")); var_dump(session_name("bar")); ?> --EXPECT-- -Warning: PHP Startup: session.name "" cannot be numeric or empty in Unknown on line 0 +Warning: PHP Startup: session.name "" cannot be empty in Unknown on line 0 string(9) "PHPSESSID" string(3) "foo" diff --git a/ext/session/tests/rfc1867_sid_post.phpt b/ext/session/tests/rfc1867_sid_post.phpt index 62bc17bd83831..f029d7eb45f09 100644 --- a/ext/session/tests/rfc1867_sid_post.phpt +++ b/ext/session/tests/rfc1867_sid_post.phpt @@ -48,7 +48,7 @@ var_dump($_SESSION["upload_progress_" . basename(__FILE__)]); session_destroy(); ?> --EXPECTF-- -string(%d) "rfc1867-sid-post" +string(16) "rfc1867-sid-post" bool(true) array(2) { ["file1"]=> diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index ef68b80b3def9..facb8390d5b52 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -32,6 +32,8 @@ ob_end_flush(); ?> --EXPECTF-- *** Testing session_name() : variation *** + +Warning: session_name(): session.name "" cannot contain nul bytes in %s on line %d string(9) "PHPSESSID" Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d @@ -40,7 +42,7 @@ string(1) " " bool(true) string(1) " " -Warning: session_name(): session.name "" cannot be numeric or empty in %s on line %d +Warning: session_name(): session.name "" cannot be empty in %s on line %d string(1) " " Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d From b1c18ae763a0a58c4162edfdc26216a6e1a85a08 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 14:21:32 +0100 Subject: [PATCH 03/10] Use NUL spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Tim Düsterhus --- ext/session/session.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index 73949bac8ae86..eaa0e22108be3 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -719,11 +719,11 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ } return FAILURE; } - /* Nul bytes are not allowed */ + /* NUL bytes are not allowed */ if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { /* Do not output error when restoring ini options. */ if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot contain nul bytes", ZSTR_VAL(new_value)); + php_error_docref(NULL, err_type, "session.name \"%s\" cannot contain NUL bytes", ZSTR_VAL(new_value)); } return FAILURE; } From 3b213dd8c88d10cc61e05e28bc55fd475dc35b45 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 14:31:36 +0100 Subject: [PATCH 04/10] address review comments --- ext/session/session.c | 6 ++-- .../tests/session_name_variation1.phpt | 34 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index eaa0e22108be3..f282eae22fd3a 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -727,11 +727,13 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ } return FAILURE; } - /* Numeric session.name won't work at all */ + /* Numeric session.name won't work at all + * See https://bugs.php.net/bug.php?id=35703 + (TL;DR: name is stored in HashTable so numeric string is converted to int key, but lookup looks for string key). */ if (is_numeric_str_function(new_value, NULL, NULL)) { /* Do not output error when restoring ini options. */ if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric or empty", ZSTR_VAL(new_value)); + php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric", ZSTR_VAL(new_value)); } return FAILURE; } diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index facb8390d5b52..f0fa500de6f9a 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -15,6 +15,24 @@ ob_start(); echo "*** Testing session_name() : variation ***\n"; +var_dump(session_name("\0")); +var_dump(session_start()); +var_dump(session_name()); +var_dump(session_destroy()); +var_dump(session_name()); + +var_dump(session_name("15")); +var_dump(session_start()); +var_dump(session_name()); +var_dump(session_destroy()); +var_dump(session_name()); + +var_dump(session_name("10.25")); +var_dump(session_start()); +var_dump(session_name()); +var_dump(session_destroy()); +var_dump(session_name()); + var_dump(session_name("\t")); var_dump(session_start()); var_dump(session_name()); @@ -33,7 +51,21 @@ ob_end_flush(); --EXPECTF-- *** Testing session_name() : variation *** -Warning: session_name(): session.name "" cannot contain nul bytes in %s on line %d +Warning: session_name(): session.name "" cannot contain NUL bytes in %s on line %d +string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" + +Warning: session_name(): session.name "15" cannot be numeric in %s on line %d +string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" + +Warning: session_name(): session.name "10.25" cannot be numeric in %s on line %d string(9) "PHPSESSID" Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d From b03b233b887cff585230b7f3b0a86ff1a3f6101a Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 15:18:35 +0100 Subject: [PATCH 05/10] Check for session name criterias consistently --- ext/session/session.c | 68 +++++++++++-------- .../tests/session_name_variation1.phpt | 19 +++--- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index f282eae22fd3a..d2eb19d0933b9 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -698,6 +698,41 @@ static PHP_INI_MH(OnUpdateSaveDir) /* {{{ */ } /* }}} */ +/* If diagnostic_type is 0 then no diagnostic will be emitted */ +static bool is_session_name_valid(const zend_string *name, int diagnostic_type) +{ + if (ZSTR_LEN(name) == 0) { + if (diagnostic_type) { + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot be empty", ZSTR_VAL(name)); + } + return false; + } + /* NUL bytes are not allowed */ + if (ZSTR_LEN(name) != strlen(ZSTR_VAL(name))) { + if (diagnostic_type) { + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot contain NUL bytes", ZSTR_VAL(name)); + } + return false; + } + /* Numeric session.name won't work at all + * See https://bugs.php.net/bug.php?id=35703 + (TL;DR: name is stored in HashTable so numeric string is converted to int key, but lookup looks for string key). */ + if (is_numeric_str_function(name, NULL, NULL)) { + if (diagnostic_type) { + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot be numeric", ZSTR_VAL(name)); + } + return false; + } + /* Prevent broken Set-Cookie header, because the session_name might be user supplied */ + if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + if (diagnostic_type) { + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot contain any of the following " + "'=,; \\t\\r\\n\\013\\014'", ZSTR_VAL(name)); + } + return false; + } + return true; +} static PHP_INI_MH(OnUpdateName) /* {{{ */ { @@ -708,33 +743,14 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) { err_type = E_WARNING; + } else if (stage == ZEND_INI_STAGE_DEACTIVATE) { + /* Do not output error when restoring ini options. */ + err_type = 0; } else { err_type = E_ERROR; } - if (ZSTR_LEN(new_value) == 0) { - /* Do not output error when restoring ini options. */ - if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot be empty", ZSTR_VAL(new_value)); - } - return FAILURE; - } - /* NUL bytes are not allowed */ - if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) { - /* Do not output error when restoring ini options. */ - if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot contain NUL bytes", ZSTR_VAL(new_value)); - } - return FAILURE; - } - /* Numeric session.name won't work at all - * See https://bugs.php.net/bug.php?id=35703 - (TL;DR: name is stored in HashTable so numeric string is converted to int key, but lookup looks for string key). */ - if (is_numeric_str_function(new_value, NULL, NULL)) { - /* Do not output error when restoring ini options. */ - if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric", ZSTR_VAL(new_value)); - } + if (!is_session_name_valid(new_value, err_type)) { return FAILURE; } @@ -1413,7 +1429,7 @@ static void php_session_remove_cookie(void) { size_t session_cookie_len; size_t len = sizeof("Set-Cookie")-1; - ZEND_ASSERT(strpbrk(ZSTR_VAL(PS(session_name)), SESSION_FORBIDDEN_CHARS) == NULL); + ZEND_ASSERT(is_session_name_valid(PS(session_name), 0)); spprintf(&session_cookie, 0, "Set-Cookie: %s=", ZSTR_VAL(PS(session_name))); // TODO Manually compute from known information? @@ -1454,10 +1470,8 @@ static zend_result php_session_send_cookie(void) /* {{{ */ return FAILURE; } - // TODO need to Check for nul byte? /* Prevent broken Set-Cookie header, because the session_name might be user supplied */ - if (strpbrk(ZSTR_VAL(PS(session_name)), SESSION_FORBIDDEN_CHARS) != NULL) { /* man isspace for \013 and \014 */ - php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,; \\t\\r\\n\\013\\014'"); + if (!is_session_name_valid(PS(session_name), E_WARNING)) { return FAILURE; } diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index f0fa500de6f9a..d6885234924d5 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -67,19 +67,22 @@ string(9) "PHPSESSID" Warning: session_name(): session.name "10.25" cannot be numeric in %s on line %d string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" +bool(true) +string(9) "PHPSESSID" -Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d +Warning: session_name(): session.name " " cannot contain any of the following '=,; \t\r\n\013\014' in %s on line %d +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" Warning: session_name(): session.name "" cannot be empty in %s on line %d -string(1) " " - -Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" Done From b01aa3e49ebcc8d9582f0e5fec4996da4fae9298 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 22 Aug 2022 16:16:08 +0100 Subject: [PATCH 06/10] Use new API for checking if string has NUL bytes --- ext/session/session.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/session/session.c b/ext/session/session.c index d2eb19d0933b9..acb7b780ab39d 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -708,7 +708,7 @@ static bool is_session_name_valid(const zend_string *name, int diagnostic_type) return false; } /* NUL bytes are not allowed */ - if (ZSTR_LEN(name) != strlen(ZSTR_VAL(name))) { + if (zend_str_has_nul_byte(name)) { if (diagnostic_type) { php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot contain NUL bytes", ZSTR_VAL(name)); } From aaa67ee90e3e2bafd1451c21639dccd6bf6bef07 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 30 Aug 2022 13:26:13 +0100 Subject: [PATCH 07/10] Use known length --- ext/session/session.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index acb7b780ab39d..e84fbb458f31d 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -2007,13 +2007,13 @@ PHP_FUNCTION(session_get_cookie_params) add_assoc_long(return_value, "lifetime", PS(cookie_lifetime)); // TODO Use add_assoc_str() but figure out why it emits a // Zend/zend_types.h:1222: zend_gc_delref: Assertion `(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)' failed. - add_assoc_string(return_value, "path", ZSTR_VAL(PS(cookie_path))); - add_assoc_string(return_value, "domain", ZSTR_VAL(PS(cookie_domain))); + add_assoc_stringl(return_value, "path", ZSTR_VAL(PS(cookie_path)), ZSTR_LEN(PS(cookie_path))); + add_assoc_stringl(return_value, "domain", ZSTR_VAL(PS(cookie_domain)), ZSTR_LEN(PS(cookie_domain))); add_assoc_bool(return_value, "secure", PS(cookie_secure)); add_assoc_bool(return_value, "httponly", PS(cookie_httponly)); // TODO Use add_assoc_str() but figure out why it emits a // Zend/zend_types.h:1222: zend_gc_delref: Assertion `(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)' failed. - add_assoc_string(return_value, "samesite", ZSTR_VAL(PS(cookie_samesite))); + add_assoc_stringl(return_value, "samesite", ZSTR_VAL(PS(cookie_samesite)), ZSTR_LEN(PS(cookie_samesite))); } /* }}} */ From 0393e652a25a1e68bd22ed4cbfb34445f8138ebf Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Sun, 13 Oct 2024 19:36:09 +0200 Subject: [PATCH 08/10] Rebase alignments --- ext/session/php_session.h | 4 ++-- ext/session/session.c | 19 ++----------------- .../tests/session_name_variation1.phpt | 13 ------------- 3 files changed, 4 insertions(+), 32 deletions(-) diff --git a/ext/session/php_session.h b/ext/session/php_session.h index 39d5a2f1d7fbe..292828ecb182b 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -147,9 +147,9 @@ typedef struct _php_ps_globals { zend_long cookie_lifetime; zend_string *cookie_path; zend_string *cookie_domain; + zend_string *cookie_samesite; bool cookie_secure; bool cookie_httponly; - zend_string *cookie_samesite; const ps_module *mod; const ps_module *default_mod; void *mod_data; @@ -198,7 +198,7 @@ typedef struct _php_ps_globals { bool rfc1867_enabled; /* session.upload_progress.enabled */ bool rfc1867_cleanup; /* session.upload_progress.cleanup */ - bool use_strict_mode; /* whether or not PHP accepts unknown session ids */ + bool use_strict_mode; /* whether PHP accepts unknown session ids */ bool lazy_write; /* omit session write when it is possible */ bool in_save_handler; /* state if session is in save handler or not */ bool set_handler; /* state if session module i setting handler or not */ diff --git a/ext/session/session.c b/ext/session/session.c index e84fbb458f31d..bc44f99d0e52f 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -969,7 +969,7 @@ PHP_INI_BEGIN() STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateSessionStr, extern_referer_chk, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateRefererCheck, extern_referer_chk, php_ps_globals, ps_globals) STD_PHP_INI_ENTRY("session.cache_limiter", "nocache", PHP_INI_ALL, OnUpdateSessionStr, cache_limiter, php_ps_globals, ps_globals) STD_PHP_INI_ENTRY("session.cache_expire", "180", PHP_INI_ALL, OnUpdateSessionLong, cache_expire, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_trans_sid", "0", PHP_INI_ALL, OnUpdateUseTransSid, use_trans_sid, php_ps_globals, ps_globals) @@ -1652,7 +1652,7 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ { zval *ppid; zval *data; - char *p, *value; + char *value; switch (PS(session_status)) { case php_session_active: @@ -1718,21 +1718,6 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ ppid2sid(ppid); } } - /* Check the REQUEST_URI symbol for a string of the form - * '=' to allow URLs of the form - * http://yoursite/=/script.php */ - if (!PS(id) && zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER)) == SUCCESS && - (data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "REQUEST_URI", sizeof("REQUEST_URI") - 1)) && - Z_TYPE_P(data) == IS_STRING && - (p = strstr(Z_STRVAL_P(data), ZSTR_VAL(PS(session_name)))) && - p[ZSTR_LEN(PS(session_name))] == '=' - ) { - char *q; - p += ZSTR_LEN(PS(session_name)); - if ((q = strpbrk(p, "/?\\"))) { - PS(id) = zend_string_init(p, q - p, 0); - } - } /* Check whether the current request was referred to by * an external site which invalidates the previously found id. */ if (PS(id) && PS(extern_referer_chk) && ZSTR_LEN(PS(extern_referer_chk)) != 0 && diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index d6885234924d5..30986f2e50b6e 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -15,12 +15,6 @@ ob_start(); echo "*** Testing session_name() : variation ***\n"; -var_dump(session_name("\0")); -var_dump(session_start()); -var_dump(session_name()); -var_dump(session_destroy()); -var_dump(session_name()); - var_dump(session_name("15")); var_dump(session_start()); var_dump(session_name()); @@ -51,13 +45,6 @@ ob_end_flush(); --EXPECTF-- *** Testing session_name() : variation *** -Warning: session_name(): session.name "" cannot contain NUL bytes in %s on line %d -string(9) "PHPSESSID" -bool(true) -string(9) "PHPSESSID" -bool(true) -string(9) "PHPSESSID" - Warning: session_name(): session.name "15" cannot be numeric in %s on line %d string(9) "PHPSESSID" bool(true) From 6660a1ca2fe9aefefc8d3e3e246104b1696c4cb7 Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Fri, 6 Dec 2024 01:10:28 +0100 Subject: [PATCH 09/10] Improved error messages --- ext/session/session.c | 14 +++++++------- ext/session/tests/bug66481.phpt | 2 +- ext/session/tests/session_name_variation1.phpt | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index bc44f99d0e52f..e73bf2d2768c6 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -93,7 +93,7 @@ zend_class_entry *php_session_update_timestamp_iface_entry; return FAILURE; \ } -#define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\013\014" +#define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\v\f" #define APPLY_TRANS_SID (PS(use_trans_sid) && !PS(use_only_cookies)) @@ -703,14 +703,14 @@ static bool is_session_name_valid(const zend_string *name, int diagnostic_type) { if (ZSTR_LEN(name) == 0) { if (diagnostic_type) { - php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot be empty", ZSTR_VAL(name)); + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not be empty", ZSTR_VAL(name)); } return false; } /* NUL bytes are not allowed */ if (zend_str_has_nul_byte(name)) { if (diagnostic_type) { - php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot contain NUL bytes", ZSTR_VAL(name)); + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not contain any null bytes", ZSTR_VAL(name)); } return false; } @@ -719,15 +719,15 @@ static bool is_session_name_valid(const zend_string *name, int diagnostic_type) (TL;DR: name is stored in HashTable so numeric string is converted to int key, but lookup looks for string key). */ if (is_numeric_str_function(name, NULL, NULL)) { if (diagnostic_type) { - php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot be numeric", ZSTR_VAL(name)); + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not be numeric", ZSTR_VAL(name)); } return false; } /* Prevent broken Set-Cookie header, because the session_name might be user supplied */ - if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\v\f") != NULL) { /* man isspace for \v and \f */ if (diagnostic_type) { - php_error_docref(NULL, diagnostic_type, "session.name \"%s\" cannot contain any of the following " - "'=,; \\t\\r\\n\\013\\014'", ZSTR_VAL(name)); + php_error_docref(NULL, diagnostic_type, "session.name \"%s\" must not contain any of the following " + "'=,; \\t\\r\\n\\v\\f'", ZSTR_VAL(name)); } return false; } diff --git a/ext/session/tests/bug66481.phpt b/ext/session/tests/bug66481.phpt index 91fd956fde024..a092e1ee0b809 100644 --- a/ext/session/tests/bug66481.phpt +++ b/ext/session/tests/bug66481.phpt @@ -15,6 +15,6 @@ var_dump(session_name("foo")); var_dump(session_name("bar")); ?> --EXPECT-- -Warning: PHP Startup: session.name "" cannot be empty in Unknown on line 0 +Warning: PHP Startup: session.name "" must not be empty in Unknown on line 0 string(9) "PHPSESSID" string(3) "foo" diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index 30986f2e50b6e..81c2ae3c788a3 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -45,28 +45,28 @@ ob_end_flush(); --EXPECTF-- *** Testing session_name() : variation *** -Warning: session_name(): session.name "15" cannot be numeric in %s on line %d +Warning: session_name(): session.name "15" must not be numeric in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -Warning: session_name(): session.name "10.25" cannot be numeric in %s on line %d +Warning: session_name(): session.name "10.25" must not be numeric in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -Warning: session_name(): session.name " " cannot contain any of the following '=,; \t\r\n\013\014' in %s on line %d +Warning: session_name(): session.name " " must not contain any of the following '=,; \t\r\n\v\f' in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -Warning: session_name(): session.name "" cannot be empty in %s on line %d +Warning: session_name(): session.name "" must not be empty in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" From d5de6ed02c0baa52bc4cb14b06ab51393c34c56c Mon Sep 17 00:00:00 2001 From: Jorg Sowa Date: Fri, 10 Jan 2025 19:30:40 +0100 Subject: [PATCH 10/10] Replaced invalid OnUpdateString --- ext/session/session.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index e73bf2d2768c6..9a3b490cd2c30 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -947,7 +947,11 @@ static PHP_INI_MH(OnUpdateRefererCheck) if (ZSTR_LEN(new_value) != 0) { php_error_docref("session.configuration", E_DEPRECATED, "Usage of session.referer_check INI setting is deprecated"); } - return OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); + + zend_string **p = (zend_string **) ZEND_INI_GET_ADDR(); + *p = new_value ? new_value : NULL; + return SUCCESS; + } /* {{{ PHP_INI */ @@ -1720,7 +1724,9 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ } /* Check whether the current request was referred to by * an external site which invalidates the previously found id. */ - if (PS(id) && PS(extern_referer_chk) && ZSTR_LEN(PS(extern_referer_chk)) != 0 && + if (PS(id) && + PS(extern_referer_chk) && + ZSTR_LEN(PS(extern_referer_chk)) != 0 && !Z_ISUNDEF(PG(http_globals)[TRACK_VARS_SERVER]) && (data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "HTTP_REFERER", sizeof("HTTP_REFERER") - 1)) && Z_TYPE_P(data) == IS_STRING &&