Skip to content

ext/session: Fix cache_expire ini overflow/underflow. #16445

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

Draft
wants to merge 4 commits into
base: PHP-8.2
Choose a base branch
from
Draft

ext/session: Fix cache_expire ini overflow/underflow. #16445

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34fbf71
ext/session: Fix cache_expire ini overflow/underflow.
devnexen Oct 14, 2024
daf1cf6
fix tests
devnexen Oct 15, 2024
959b242
trying out checking before the actual calculation.
devnexen Oct 15, 2024
00f2ded
compile time knowledge
devnexen Oct 15, 2024
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
19 changes: 18 additions & 1 deletion ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,23 @@ static PHP_INI_MH(OnUpdateCookieLifetime) /* {{{ */
}
/* }}} */

static PHP_INI_MH(OnUpdateCacheExpire)
{
SESSION_CHECK_ACTIVE_STATE;
SESSION_CHECK_OUTPUT_STATE;

#ifdef ZEND_ENABLE_ZVAL_LONG64
const zend_long maxexpire = ((ZEND_LONG_MAX - INT_MAX) / 60) - 1;
#else
const zend_long maxexpire = ((ZEND_LONG_MAX / 2) / 60) - 1;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this is highly platform dependent, since the C standard makes no claim about time_t other than that it is an arithmetic type (could even be float/double). Current POSIX states "time_t shall be an integer type with a width of at least 64 bits". Now, that it not true for our x86 Windows builds (it's 32bit wide there); I don't know about other systems.

To handle this cleanly, we should probably determine something like TIME_MIN and TIME_MAX during configuration (and deal one way or another with non integral time_t implementations).

Copy link
Member Author

@devnexen devnexen Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Concretally, even QNX does not implement time_t as float/double.

zend_long v = (zend_long)atol(ZSTR_VAL(new_value));
if (v < 0 || v > maxexpire) {
return SUCCESS;
}
Copy link
Member

Choose a reason for hiding this comment

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

Surely this should be return FAILURE ?

Copy link
Member Author

@devnexen devnexen Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In fact I applied the same "policy" as for cookie_lifetime as to not disturb things for stable branches but I can return FAILURE sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think this strategy makes sense.

However, the approach appears to be insufficient. For 32bit architectures we have ((ZEND_LONG_MAX / 2) / 60) - 1;. Asssuming that time_t is a signed 32bit, that can still overflow: let's say the current timestamp is 1728992731; the max value would be 7456539. Then we do tv.tv_sec + PS(cache_expire) * 60, which results in signed overflow. For our Windows 32bit builds, time_t is long which is a signed 32bit value on LLP64.

It seems to me that we need to do the overflow check when we're actually calculating the time (maybe instead of failing, just clamping the value). Rejecting very large values in the INI modifcation handler makes sense, but doesn't solve all issues; not even for 64bit architectures when running this code in say 15 years.

Copy link
Member Author

Choose a reason for hiding this comment

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

alright thx, I ll get back to it in couple of hours.

return OnUpdateLongGEZero(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
}


static PHP_INI_MH(OnUpdateSessionLong) /* {{{ */
{
Expand Down Expand Up @@ -818,7 +835,7 @@ PHP_INI_BEGIN()
STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.referer_check", "", PHP_INI_ALL, OnUpdateSessionString, extern_referer_chk, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cache_limiter", "nocache", PHP_INI_ALL, OnUpdateSessionString, cache_limiter, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cache_expire", "180", PHP_INI_ALL, OnUpdateSessionLong, cache_expire, php_ps_globals, ps_globals)
STD_PHP_INI_ENTRY("session.cache_expire", "180", PHP_INI_ALL, OnUpdateCacheExpire, cache_expire, php_ps_globals, ps_globals)
STD_PHP_INI_BOOLEAN("session.use_trans_sid", "0", PHP_INI_ALL, OnUpdateSessionBool, use_trans_sid, php_ps_globals, ps_globals)
PHP_INI_ENTRY("session.sid_length", "32", PHP_INI_ALL, OnUpdateSidLength)
PHP_INI_ENTRY("session.sid_bits_per_character", "4", PHP_INI_ALL, OnUpdateSidBits)
Expand Down
4 changes: 2 additions & 2 deletions ext/session/tests/session_cache_expire_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ var_dump(session_cache_expire());
echo "Done";
ob_end_flush();
?>
--EXPECT--
--EXPECTF--
*** Testing session_cache_expire() : basic functionality ***
int(180)
int(180)
int(1234567890)
int(%d)
bool(true)
int(180)
bool(true)
Expand Down
27 changes: 27 additions & 0 deletions ext/session/tests/session_cache_expire_oflow.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
session_cache_expire() overflow
--EXTENSIONS--
session
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php

ob_start();

echo "*** Testing session_cache_expire() : overflow test ***\n";

session_cache_limiter("public");
var_dump(session_cache_expire((int)(PHP_INT_MAX/60)));
session_start();
var_dump(session_cache_expire() * 60);

echo "Done";
ob_end_flush();
?>
--EXPECTF--
*** Testing session_cache_expire() : overflow test ***
int(180)
int(%s)
Done

4 changes: 2 additions & 2 deletions ext/session/tests/session_cache_expire_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ var_dump(session_cache_expire());
echo "Done";
ob_end_flush();
?>
--EXPECT--
--EXPECTF--
*** Testing session_cache_expire() : variation ***
int(360)
int(360)
int(1234567890)
int(%d)
bool(true)
int(180)
bool(true)
Expand Down
4 changes: 2 additions & 2 deletions ext/session/tests/session_cache_expire_variation2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ var_dump(session_cache_expire());
echo "Done";
ob_end_flush();
?>
--EXPECT--
--EXPECTF--
*** Testing session_cache_expire() : variation ***
int(360)
int(360)
int(1234567890)
int(%d)
bool(true)
int(180)
bool(true)
Expand Down
6 changes: 3 additions & 3 deletions ext/session/tests/session_cache_expire_variation3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ var_dump(ini_get("session.cache_expire"));
echo "Done";
ob_end_flush();
?>
--EXPECT--
--EXPECTF--
*** Testing session_cache_expire() : variation ***
string(3) "180"
int(180)
string(3) "180"
int(180)
string(10) "1234567890"
bool(true)
int(1234567890)
int(%d)
string(10) "1234567890"
bool(true)
int(1234567890)
int(%d)
string(10) "1234567890"
Done
Loading