ext/session: reject cookie separators in cookie_path and cookie_domain (#22925)

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. cookie_path now
rejects ";" and CR/LF, which RFC 6265 path-value excludes, while keeping
the comma it allows; cookie_domain takes the set setcookie() enforces on
$domain.

Closes GH-22925

UPGRADING

@@ -27,6 +27,13 @@ PHP 8.7 UPGRADE NOTES
     not present in the result set. It previously reported the condition
     through PDO::ATTR_ERRMODE and returned false.
 
+- Session:
+  . Setting session.cookie_path to a value containing one of ";\r\n", or
+    session.cookie_domain to a value containing one of ",; \t\r\n\013\014",
+    now emits a warning and leaves the setting unchanged. Previously such a
+    value was appended to the Set-Cookie header verbatim, which let an
+    application terminate one attribute and append further ones.
+
 - Standard:
   . The number of filters that can be chained in a php://filter URL is limited
     to 16 by default. Set the stream context option max_filter_count to change

ext/session/session.c

@@ -97,6 +97,12 @@ 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_PATH_CHARS ";\r\n"
+#define SESSION_FORBIDDEN_COOKIE_PATH_CHARS_FOR_ERROR_MSG ";\\r\\n"
+
+#define SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS ",; \t\r\n\013\014"
+#define SESSION_FORBIDDEN_COOKIE_DOMAIN_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);
@@ -769,6 +775,36 @@ static PHP_INI_MH(OnUpdateSessionStr)
 	return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
 }
 
+static bool php_session_cookie_attr_is_valid(const zend_ini_entry *entry, const zend_string *new_value, int stage, const char *forbidden, const char *forbidden_msg)
+{
+	if (new_value && strpbrk(ZSTR_VAL(new_value), forbidden) != NULL) {
+		if (stage != ZEND_INI_STAGE_DEACTIVATE) {
+			php_error_docref(NULL, E_WARNING, "\"%s\" must not contain any of the following characters \"%s\"", ZSTR_VAL(entry->name), forbidden_msg);
+		}
+		return false;
+	}
+
+	return true;
+}
+
+static PHP_INI_MH(OnUpdateSessionCookiePath)
+{
+	if (!php_session_cookie_attr_is_valid(entry, new_value, stage, SESSION_FORBIDDEN_COOKIE_PATH_CHARS, SESSION_FORBIDDEN_COOKIE_PATH_CHARS_FOR_ERROR_MSG)) {
+		return FAILURE;
+	}
+
+	return OnUpdateSessionStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
+}
+
+static PHP_INI_MH(OnUpdateSessionCookieDomain)
+{
+	if (!php_session_cookie_attr_is_valid(entry, new_value, stage, SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS, SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS_FOR_ERROR_MSG)) {
+		return FAILURE;
+	}
+
+	return OnUpdateSessionStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
+}
+
 static PHP_INI_MH(OnUpdateSessionSameSite)
 {
 	SESSION_CHECK_ACTIVE_STATE;
@@ -956,8 +992,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,    OnUpdateSessionCookiePath,    cookie_path,        php_ps_globals, ps_globals)
+	STD_PHP_INI_ENTRY("session.cookie_domain",        "",          PHP_INI_ALL,    OnUpdateSessionCookieDomain,  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' => '/foo,bar', '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 ";\r\n" 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 ";\r\n" 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(8) "/foo,bar"
+string(11) "example.com"