From 34fbf71cdd8496ff0cbd7abea5a56fb84a9017dc Mon Sep 17 00:00:00 2001 From: David Carlier Date: Mon, 14 Oct 2024 23:00:47 +0100 Subject: [PATCH 1/4] ext/session: Fix cache_expire ini overflow/underflow. Setting with PHP_INT_MIN/PHP_INT_MAX lead to these. ``` ext/session/session.c:1181:37: runtime error: signed integer overflow: -9223372036854775808 * 60 cannot be represented in type 'long int' ext/session/session.c:1181:37: runtime error: signed integer overflow: 9223372036854775807 * 60 cannot be represented in type 'long int' ``` --- ext/session/session.c | 19 ++++++++++++- .../tests/session_cache_expire_oflow.phpt | 27 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 ext/session/tests/session_cache_expire_oflow.phpt diff --git a/ext/session/session.c b/ext/session/session.c index dd780f4afd424..b27ded48388d2 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -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 + zend_long v = (zend_long)atol(ZSTR_VAL(new_value)); + if (v < 0 || v > maxexpire) { + return SUCCESS; + } + return OnUpdateLongGEZero(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); +} + static PHP_INI_MH(OnUpdateSessionLong) /* {{{ */ { @@ -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) diff --git a/ext/session/tests/session_cache_expire_oflow.phpt b/ext/session/tests/session_cache_expire_oflow.phpt new file mode 100644 index 0000000000000..8c975436e9463 --- /dev/null +++ b/ext/session/tests/session_cache_expire_oflow.phpt @@ -0,0 +1,27 @@ +--TEST-- +session_cache_expire() overflow +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECT-- +*** Testing session_cache_expire() : overflow test *** +int(180) +int(10800) +Done + From daf1cf66d83812e1a93f5d77bb491451649e9933 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Tue, 15 Oct 2024 05:01:00 +0100 Subject: [PATCH 2/4] fix tests --- ext/session/tests/session_cache_expire_basic.phpt | 4 ++-- ext/session/tests/session_cache_expire_oflow.phpt | 6 +++--- ext/session/tests/session_cache_expire_variation1.phpt | 4 ++-- ext/session/tests/session_cache_expire_variation2.phpt | 4 ++-- ext/session/tests/session_cache_expire_variation3.phpt | 6 +++--- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/ext/session/tests/session_cache_expire_basic.phpt b/ext/session/tests/session_cache_expire_basic.phpt index 9873b80da2b58..dd5964e0a362f 100644 --- a/ext/session/tests/session_cache_expire_basic.phpt +++ b/ext/session/tests/session_cache_expire_basic.phpt @@ -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) diff --git a/ext/session/tests/session_cache_expire_oflow.phpt b/ext/session/tests/session_cache_expire_oflow.phpt index 8c975436e9463..39a6cd13f7dbc 100644 --- a/ext/session/tests/session_cache_expire_oflow.phpt +++ b/ext/session/tests/session_cache_expire_oflow.phpt @@ -12,16 +12,16 @@ ob_start(); echo "*** Testing session_cache_expire() : overflow test ***\n"; session_cache_limiter("public"); -var_dump(session_cache_expire(PHP_INT_MAX)); +var_dump(session_cache_expire((int)(PHP_INT_MAX/60))); session_start(); var_dump(session_cache_expire() * 60); echo "Done"; ob_end_flush(); ?> ---EXPECT-- +--EXPECTF-- *** Testing session_cache_expire() : overflow test *** int(180) -int(10800) +int(%s) Done diff --git a/ext/session/tests/session_cache_expire_variation1.phpt b/ext/session/tests/session_cache_expire_variation1.phpt index 6383092f56257..044275baee627 100644 --- a/ext/session/tests/session_cache_expire_variation1.phpt +++ b/ext/session/tests/session_cache_expire_variation1.phpt @@ -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) diff --git a/ext/session/tests/session_cache_expire_variation2.phpt b/ext/session/tests/session_cache_expire_variation2.phpt index e3c598affb8fb..04cf13323ab72 100644 --- a/ext/session/tests/session_cache_expire_variation2.phpt +++ b/ext/session/tests/session_cache_expire_variation2.phpt @@ -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) diff --git a/ext/session/tests/session_cache_expire_variation3.phpt b/ext/session/tests/session_cache_expire_variation3.phpt index a243418001f34..8674dab2b15bd 100644 --- a/ext/session/tests/session_cache_expire_variation3.phpt +++ b/ext/session/tests/session_cache_expire_variation3.phpt @@ -26,7 +26,7 @@ var_dump(ini_get("session.cache_expire")); echo "Done"; ob_end_flush(); ?> ---EXPECT-- +--EXPECTF-- *** Testing session_cache_expire() : variation *** string(3) "180" int(180) @@ -34,9 +34,9 @@ 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 From 959b242be6e4eb12fd6663eddd3dc1f41505b141 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Tue, 15 Oct 2024 19:56:29 +0100 Subject: [PATCH 3/4] trying out checking before the actual calculation. for now checking time_t size at runtime just for POC purpose.. --- ext/session/session.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index b27ded48388d2..f3bd0fbb430de 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -710,23 +710,6 @@ 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 - zend_long v = (zend_long)atol(ZSTR_VAL(new_value)); - if (v < 0 || v > maxexpire) { - return SUCCESS; - } - return OnUpdateLongGEZero(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); -} - static PHP_INI_MH(OnUpdateSessionLong) /* {{{ */ { @@ -835,7 +818,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, OnUpdateCacheExpire, cache_expire, 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_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) @@ -1192,10 +1175,24 @@ CACHE_LIMITER_FUNC(public) /* {{{ */ { char buf[MAX_STR + 1]; struct timeval tv; - time_t now; + time_t now, max; gettimeofday(&tv, NULL); - now = tv.tv_sec + PS(cache_expire) * 60; + zend_long cache_expire = PS(cache_expire); + + if (sizeof(time_t) == 8 || sizeof(time_t) == 4) { + max = (time_t)((zend_ulong)~0 >> 1); + } else { + max = (time_t)((~(time_t)0) >> 1); + } + + if (cache_expire < 0) { + now = tv.tv_sec; + } else if ((cache_expire / 60) > (max - (zend_long)max)) { + now = max; + } else { + now = tv.tv_sec + cache_expire * 60; + } memcpy(buf, EXPIRES, sizeof(EXPIRES) - 1); strcpy_gmt(buf + sizeof(EXPIRES) - 1, &now); ADD_HEADER(buf); From 00f2dedd340a49373276d867f24b49413e71b0ac Mon Sep 17 00:00:00 2001 From: David Carlier Date: Tue, 15 Oct 2024 22:24:15 +0100 Subject: [PATCH 4/4] compile time knowledge --- build/php.m4 | 1 + ext/session/session.c | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/build/php.m4 b/build/php.m4 index e13fc3367ea1e..e772814f0e12e 100644 --- a/build/php.m4 +++ b/build/php.m4 @@ -2430,6 +2430,7 @@ AC_DEFUN([PHP_CHECK_STDINT_TYPES], [ AC_CHECK_SIZEOF([long long]) AC_CHECK_SIZEOF([size_t]) AC_CHECK_SIZEOF([off_t]) + AC_CHECK_SIZEOF([time_t]) AC_CHECK_TYPES([int8_t, int16_t, int32_t, int64_t, uint8_t, uint16_t, uint32_t, uint64_t], [], [ AC_MSG_ERROR([One of the intN_t or uintN_t types is not available]) ], [ diff --git a/ext/session/session.c b/ext/session/session.c index f3bd0fbb430de..00d8c81cda729 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -1175,16 +1175,16 @@ CACHE_LIMITER_FUNC(public) /* {{{ */ { char buf[MAX_STR + 1]; struct timeval tv; - time_t now, max; + time_t now; gettimeofday(&tv, NULL); zend_long cache_expire = PS(cache_expire); - if (sizeof(time_t) == 8 || sizeof(time_t) == 4) { - max = (time_t)((zend_ulong)~0 >> 1); - } else { - max = (time_t)((~(time_t)0) >> 1); - } +#if SIZEOF_TIME_T == 4 || SIZEOF_TIME_T == 8 + const time_t max = (time_t)((zend_ulong)~0 >> 1); +#else + const time_t max = (time_t)((~(time_t)0) >> 1); +#endif if (cache_expire < 0) { now = tv.tv_sec;