Skip to content

Commit c130d42

Browse files
jorgsowajorgsowa
authored
Validate SameSite cookie attribute against allowed values (#21670)
Extract php_is_valid_samesite_value() in ext/standard/head.c as a shared validation function that enforces the SameSite whitelist (Strict, Lax, None, or empty string) with case-insensitive matching. Apply validation in both setcookie()/setrawcookie() (replacing the existing TODO comment) and the session.cookie_samesite INI handler. Previously arbitrary strings including CRLF sequences were accepted and appended verbatim into the Set-Cookie header.
1 parent b9f64f7 commit c130d42

9 files changed

Lines changed: 148 additions & 27 deletions

‎ext/session/session.c‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -739,6 +739,20 @@ static PHP_INI_MH(OnUpdateSessionStr)
739739
return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
740740
}
741741

742+
static PHP_INI_MH(OnUpdateSessionSameSite)
743+
{
744+
SESSION_CHECK_ACTIVE_STATE;
745+
SESSION_CHECK_OUTPUT_STATE;
746+
747+
if (new_value && ZSTR_LEN(new_value) > 0 && !php_is_valid_samesite_value(new_value)) {
748+
php_error_docref(NULL, E_WARNING,
749+
"session.cookie_samesite must be \"Strict\", \"Lax\", \"None\", or \"\"");
750+
return FAILURE;
751+
}
752+
753+
return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
754+
}
755+
742756
static PHP_INI_MH(OnUpdateSessionBool)
743757
{
744758
SESSION_CHECK_ACTIVE_STATE;
@@ -910,7 +924,7 @@ PHP_INI_BEGIN()
910924
STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals)
911925
STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)
912926
STD_PHP_INI_BOOLEAN("session.cookie_httponly", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals)
913-
STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_samesite, php_ps_globals, ps_globals)
927+
STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionSameSite, cookie_samesite, php_ps_globals, ps_globals)
914928
STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals)
915929
STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals)
916930
STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals)

‎ext/session/tests/session_get_cookie_params_basic.phpt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ var_dump(session_set_cookie_params([
3030
"domain" => "baz",
3131
"secure" => FALSE,
3232
"httponly" => FALSE,
33-
"samesite" => "please"]));
33+
"samesite" => "Strict"]));
3434
var_dump(session_get_cookie_params());
3535
var_dump(session_set_cookie_params([
3636
"secure" => TRUE,
@@ -107,7 +107,7 @@ array(7) {
107107
["httponly"]=>
108108
bool(false)
109109
["samesite"]=>
110-
string(6) "please"
110+
string(6) "Strict"
111111
}
112112
bool(true)
113113
array(7) {
@@ -124,6 +124,6 @@ array(7) {
124124
["httponly"]=>
125125
bool(false)
126126
["samesite"]=>
127-
string(6) "please"
127+
string(6) "Strict"
128128
}
129129
Done

‎ext/session/tests/session_get_cookie_params_variation1.phpt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ ini_set("session.cookie_secure", TRUE);
3030
var_dump(session_get_cookie_params());
3131
ini_set("session.cookie_httponly", TRUE);
3232
var_dump(session_get_cookie_params());
33-
ini_set("session.cookie_samesite", "foo");
33+
ini_set("session.cookie_samesite", "Lax");
3434
var_dump(session_get_cookie_params());
3535
ini_set("session.cookie_partitioned", TRUE);
3636
var_dump(session_get_cookie_params());
@@ -150,7 +150,7 @@ array(7) {
150150
["httponly"]=>
151151
bool(true)
152152
["samesite"]=>
153-
string(3) "foo"
153+
string(3) "Lax"
154154
}
155155
array(7) {
156156
["lifetime"]=>
@@ -166,6 +166,6 @@ array(7) {
166166
["httponly"]=>
167167
bool(true)
168168
["samesite"]=>
169-
string(3) "foo"
169+
string(3) "Lax"
170170
}
171171
Done

‎ext/session/tests/session_set_cookie_params_invalid_ini.phpt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
Test session.cookie_samesite with invalid INI value
3+
--INI--
4+
session.cookie_samesite=Invalid
5+
--EXTENSIONS--
6+
session
7+
--SKIPIF--
8+
<?php include('skipif.inc'); ?>
9+
--FILE--
10+
<?php
11+
12+
ob_start();
13+
14+
var_dump(ini_get("session.cookie_samesite"));
15+
16+
echo "Done";
17+
ob_end_flush();
18+
?>
19+
--EXPECTF--
20+
Warning: PHP Startup: session.cookie_samesite must be "Strict", "Lax", "None", or "" in Unknown on line 0
21+
string(0) ""
22+
Done

‎ext/session/tests/session_set_cookie_params_variation6.phpt‎

Lines changed: 37 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
--TEST--
2-
Test session_set_cookie_params() function : variation
2+
Test session_set_cookie_params() samesite validation
33
--INI--
4-
session.cookie_samesite=test
4+
session.cookie_samesite=Lax
55
--EXTENSIONS--
66
session
77
--SKIPIF--
@@ -11,36 +11,56 @@ session
1111

1212
ob_start();
1313

14-
echo "*** Testing session_set_cookie_params() : variation ***\n";
15-
14+
echo "-- Valid values --\n";
1615
var_dump(ini_get("session.cookie_samesite"));
17-
var_dump(session_set_cookie_params(["samesite" => "nothing"]));
16+
var_dump(session_set_cookie_params(["samesite" => "Strict"]));
1817
var_dump(ini_get("session.cookie_samesite"));
19-
var_dump(session_start());
18+
var_dump(session_set_cookie_params(["samesite" => "None"]));
2019
var_dump(ini_get("session.cookie_samesite"));
21-
var_dump(session_set_cookie_params(["samesite" => "test"]));
20+
var_dump(session_set_cookie_params(["samesite" => ""]));
2221
var_dump(ini_get("session.cookie_samesite"));
23-
var_dump(session_destroy());
22+
23+
echo "-- Invalid value via session_set_cookie_params --\n";
24+
var_dump(session_set_cookie_params(["samesite" => "Invalid"]));
2425
var_dump(ini_get("session.cookie_samesite"));
25-
var_dump(session_set_cookie_params(["samesite" => "other"]));
26+
27+
echo "-- Invalid value via ini_set --\n";
28+
var_dump(ini_set("session.cookie_samesite", "Invalid"));
2629
var_dump(ini_get("session.cookie_samesite"));
2730

31+
echo "-- Cannot change while session is active --\n";
32+
var_dump(session_set_cookie_params(["samesite" => "Lax"]));
33+
var_dump(session_start());
34+
var_dump(session_set_cookie_params(["samesite" => "Strict"]));
35+
var_dump(session_destroy());
36+
2837
echo "Done";
2938
ob_end_flush();
3039
?>
3140
--EXPECTF--
32-
*** Testing session_set_cookie_params() : variation ***
33-
string(4) "test"
41+
-- Valid values --
42+
string(3) "Lax"
3443
bool(true)
35-
string(7) "nothing"
44+
string(6) "Strict"
3645
bool(true)
37-
string(7) "nothing"
46+
string(4) "None"
47+
bool(true)
48+
string(0) ""
49+
-- Invalid value via session_set_cookie_params --
3850

39-
Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d
51+
Warning: session_set_cookie_params(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d
52+
bool(false)
53+
string(0) ""
54+
-- Invalid value via ini_set --
55+
56+
Warning: ini_set(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d
4057
bool(false)
41-
string(7) "nothing"
58+
string(0) ""
59+
-- Cannot change while session is active --
4260
bool(true)
43-
string(7) "nothing"
4461
bool(true)
45-
string(5) "other"
62+
63+
Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d
64+
bool(false)
65+
bool(true)
4666
Done

‎ext/session/tests/session_set_cookie_params_variation7.phpt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ try {
3333

3434
var_dump(ini_get("session.cookie_secure"));
3535
var_dump(ini_get("session.cookie_samesite"));
36-
var_dump(session_set_cookie_params(["secure" => true, "samesite" => "please"]));
36+
var_dump(session_set_cookie_params(["secure" => true, "samesite" => "Strict"]));
3737
var_dump(ini_get("session.cookie_secure"));
3838
var_dump(ini_get("session.cookie_samesite"));
3939

@@ -66,7 +66,7 @@ string(1) "0"
6666
string(0) ""
6767
bool(true)
6868
string(1) "1"
69-
string(6) "please"
69+
string(6) "Strict"
7070
string(1) "0"
7171
bool(true)
7272
string(2) "42"

‎ext/standard/head.c‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,13 @@ PHPAPI bool php_header(void)
7474
}
7575
}
7676

77+
PHPAPI bool php_is_valid_samesite_value(zend_string *value)
78+
{
79+
return zend_string_equals_literal_ci(value, "Strict")
80+
|| zend_string_equals_literal_ci(value, "Lax")
81+
|| zend_string_equals_literal_ci(value, "None");
82+
}
83+
7784
#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\014\""
7885
PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires,
7986
zend_string *path, zend_string *domain, bool secure, bool httponly,
@@ -121,7 +128,11 @@ PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t e
121128
return FAILURE;
122129
}
123130

124-
/* Should check value of SameSite? */
131+
if (samesite && ZSTR_LEN(samesite) > 0 && !php_is_valid_samesite_value(samesite)) {
132+
zend_value_error("%s(): \"samesite\" option must be \"Strict\", \"Lax\", \"None\", or \"\"",
133+
get_active_function_name());
134+
return FAILURE;
135+
}
125136

126137
if (value == NULL || ZSTR_LEN(value) == 0) {
127138
/*

‎ext/standard/head.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
#define COOKIE_SAMESITE "; SameSite="
2525
#define COOKIE_PARTITIONED "; Partitioned"
2626

27+
PHPAPI bool php_is_valid_samesite_value(zend_string *value);
28+
2729
extern PHP_RINIT_FUNCTION(head);
2830

2931
PHPAPI bool php_header(void);

‎ext/standard/tests/setcookie_samesite_validation.phpt‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
setcookie() and setrawcookie() validate samesite option
3+
--FILE--
4+
<?php
5+
ob_start();
6+
7+
// Valid values
8+
var_dump(setcookie('test', 'value', ['samesite' => 'Strict']));
9+
var_dump(setcookie('test', 'value', ['samesite' => 'Lax']));
10+
var_dump(setcookie('test', 'value', ['samesite' => 'None']));
11+
var_dump(setcookie('test', 'value', ['samesite' => '']));
12+
13+
// Case-insensitive
14+
var_dump(setcookie('test', 'value', ['samesite' => 'strict']));
15+
var_dump(setcookie('test', 'value', ['samesite' => 'LAX']));
16+
var_dump(setcookie('test', 'value', ['samesite' => 'NONE']));
17+
18+
// setrawcookie uses the same validation
19+
var_dump(setrawcookie('test', 'value', ['samesite' => 'Lax']));
20+
21+
// Invalid values
22+
try {
23+
setcookie('test', 'value', ['samesite' => 'Invalid']);
24+
} catch (ValueError $e) {
25+
echo $e->getMessage() . "\n";
26+
}
27+
28+
try {
29+
setcookie('test', 'value', ['samesite' => "Strict\r\nX-Injected: evil"]);
30+
} catch (ValueError $e) {
31+
echo $e->getMessage() . "\n";
32+
}
33+
34+
try {
35+
setrawcookie('test', 'value', ['samesite' => 'Invalid']);
36+
} catch (ValueError $e) {
37+
echo $e->getMessage() . "\n";
38+
}
39+
40+
?>
41+
--EXPECTF--
42+
bool(true)
43+
bool(true)
44+
bool(true)
45+
bool(true)
46+
bool(true)
47+
bool(true)
48+
bool(true)
49+
bool(true)
50+
setcookie(): "samesite" option must be "Strict", "Lax", "None", or ""
51+
setcookie(): "samesite" option must be "Strict", "Lax", "None", or ""
52+
setrawcookie(): "samesite" option must be "Strict", "Lax", "None", or ""

0 commit comments

Comments
 (0)