Skip to content

Commit 4e8efc5

Browse files
iliaaliliaal
committed
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. 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
1 parent c65321c commit 4e8efc5

3 files changed

Lines changed: 88 additions & 2 deletions

File tree

‎UPGRADING‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@ PHP 8.7 UPGRADE NOTES
2727
not present in the result set. It previously reported the condition
2828
through PDO::ATTR_ERRMODE and returned false.
2929

30+
- Session:
31+
. Setting session.cookie_path to a value containing one of ";\r\n", or
32+
session.cookie_domain to a value containing one of ",; \t\r\n\013\014",
33+
now emits a warning and leaves the setting unchanged. Previously such a
34+
value was appended to the Set-Cookie header verbatim, which let an
35+
application terminate one attribute and append further ones.
36+
3037
- Standard:
3138
. The number of filters that can be chained in a php://filter URL is limited
3239
to 16 by default. Set the stream context option max_filter_count to change

‎ext/session/session.c‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,12 @@ zend_class_entry *php_session_update_timestamp_iface_entry;
9797
#define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\013\014"
9898
#define SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "=,;.[ \\t\\r\\n\\013\\014"
9999

100+
#define SESSION_FORBIDDEN_COOKIE_PATH_CHARS ";\r\n"
101+
#define SESSION_FORBIDDEN_COOKIE_PATH_CHARS_FOR_ERROR_MSG ";\\r\\n"
102+
103+
#define SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS ",; \t\r\n\013\014"
104+
#define SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS_FOR_ERROR_MSG ",; \\t\\r\\n\\013\\014"
105+
100106
#define APPLY_TRANS_SID (PS(use_trans_sid) && !PS(use_only_cookies))
101107

102108
static zend_result php_session_send_cookie(void);
@@ -769,6 +775,36 @@ static PHP_INI_MH(OnUpdateSessionStr)
769775
return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
770776
}
771777

778+
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)
779+
{
780+
if (new_value && strpbrk(ZSTR_VAL(new_value), forbidden) != NULL) {
781+
if (stage != ZEND_INI_STAGE_DEACTIVATE) {
782+
php_error_docref(NULL, E_WARNING, "\"%s\" must not contain any of the following characters \"%s\"", ZSTR_VAL(entry->name), forbidden_msg);
783+
}
784+
return false;
785+
}
786+
787+
return true;
788+
}
789+
790+
static PHP_INI_MH(OnUpdateSessionCookiePath)
791+
{
792+
if (!php_session_cookie_attr_is_valid(entry, new_value, stage, SESSION_FORBIDDEN_COOKIE_PATH_CHARS, SESSION_FORBIDDEN_COOKIE_PATH_CHARS_FOR_ERROR_MSG)) {
793+
return FAILURE;
794+
}
795+
796+
return OnUpdateSessionStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
797+
}
798+
799+
static PHP_INI_MH(OnUpdateSessionCookieDomain)
800+
{
801+
if (!php_session_cookie_attr_is_valid(entry, new_value, stage, SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS, SESSION_FORBIDDEN_COOKIE_DOMAIN_CHARS_FOR_ERROR_MSG)) {
802+
return FAILURE;
803+
}
804+
805+
return OnUpdateSessionStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
806+
}
807+
772808
static PHP_INI_MH(OnUpdateSessionSameSite)
773809
{
774810
SESSION_CHECK_ACTIVE_STATE;
@@ -956,8 +992,8 @@ PHP_INI_BEGIN()
956992
STD_PHP_INI_ENTRY("session.gc_maxlifetime", "1440", PHP_INI_ALL, OnUpdateSessionLong, gc_maxlifetime, php_ps_globals, ps_globals)
957993
PHP_INI_ENTRY("session.serialize_handler", "php", PHP_INI_ALL, OnUpdateSerializer)
958994
STD_PHP_INI_ENTRY("session.cookie_lifetime", "0", PHP_INI_ALL, OnUpdateCookieLifetime, cookie_lifetime, php_ps_globals, ps_globals)
959-
STD_PHP_INI_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionStr, cookie_path, php_ps_globals, ps_globals)
960-
STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_domain, php_ps_globals, ps_globals)
995+
STD_PHP_INI_ENTRY("session.cookie_path", "/", PHP_INI_ALL, OnUpdateSessionCookiePath, cookie_path, php_ps_globals, ps_globals)
996+
STD_PHP_INI_ENTRY("session.cookie_domain", "", PHP_INI_ALL, OnUpdateSessionCookieDomain, cookie_domain, php_ps_globals, ps_globals)
961997
STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals)
962998
STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)
963999
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‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
--TEST--
2+
session.cookie_path and session.cookie_domain reject cookie separators
3+
--INI--
4+
session.save_handler=files
5+
session.name=PHPSESSID
6+
session.gc_probability=0
7+
--EXTENSIONS--
8+
session
9+
--FILE--
10+
<?php
11+
12+
ob_start();
13+
14+
var_dump(session_set_cookie_params(['path' => '/; Domain=evil.example']));
15+
var_dump(session_set_cookie_params(['domain' => 'example.com; HttpOnly']));
16+
var_dump(session_set_cookie_params(['path' => "/\r\nX-Injected: yes"]));
17+
var_dump(ini_set('session.cookie_domain', "example.com\tevil"));
18+
19+
$params = session_get_cookie_params();
20+
var_dump($params['path'], $params['domain']);
21+
22+
var_dump(session_set_cookie_params(['path' => '/foo,bar', 'domain' => 'example.com']));
23+
$params = session_get_cookie_params();
24+
var_dump($params['path'], $params['domain']);
25+
26+
?>
27+
--EXPECTF--
28+
Warning: session_set_cookie_params(): "session.cookie_path" must not contain any of the following characters ";\r\n" in %s on line %d
29+
bool(false)
30+
31+
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
32+
bool(false)
33+
34+
Warning: session_set_cookie_params(): "session.cookie_path" must not contain any of the following characters ";\r\n" in %s on line %d
35+
bool(false)
36+
37+
Warning: ini_set(): "session.cookie_domain" must not contain any of the following characters ",; \t\r\n\013\014" in %s on line %d
38+
bool(false)
39+
string(1) "/"
40+
string(0) ""
41+
bool(true)
42+
string(8) "/foo,bar"
43+
string(11) "example.com"

0 commit comments

Comments
 (0)