Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 1 commit into
base: master
Choose a base branch
from
+66 −2
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
iliaal marked this conversation as resolved.
$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
Expand Down
19 changes: 17 additions & 2 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
43 changes: 43 additions & 0 deletions ext/session/tests/session_cookie_params_forbidden_chars.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

@Sjord Sjord Aug 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these characters seem as they may be legitimately used in paths. It could be possible to have a path with a space or a comma in it.

Perhaps https://datatracker.ietf.org/doc/html/rfc6265 can be used to more precisely determine which characters can be in which field.

@iliaal iliaal Aug 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC 6265 Path allows space and comma; Domain does not. Browsers split cookie-av on ;.

This copies setcookie()'s path/domain check. Loosening session.cookie_path alone would accept values setcookie() still rejects.

@Sjord Sjord Aug 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Making this consistent with setcookie makes sense. Even so, there is a slight risk of BC break when someone configured /my app/ as their cookie path and that breaks when upgrading. I think rejecting ; is fine because that was an obvious invalid value and wouldn't have worked. But this does not hold for rejecting space or comma.

@iliaal iliaal Aug 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not hold for rejecting space or comma

setcookie() has rejected both in path since 7.3. I'd rather keep that set than special-case session.cookie_path.

@Sjord Sjord Aug 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

someone configured /my app/ as their cookie path and that breaks when upgrading

I was wrong about this. Having a space in the cookie path doesn't work. It seems that the browser automatically encodes the space in the path to %20, so the cookie path should also have %20 for it to match.

A comma works though. I consider it less likely that people use a comma in their path, but having session.cookie_path=foo,bar works and would break when this PR is merged.

@cmb69 do you have any thoughts on this?

@Sjord Sjord Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that @cmb69 copied the validation from cookie value to cookie path. This is not entirely correct, since these can contain different characters according to the RFC. I don't think it is a good idea to persist the mistake into the INI values.

I can see the following ways forward:

  • @cmb69 explains why the validation in setcookie actually is correct, instead of a copy-paste error like I assumed.
  • the validation for the INI setting is changed to conform to the RFC (optionally also the validation for setcookie).
  • we discuss on the internals mailing list or even via an PHP RFC what the validation should be.

@iliaal iliaal Sep 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a copy-paste error. 5cb825d's message is explicit: "characters not allowed for name and value should also be forbidden for path and domain."

Comma in a path is RFC-legal, but RFC 6265 section 3 bans folding Set-Cookie precisely because "," conflicts with standard header folding, so anything that folds or re-splits on commas misreads a comma'd path. I'd keep setcookie's set here; relaxing both to the RFC grammar is a setcookie() question, and this INI handler would follow it.

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"
Loading