Skip to content

Commit d1d79ac

Browse files
jorgsowajorgsowa
committed
Validate SameSite cookie attribute against allowed values
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 5e45c17 commit d1d79ac

8 files changed

Lines changed: 115 additions & 28 deletions

‎ext/session/session.c‎

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

736+
static PHP_INI_MH(OnUpdateSessionSameSite)
737+
{
738+
SESSION_CHECK_ACTIVE_STATE;
739+
SESSION_CHECK_OUTPUT_STATE;
740+
741+
if (new_value && ZSTR_LEN(new_value) > 0 && !php_is_valid_samesite_value(new_value)) {
742+
php_error_docref(NULL, E_WARNING,
743+
"session.cookie_samesite must be \"Strict\", \"Lax\", \"None\", or \"\"");
744+
return FAILURE;
745+
}
746+
747+
return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
748+
}
749+
736750
static PHP_INI_MH(OnUpdateSessionBool)
737751
{
738752
SESSION_CHECK_ACTIVE_STATE;
@@ -904,7 +918,7 @@ PHP_INI_BEGIN()
904918
STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals)
905919
STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals)
906920
STD_PHP_INI_BOOLEAN("session.cookie_httponly", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals)
907-
STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_samesite, php_ps_globals, ps_globals)
921+
STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionSameSite, cookie_samesite, php_ps_globals, ps_globals)
908922
STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals)
909923
STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals)
910924
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_variation6.phpt‎

Lines changed: 26 additions & 18 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,44 @@ session
1111

1212
ob_start();
1313

14-
echo "*** Testing session_set_cookie_params() : variation ***\n";
15-
16-
var_dump(ini_get("session.cookie_samesite"));
17-
var_dump(session_set_cookie_params(["samesite" => "nothing"]));
14+
// Valid values
1815
var_dump(ini_get("session.cookie_samesite"));
19-
var_dump(session_start());
16+
var_dump(session_set_cookie_params(["samesite" => "Strict"]));
2017
var_dump(ini_get("session.cookie_samesite"));
21-
var_dump(session_set_cookie_params(["samesite" => "test"]));
18+
var_dump(session_set_cookie_params(["samesite" => "None"]));
2219
var_dump(ini_get("session.cookie_samesite"));
23-
var_dump(session_destroy());
20+
var_dump(session_set_cookie_params(["samesite" => ""]));
2421
var_dump(ini_get("session.cookie_samesite"));
25-
var_dump(session_set_cookie_params(["samesite" => "other"]));
22+
23+
// Invalid value
24+
var_dump(session_set_cookie_params(["samesite" => "Invalid"]));
2625
var_dump(ini_get("session.cookie_samesite"));
2726

27+
// Cannot change while session is active
28+
var_dump(session_set_cookie_params(["samesite" => "Lax"]));
29+
var_dump(session_start());
30+
var_dump(session_set_cookie_params(["samesite" => "Strict"]));
31+
var_dump(session_destroy());
32+
2833
echo "Done";
2934
ob_end_flush();
3035
?>
3136
--EXPECTF--
32-
*** Testing session_set_cookie_params() : variation ***
33-
string(4) "test"
37+
string(3) "Lax"
38+
bool(true)
39+
string(6) "Strict"
3440
bool(true)
35-
string(7) "nothing"
41+
string(4) "None"
3642
bool(true)
37-
string(7) "nothing"
43+
string(0) ""
3844

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
45+
Warning: session_set_cookie_params(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d
4046
bool(false)
41-
string(7) "nothing"
47+
string(0) ""
48+
bool(true)
4249
bool(true)
43-
string(7) "nothing"
50+
51+
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
52+
bool(false)
4453
bool(true)
45-
string(5) "other"
4654
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
@@ -76,6 +76,13 @@ PHPAPI bool php_header(void)
7676
}
7777
}
7878

79+
PHPAPI bool php_is_valid_samesite_value(zend_string *value)
80+
{
81+
return zend_string_equals_literal_ci(value, "Strict")
82+
|| zend_string_equals_literal_ci(value, "Lax")
83+
|| zend_string_equals_literal_ci(value, "None");
84+
}
85+
7986
#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\014\""
8087
PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires,
8188
zend_string *path, zend_string *domain, bool secure, bool httponly,
@@ -123,7 +130,11 @@ PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t e
123130
return FAILURE;
124131
}
125132

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

128139
if (value == NULL || ZSTR_LEN(value) == 0) {
129140
/*

‎ext/standard/head.h‎

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

29+
PHPAPI bool php_is_valid_samesite_value(zend_string *value);
30+
2931
extern PHP_RINIT_FUNCTION(head);
3032

3133
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)