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

Merged
Girgias merged 5 commits into from
Apr 20, 2026
Merged

ext/session: fix cookie_lifetime overflow #21704

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 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
481e9b7
chore: modify error flow and messages of warnings
jorgsowa Apr 13, 2026
83991b6
fix: return FAILURE on overflow
jorgsowa Apr 14, 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
22 changes: 18 additions & 4 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) {
php_error_docref(NULL, E_WARNING, "CookieLifetime cannot be negative");
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 (UNEXPECTED(type != IS_LONG)) {
if (oflow != 0) {
php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie);
} else {
php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be an integer");
Copy link
Copy Markdown
Member

@Girgias Girgias Apr 14, 2026

Choose a reason for hiding this comment

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

The usual error message is something along the line of must be of type int.

Copy link
Copy Markdown
Contributor Author

@jorgsowa jorgsowa Apr 14, 2026

Choose a reason for hiding this comment

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

Applied + created an addition to CODING_CONVENTIONS.md in #21761

}
return FAILURE;
}
if (lval < 0) {
php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie);
return FAILURE;
} else if (v > maxcookie) {
} else if (lval > maxcookie) {
php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT ", value clamped to maximum", maxcookie);
zend_long *p = ZEND_INI_GET_ADDR();
*p = maxcookie;
entry->value = zend_long_to_str(maxcookie);
return SUCCESS;
}
Copy link
Copy Markdown
Member

@Girgias Girgias Apr 14, 2026

Choose a reason for hiding this comment

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

I think it was a bug before for it to return SUCCESS, so I would rather have it return FAILURE and warn then silently change behaviour.

Effectively just change the prior if condition to if (lval < 0 || lval > maxcookie) {

Copy link
Copy Markdown
Contributor Author

@jorgsowa jorgsowa Apr 14, 2026

Choose a reason for hiding this comment

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

Sure. It changes logic and may break some implementations, but it's fine to me.


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(): session.cookie_lifetime must be between 0 and %d, value clamped to maximum 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(): session.cookie_lifetime must be an integer in %s on line %d
string(3) "100"

Warning: ini_set(): session.cookie_lifetime must be an integer in %s on line %d
string(3) "100"

Warning: ini_set(): session.cookie_lifetime must be between 0 and %d in %s on line %d
string(3) "100"

Warning: ini_set(): session.cookie_lifetime must be between 0 and %d 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(): session.cookie_lifetime must be between 0 and %d, value clamped to maximum 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
2 changes: 1 addition & 1 deletion ext/session/tests/session_set_cookie_params_variation8.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ bool(true)
string(1) "0"
string(1) "0"

Warning: session_set_cookie_params(): CookieLifetime cannot be negative in %s on line %d
Warning: session_set_cookie_params(): session.cookie_lifetime must be between 0 and %d in %s on line %d
bool(false)
string(1) "0"
Done
Loading