ext/session: reject cookie separators in cookie_path and cookie_domain

php_session_send_cookie() appends session.cookie_path and
session.cookie_domain to the Set-Cookie header verbatim, and neither
setting rejected the separators that terminate a cookie attribute. An
application could therefore append its own Domain, Path, HttpOnly,
Secure, or SameSite attribute through either value. Apply the character
set setcookie() already enforces on its $path and $domain options.

Author: iliaal

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

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)

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--
+<?php
+
+ob_start();
+
+var_dump(session_set_cookie_params(['path' => '/; 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"