Skip to content

Make the behavior of request_parse_body() and ini settings congruent in option max_input_vars#22223

Open
OracleNep wants to merge 1 commit into
php:masterfrom
OracleNep:reject-negative-request-parse-body-options
Open

Make the behavior of request_parse_body() and ini settings congruent in option max_input_vars#22223
OracleNep wants to merge 1 commit into
php:masterfrom
OracleNep:reject-negative-request-parse-body-options

Conversation

@OracleNep
Copy link
Copy Markdown

@OracleNep OracleNep commented Jun 4, 2026 •
edited
Loading

request_parse_body() currently accepts out-of-range values for limit-related options.

This can weaken or bypass request body limits. In particular, for application/x-www-form-urlencoded bodies, negative max_input_vars values are read into an unsigned uint64_t, so passing max_input_vars => -1 effectively turns the limit into a very large value.

post_max_size => -1 and post_max_size => 0 also disable the size check, because the check only runs when the configured value is greater than zero. upload_max_filesize follows the same positive-size-limit pattern.

This patch rejects invalid option ranges for:

max_file_uploads       must be >= 0
max_input_vars         must be >= 0
post_max_size          must be > 0
upload_max_filesize    must be > 0

max_multipart_body_parts is left unchanged because negative values currently have a special meaning there: derive the multipart body part limit from max_input_vars + max_file_uploads.

This is a hardening / validation fix, not a security report. The options are supplied by application code, but accepting invalid ranges makes it easy to accidentally disable request body limits.

Local verification

Using php-src master be41c36b68f, tested with the built-in web server:

request_parse_body(['max_input_vars' => -1])

accepted all urlencoded fields, while:

request_parse_body(['max_input_vars' => 1])

triggered the expected max-input-vars warning path.

Also verified:

request_parse_body(['post_max_size' => -1])

accepted a body that was rejected when post_max_size => 1.

Tests

sapi/cli/php run-tests.php ext/standard/tests/http/request_parse_body/options_negative_values.phpt
sapi/cli/php run-tests.php ext/standard/tests/http/request_parse_body

@LamentXU123 LamentXU123 self-assigned this Jun 4, 2026
@OracleNep OracleNep marked this pull request as ready for review June 4, 2026 05:59
@OracleNep OracleNep requested a review from bukka as a code owner June 4, 2026 05:59
@LamentXU123
Copy link
Copy Markdown
Contributor

LamentXU123 commented Jun 4, 2026 •
edited
Loading

Interesting, but

  • How is it going to work on 0? Untested, but after reading the code I think post_max_size and max_input_vars checks with something like if (post_max_size > 0 && ...) so only rejecting negative values is not enough, if you want to harden in this case you might need to reject all non-positive numbers, or 0 can still cause infinity in at least those two args. You may also need to check 0's behavior in other args you've changed.
  • This works on zend_long, right? So just in case I'd suggest to test on some numeric-strings. That is values like '-1M', '-1.0' or so.
  • Is there intentional pattern of request_parse_body(['max_input_vars' => -1])? As you've said it is unsigned int so parsing -1 will make it to a huge number, that's correct. But just in case someone want to use -1 instead of PHP_INT_MAX I think we can search the code base to see if the downstream is already writing this intentionally.
  • As a BC break, you may need to write NEWS and UPGRADING files.

@OracleNep OracleNep marked this pull request as draft June 4, 2026 06:28
@OracleNep OracleNep force-pushed the reject-negative-request-parse-body-options branch from bcad8b6 to bac9666 Compare June 4, 2026 06:33
@OracleNep OracleNep changed the title Reject negative request_parse_body() limit options Validate request_parse_body() limit option ranges Jun 4, 2026
@OracleNep
Copy link
Copy Markdown
Author

OracleNep commented Jun 4, 2026

Thanks for the review. Updated the PR accordingly:

  • Checked the 0 behavior and adjusted the range validation instead of rejecting every zero value:
    • max_file_uploads = 0 remains valid and means no uploads are allowed.
    • max_input_vars = 0 remains valid and means no input variables are allowed; it does not become unlimited in either the urlencoded or multipart paths.
    • post_max_size = 0 and upload_max_filesize = 0 are now rejected because those size checks only run for values greater than zero.
  • Added numeric-string coverage for negative quantities, including -1M and the warning-producing -1.0 path.
  • Searched php-src and public GitHub code search for obvious request_parse_body() option patterns using -1; I did not find hits for max_input_vars, post_max_size, upload_max_filesize, or max_file_uploads.
  • Added NEWS and UPGRADING entries for the BC impact.

I moved the PR back to draft while the updated CI is running.

@LamentXU123
Copy link
Copy Markdown
Contributor

LamentXU123 commented Jun 4, 2026

Thank you. This looks fine to me and I will have a closer look hours later before I request review from other people :)

@OracleNep OracleNep marked this pull request as ready for review June 4, 2026 07:21
LamentXU123
LamentXU123 approved these changes Jun 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 left a comment

Choose a reason for hiding this comment

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

Overall correct to me. Let's hear from @iluuu1994 for further opinions :)

Comment thread ext/standard/http.c
Comment thread UPGRADING Outdated
Comment thread NEWS Outdated
Comment thread ext/standard/http.c
@iluuu1994
Copy link
Copy Markdown
Member

iluuu1994 commented Jun 4, 2026

Hi. Thanks for the PR. Two notes:

  • Negative values in ini settings commonly signal "no limit".
  • Does it make sense to have these warnings be present only for request_parse_body() but not for the regular ini settings? I'd argue no. The behavior of both should be congruent.

@LamentXU123
Copy link
Copy Markdown
Contributor

LamentXU123 commented Jun 4, 2026 •
edited
Loading

Thanks for introducing this concern. After a closer look of the ini behavior, I'd suggest that:

  • For max_input_vars: The ini handler is OnUpdateLongGEZero, it raises warnings when dealing with negative values while in request_parse_body() it silently accepts. That is, this PR is in the correct direction to this extent by fixing the behavior of request_parse_body() to align with the ini behavior. For example, parsing -1, in ini settings it is parsed as 1000 with E_WARNING, in request_parse_body() it is silently parsed as PHP_INT_MAX due to an overflow.
  • For the rest options, I'd suggest to revert the changes, this creates split between ini settings and BC breaks we don't want.

@OracleNep OracleNep force-pushed the reject-negative-request-parse-body-options branch 2 times, most recently from 44be3c5 to 861aaf7 Compare June 4, 2026 14:24
@LamentXU123
Copy link
Copy Markdown
Contributor

LamentXU123 commented Jun 4, 2026

Now I believe this PR makes the behavior of request_parse_body and ini congruent.

@LamentXU123 LamentXU123 requested a review from iluuu1994 June 4, 2026 14:35
@LamentXU123 LamentXU123 changed the title Validate request_parse_body() limit option ranges Make the behavior of request_parse_body() and ini settings congruent in option max_input_vars Jun 4, 2026
@OracleNep OracleNep force-pushed the reject-negative-request-parse-body-options branch 2 times, most recently from d19f647 to 0394354 Compare June 4, 2026 14:58
@OracleNep OracleNep force-pushed the reject-negative-request-parse-body-options branch from 0394354 to 0743a1e Compare June 4, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LamentXU123 LamentXU123 LamentXU123 approved these changes

@bukka bukka Awaiting requested review from bukka bukka is a code owner

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

@LamentXU123 LamentXU123

Labels

Extension: standard Feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@OracleNep @LamentXU123 @iluuu1994