diff --git a/UPGRADING b/UPGRADING index 47a9c023bf3f..cbfaeab3a545 100644 --- a/UPGRADING +++ b/UPGRADING @@ -121,6 +121,12 @@ PHP 8.6 UPGRADE NOTES unchanged. Previously, NUL bytes were silently accepted: for cookie_path and cookie_domain this caused the SAPI to drop the Set-Cookie header; for cache_limiter the value was silently truncated at the NUL byte. + . Setting session.cookie_path or session.cookie_domain to a value containing + one of ",; \t\r\n\013\014" now emits a warning and leaves the setting + unchanged, matching the validation setcookie() already applies to its + $path and $domain options. Previously such a value was appended to the + Set-Cookie header verbatim, which let an application terminate one + attribute and append further ones. . A ValueError is not thrown if $name is a string containing NUL bytes in session_module_name(). . session_encode() now returns an empty string instead of false for empty diff --git a/ext/session/session.c b/ext/session/session.c index dd968d453bda..86a0730020fb 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -97,6 +97,9 @@ zend_class_entry *php_session_update_timestamp_iface_entry; #define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\013\014" #define SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "=,;.[ \\t\\r\\n\\013\\014" +#define SESSION_FORBIDDEN_COOKIE_CHARS ",; \t\r\n\013\014" +#define SESSION_FORBIDDEN_COOKIE_CHARS_FOR_ERROR_MSG ",; \\t\\r\\n\\013\\014" + #define APPLY_TRANS_SID (PS(use_trans_sid) && !PS(use_only_cookies)) static zend_result php_session_send_cookie(void); @@ -759,6 +762,18 @@ static PHP_INI_MH(OnUpdateSessionStr) return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); } +static PHP_INI_MH(OnUpdateSessionCookieStr) +{ + if (new_value && strpbrk(ZSTR_VAL(new_value), SESSION_FORBIDDEN_COOKIE_CHARS) != NULL) { + if (stage != ZEND_INI_STAGE_DEACTIVATE) { + php_error_docref(NULL, E_WARNING, "\"%s\" must not contain any of the following characters \"" SESSION_FORBIDDEN_COOKIE_CHARS_FOR_ERROR_MSG "\"", ZSTR_VAL(entry->name)); + } + return FAILURE; + } + + return OnUpdateSessionStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); +} + static PHP_INI_MH(OnUpdateSessionSameSite) { SESSION_CHECK_ACTIVE_STATE; @@ -946,8 +961,8 @@ 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, 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_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionCookieStr, cookie_path, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionCookieStr, 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_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_httponly", "1", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals) diff --git a/ext/session/tests/session_cookie_params_forbidden_chars.phpt b/ext/session/tests/session_cookie_params_forbidden_chars.phpt new file mode 100644 index 000000000000..167f99e09f75 --- /dev/null +++ b/ext/session/tests/session_cookie_params_forbidden_chars.phpt @@ -0,0 +1,43 @@ +--TEST-- +session.cookie_path and session.cookie_domain reject cookie separators +--INI-- +session.save_handler=files +session.name=PHPSESSID +session.gc_probability=0 +--EXTENSIONS-- +session +--FILE-- + '/; Domain=evil.example'])); +var_dump(session_set_cookie_params(['domain' => 'example.com; HttpOnly'])); +var_dump(session_set_cookie_params(['path' => "/\r\nX-Injected: yes"])); +var_dump(ini_set('session.cookie_domain', "example.com\tevil")); + +$params = session_get_cookie_params(); +var_dump($params['path'], $params['domain']); + +var_dump(session_set_cookie_params(['path' => '/app', 'domain' => 'example.com'])); +$params = session_get_cookie_params(); +var_dump($params['path'], $params['domain']); + +?> +--EXPECTF-- +Warning: session_set_cookie_params(): "session.cookie_path" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d +bool(false) + +Warning: session_set_cookie_params(): "session.cookie_domain" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d +bool(false) + +Warning: session_set_cookie_params(): "session.cookie_path" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d +bool(false) + +Warning: ini_set(): "session.cookie_domain" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d +bool(false) +string(1) "/" +string(0) "" +bool(true) +string(4) "/app" +string(11) "example.com"