Skip to content

ext/session: fix cookie_lifetime overflow #21704

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
jorgsowa wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ext/session: fix cookie_lifetime overflow #21704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29eb822
ext/session: fix cookie_lifetime overflow causing INI value desync
jorgsowa Apr 10, 2026
2860ac4
validation for cookie lifetime
jorgsowa Apr 10, 2026
070407c
removed unnecessary comments
jorgsowa Apr 11, 2026
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
20 changes: 17 additions & 3 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,11 +706,25 @@ static PHP_INI_MH(OnUpdateCookieLifetime)
#else
const zend_long maxcookie = ZEND_LONG_MAX / 2 - 1;
#endif
zend_long v = (zend_long)atol(ZSTR_VAL(new_value));
if (v < 0) {
zend_long lval = 0;
int oflow = 0;
uint8_t type = is_numeric_string_ex(ZSTR_VAL(new_value), ZSTR_LEN(new_value), &lval, NULL, false, &oflow, NULL);
if (type == 0) {
php_error_docref(NULL, E_WARNING, "Invalid value for CookieLifetime");
return FAILURE;
} else if (type == IS_DOUBLE && oflow == 0) {
php_error_docref(NULL, E_WARNING, "CookieLifetime must be an integer");
return FAILURE;
}
Comment on lines +712 to +718
Copy link
Copy Markdown
Member

@Girgias Girgias Apr 13, 2026

Choose a reason for hiding this comment

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

Maybe move this into an if (UNEXPECTED(type != IS_LONG)) {} block?

zend_long v = lval;
if (oflow < 0 || v < 0) {
php_error_docref(NULL, E_WARNING, "CookieLifetime cannot be negative");
Copy link
Copy Markdown
Member

@Girgias Girgias Apr 13, 2026

Choose a reason for hiding this comment

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

Maybe this error could be updated to the more standard wording of must be between 0 and maxcookie?

return FAILURE;
} else if (v > maxcookie) {
} else if (oflow > 0 || v > maxcookie) {
php_error_docref(NULL, E_WARNING, "CookieLifetime value too large, value was set to the maximum of " ZEND_LONG_FMT, maxcookie);
zend_long *p = ZEND_INI_GET_ADDR();
*p = maxcookie;
entry->value = zend_long_to_str(maxcookie);
return SUCCESS;
Comment on lines +723 to 728
Copy link
Copy Markdown
Member

@Girgias Girgias Apr 13, 2026

Choose a reason for hiding this comment

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

Did this use to actually update the INI setting or was it just returning SUCCESS even if it was failing? :|

}

Expand Down
5 changes: 4 additions & 1 deletion ext/session/tests/gh16290.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ session
<?php include('skipif.inc'); ?>
--FILE--
<?php
ob_start();
session_set_cookie_params(PHP_INT_MAX, '/', null, false, true);
echo "DONE";
ob_end_flush();
?>
--EXPECT--
--EXPECTF--
Warning: session_set_cookie_params(): CookieLifetime value too large, value was set to the maximum of %d in %s on line %d
DONE
43 changes: 43 additions & 0 deletions ext/session/tests/session_cookie_lifetime_invalid.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
--TEST--
session.cookie_lifetime rejects non-integer values
--EXTENSIONS--
session
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php

ob_start();

ini_set("session.cookie_lifetime", 100);

// Float strings are rejected
ini_set("session.cookie_lifetime", "1.5");
var_dump(ini_get("session.cookie_lifetime"));

// Non-numeric strings are rejected
ini_set("session.cookie_lifetime", "abc");
var_dump(ini_get("session.cookie_lifetime"));

// Negative values are rejected
ini_set("session.cookie_lifetime", -1);
var_dump(ini_get("session.cookie_lifetime"));

// Negative overflow strings are rejected
ini_set("session.cookie_lifetime", "-99999999999999999999");
var_dump(ini_get("session.cookie_lifetime"));

ob_end_flush();
?>
--EXPECTF--
Warning: ini_set(): CookieLifetime must be an integer in %s on line %d
string(3) "100"

Warning: ini_set(): Invalid value for CookieLifetime in %s on line %d
string(3) "100"

Warning: ini_set(): CookieLifetime cannot be negative in %s on line %d
string(3) "100"

Warning: ini_set(): CookieLifetime cannot be negative in %s on line %d
string(3) "100"
34 changes: 34 additions & 0 deletions ext/session/tests/session_cookie_lifetime_overflow.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
--TEST--
session.cookie_lifetime overflow value is clamped
--EXTENSIONS--
session
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php

ob_start();

// Set a valid value first
ini_set("session.cookie_lifetime", 100);
var_dump(ini_get("session.cookie_lifetime"));

// Set an overflow value - should succeed with warning, value clamped
ini_set("session.cookie_lifetime", PHP_INT_MAX);
$val = (int) ini_get("session.cookie_lifetime");
var_dump($val < PHP_INT_MAX); // clamped, not PHP_INT_MAX
var_dump($val > 0);

// Valid values still work after clamping
ini_set("session.cookie_lifetime", 200);
var_dump(ini_get("session.cookie_lifetime"));

ob_end_flush();
?>
--EXPECTF--
string(3) "100"

Warning: ini_set(): CookieLifetime value too large, value was set to the maximum of %d in %s on line %d
bool(true)
bool(true)
string(3) "200"
6 changes: 3 additions & 3 deletions ext/session/tests/session_get_cookie_params_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ echo "*** Testing session_get_cookie_params() : basic functionality ***\n";
var_dump(session_get_cookie_params());
var_dump(session_set_cookie_params(3600, "/path", "blah", FALSE, FALSE));
var_dump(session_get_cookie_params());
var_dump(session_set_cookie_params(1234567890, "/guff", "foo", TRUE, TRUE));
var_dump(session_set_cookie_params(1000000000, "/guff", "foo", TRUE, TRUE));
var_dump(session_get_cookie_params());
var_dump(session_set_cookie_params([
"lifetime" => 123,
Expand All @@ -40,7 +40,7 @@ var_dump(session_get_cookie_params());
echo "Done";
ob_end_flush();
?>
--EXPECTF--
--EXPECT--
*** Testing session_get_cookie_params() : basic functionality ***
array(7) {
["lifetime"]=>
Expand Down Expand Up @@ -78,7 +78,7 @@ array(7) {
bool(true)
array(7) {
["lifetime"]=>
int(%d)
int(1000000000)
["path"]=>
string(5) "/guff"
["domain"]=>
Expand Down
2 changes: 1 addition & 1 deletion ext/session/tests/session_set_cookie_params_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var_dump(session_set_cookie_params(3600));
var_dump(session_start());
var_dump(session_set_cookie_params(1800));
var_dump(session_destroy());
var_dump(session_set_cookie_params(1234567890));
var_dump(session_set_cookie_params(1000000000));

echo "Done";
ob_end_flush();
Expand Down
4 changes: 2 additions & 2 deletions ext/session/tests/session_set_cookie_params_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ var_dump(ini_get("session.cookie_lifetime"));
var_dump(session_destroy());

var_dump(ini_get("session.cookie_lifetime"));
var_dump(session_set_cookie_params(1234567890));
var_dump(session_set_cookie_params(1000000000));
var_dump(ini_get("session.cookie_lifetime"));

echo "Done";
Expand All @@ -44,5 +44,5 @@ string(4) "3600"
bool(true)
string(4) "3600"
bool(true)
string(10) "1234567890"
string(10) "1000000000"
Done
Loading