From 8fdba23c67c12edc07fe5928bbcffb3f5b0065e1 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 12 Aug 2022 22:15:02 +0100 Subject: [PATCH 1/7] 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 31b96340e82d6..fa0410e368ce7 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -143,14 +143,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; + zend_string *cookie_path; + zend_string *cookie_domain; bool cookie_secure; bool cookie_httponly; - char *cookie_samesite; + 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 13e2d2dbf8144..606c730786286 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -697,11 +697,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; } /* }}} */ @@ -785,16 +788,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, OnUpdateSessionBool, 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, OnUpdateSessionString, 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, OnUpdateSessionBool, use_trans_sid, php_ps_globals, ps_globals) PHP_INI_ENTRY("session.sid_length", "32", PHP_INI_ALL, OnUpdateSidLength) @@ -1209,7 +1212,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)) { @@ -1226,7 +1229,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; } @@ -1335,14 +1339,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)) { @@ -1353,9 +1357,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); @@ -1573,12 +1577,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; @@ -1836,11 +1840,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))); } /* }}} */ @@ -2370,7 +2378,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 = zend_string_init("session.cache_limiter", sizeof("session.cache_limiter") - 1, 0); From 4db9596fea7605b9e941254f0771bad24f19b0b4 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 12 Aug 2022 22:56:23 +0100 Subject: [PATCH 2/7] 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 | 87 +++++++++++-------- ext/session/tests/bug66481.phpt | 2 +- ext/session/tests/rfc1867_sid_post.phpt | 2 +- .../tests/session_name_variation1.phpt | 4 +- 5 files changed, 57 insertions(+), 40 deletions(-) diff --git a/ext/session/php_session.h b/ext/session/php_session.h index fa0410e368ce7..1dee0095a6e59 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -141,7 +141,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 606c730786286..308687a8bce39 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -395,7 +395,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(); @@ -653,16 +653,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)); @@ -670,7 +685,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; } /* }}} */ @@ -1258,9 +1275,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), "=,; \t\r\n\013\014") == NULL); - spprintf(&session_cookie, 0, "Set-Cookie: %s=", PS(session_name)); + ZEND_ASSERT(strpbrk(ZSTR_VAL(PS(session_name)), "=,; \t\r\n\013\014") == 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) { @@ -1305,8 +1323,9 @@ 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), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + if (strpbrk(ZSTR_VAL(PS(session_name)), "=,; \t\r\n\013\014") != 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; } @@ -1315,7 +1334,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)); @@ -1441,7 +1460,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); @@ -1469,18 +1488,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; } @@ -1492,7 +1508,6 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ zval *ppid; zval *data; char *p, *value; - size_t lensess; switch (PS(session_status)) { case php_session_active: @@ -1527,8 +1542,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. @@ -1540,7 +1553,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; @@ -1550,13 +1563,13 @@ 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); } } @@ -1566,11 +1579,11 @@ PHPAPI zend_result php_session_start(void) /* {{{ */ 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), PS(session_name))) && - p[lensess] == '=' + (p = strstr(Z_STRVAL_P(data), ZSTR_VAL(PS(session_name)))) && + p[ZSTR_LEN(PS(session_name))] == '=' ) { char *q; - p += lensess + 1; + p += ZSTR_LEN(PS(session_name)); if ((q = strpbrk(p, "/?\\"))) { PS(id) = zend_string_init(p, q - p, 0); } @@ -1651,7 +1664,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); } } /* }}} */ @@ -1872,7 +1885,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 = zend_string_init("session.name", sizeof("session.name") - 1, 0); @@ -2247,7 +2261,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)); @@ -2918,7 +2932,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); @@ -3026,7 +3040,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; @@ -3048,7 +3063,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 e1f2123b56a58..fb3ae384cd0a1 100644 --- a/ext/session/tests/rfc1867_sid_post.phpt +++ b/ext/session/tests/rfc1867_sid_post.phpt @@ -47,7 +47,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 f7ccc7744a8ff..910e218bbd0fa 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -38,6 +38,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" bool(true) string(9) "PHPSESSID" @@ -51,7 +53,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 c91f1b8dd32dbc4016c965a187771b629105a7e7 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 14:21:32 +0100 Subject: [PATCH 3/7] 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 308687a8bce39..d443946f73ddd 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -668,11 +668,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 4cd856dafe1293bbf5efc6b1ec85c88a14f38baf Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 14:31:36 +0100 Subject: [PATCH 4/7] address review comments --- ext/session/session.c | 6 ++-- .../tests/session_name_variation1.phpt | 28 ++++++++++++++++++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index d443946f73ddd..23e39454e26c9 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -676,11 +676,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 910e218bbd0fa..5c5f2612f671b 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -21,6 +21,18 @@ 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()); @@ -39,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" bool(true) string(9) "PHPSESSID" From 91ec6fe572cf060ee965922cefedcaa23ddb0220 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 19 Aug 2022 15:18:35 +0100 Subject: [PATCH 5/7] Check for session name criterias consistently --- ext/session/session.c | 68 +++++++++++-------- .../tests/session_name_variation1.phpt | 16 ++--- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index 23e39454e26c9..a0d18dac8b0a7 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -647,6 +647,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) /* {{{ */ { @@ -657,33 +692,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; } @@ -1277,7 +1293,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)), "=,; \t\r\n\013\014") == 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? @@ -1325,10 +1341,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)), "=,; \t\r\n\013\014") != 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 5c5f2612f671b..d6885234924d5 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -71,20 +71,18 @@ bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -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 c8f02358ecad7de52cd17d737097ba0da67638dd Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 22 Aug 2022 16:16:08 +0100 Subject: [PATCH 6/7] 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 a0d18dac8b0a7..c2ab82808f7c7 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -657,7 +657,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 f9e3e295cbec26bf5d76747d45c116a1dd5ecb59 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 30 Aug 2022 13:26:13 +0100 Subject: [PATCH 7/7] 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 c2ab82808f7c7..226655f21fe54 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -1871,13 +1871,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))); } /* }}} */