Skip to content

ext/session: reject non-numeric session.upload_progress.freq#22579

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: reject non-numeric session.upload_progress.freq #22579

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
4dc40ed
ext/session: reject non-numeric session.upload_progress.freq
jorgsowa Jul 3, 2026
ea33eeb
ext/session: add rfc1867_freq test for garbage input in percentage mode
jorgsowa Jul 3, 2026
3f78a89
ext/session: normalize whitespace handling in session.upload_progress…
jorgsowa Jul 4, 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
38 changes: 36 additions & 2 deletions ext/session/session.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -865,16 +865,44 @@ static PHP_INI_MH(OnUpdateSessionDivisor)
return SUCCESS;
}

static bool php_session_is_ini_whitespace(char c)
{
return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\v' || c == '\f';
}

static PHP_INI_MH(OnUpdateRfc1867Freq)
{
int new_freq = ZEND_ATOL(ZSTR_VAL(new_value));
const char *str = ZSTR_VAL(new_value);
size_t len = ZSTR_LEN(new_value);

/* Ignore trailing whitespace so it doesn't hide the '%' suffix (leading
* whitespace is already tolerated by is_numeric_string_ex() below). */
while (len > 0 && php_session_is_ini_whitespace(str[len - 1])) {
len--;
}

bool is_percentage = len > 0 && str[len - 1] == '%';

@GirgiasGirgiasJul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens with trailing spaces? Not sure how our INI parser deals with those.

@jorgsowajorgsowaJul 4, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's good input. Is there any ini helper for such values? I added a comparison table to the PR description and test cases to the branch for my suggested solution for trailing whitespaces.

size_t numeric_len = is_percentage ? len - 1 : len;

/* Whitespace directly before the '%' (e.g. "5 %") is rejected. */
if (is_percentage && numeric_len > 0 && php_session_is_ini_whitespace(str[numeric_len - 1])) {
php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int");
return FAILURE;
}

zend_long new_freq = 0;
uint8_t type = is_numeric_string_ex(str, numeric_len, &new_freq, NULL, false, NULL, NULL);
if (type != IS_LONG) {
php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int");
return FAILURE;
}
Comment on lines +888 to +898

@GirgiasGirgiasAug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These error messages are wrong, as the INI is not an int, it's an int followed by a % sign.


if (new_freq < 0) {
php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be greater than or equal to 0");
return FAILURE;
}

if (ZSTR_LEN(new_value) > 0 && ZSTR_VAL(new_value)[ZSTR_LEN(new_value) - 1] == '%') {
if (is_percentage) {
if (new_freq > 100) {
php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be less than or equal to 100%%");
return FAILURE;
Expand All@@ -884,6 +912,12 @@ static PHP_INI_MH(OnUpdateRfc1867Freq)
PS(rfc1867_freq) = new_freq;
}

/* Store a whitespace-free canonical value so ini_get() doesn't echo back stray whitespace. */
char buf[MAX_LENGTH_OF_LONG + 2];
int normalized_len = snprintf(buf, sizeof(buf), is_percentage ? ZEND_LONG_FMT "%%" : ZEND_LONG_FMT, new_freq);
entry->value = zend_string_init(buf, (size_t) normalized_len, true);
GC_MAKE_PERSISTENT_LOCAL(entry->value);

return SUCCESS;
}

Expand Down
17 changes: 17 additions & 0 deletions ext/session/tests/rfc1867_freq_leading_whitespace.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
--TEST--
session rfc1867 freq: leading whitespace is tolerated and stripped from the stored value
--INI--
session.upload_progress.freq=" 5%"
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECT--
string(2) "5%"
17 changes: 17 additions & 0 deletions ext/session/tests/rfc1867_freq_trailing_percent_space.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
--TEST--
session rfc1867 freq: trailing whitespace after the "%" suffix is tolerated
--INI--
session.upload_progress.freq="5% "
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECT--
string(2) "5%"
18 changes: 18 additions & 0 deletions ext/session/tests/rfc1867_invalid_settings_3.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
--TEST--
session rfc1867 invalid settings 3: non-numeric freq is rejected instead of silently truncated
--INI--
session.upload_progress.freq=5 apples
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECTF--
Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
string(%d) "1%"
18 changes: 18 additions & 0 deletions ext/session/tests/rfc1867_invalid_settings_4.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
--TEST--
session rfc1867 invalid settings 4: non-numeric freq in percentage mode is rejected instead of silently truncated
--INI--
session.upload_progress.freq=5 apples%
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECTF--
Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
string(%d) "1%"
18 changes: 18 additions & 0 deletions ext/session/tests/rfc1867_invalid_settings_5.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
--TEST--
session rfc1867 invalid settings 5: whitespace-only freq is rejected
--INI--
session.upload_progress.freq=" "
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECTF--
Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
string(%d) "1%"
18 changes: 18 additions & 0 deletions ext/session/tests/rfc1867_invalid_settings_6.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
--TEST--
session rfc1867 invalid settings 6: whitespace between the number and the "%" suffix is rejected
--INI--
session.upload_progress.freq="5 %"
error_log=
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
?>
--FILE--
<?php
var_dump(ini_get("session.upload_progress.freq"));
?>
--EXPECTF--
Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
string(%d) "1%"
Loading