diff --git a/NEWS b/NEWS index 9df461452c1c..5f1d3b951738 100644 --- a/NEWS +++ b/NEWS @@ -32,6 +32,8 @@ PHP NEWS surrounding property access). (timwolla) . Fixed GH-22422 (zend_arena layout mismatch leaked memory in separately built extensions under AddressSanitizer). (iliaal) + . Added validation for invalid upload_max_filesize and post_max_size values + (GH-14695). (Weilin Du) . TSRM: use local-exec TLS in PIE executables. (henderkes) . perf: make all static extensions use TSRMG_STATIC. (henderkes) . Fixed bug GH-22257 (type confusion in Exception::getTraceAsString()). diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index a467df3b6be5..ef7b2e90e895 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -70,6 +70,10 @@ PHP 8.6 INTERNALS UPGRADE NOTES more correctly represents the generic nature of the returned pointer and allows to remove explicit casts, but possibly breaks pointer arithmetic performed on the result. + . Added zend_ini_parse_quantity_strict(), a strict variant of + zend_ini_parse_quantity() that returns FAILURE for ill-formatted quantity + values instead of a backwards-compatible interpretation. Added + OnUpdateLongStrict() for INI entries that should reject such values. . The zend_dval_to_lval_cap() function no longer takes a second zend_string* parameter. . EG(in_autoload) was renamed to EG(autoload_current_classnames) and no diff --git a/Zend/tests/zend_ini/gh14695.phpt b/Zend/tests/zend_ini/gh14695.phpt new file mode 100644 index 000000000000..2b72010ba634 --- /dev/null +++ b/Zend/tests/zend_ini/gh14695.phpt @@ -0,0 +1,18 @@ +--TEST-- +GH-14695: Invalid upload_max_filesize and post_max_size values are rejected +--INI-- +upload_max_filesize=1zz +post_max_size= +--FILE-- + +--EXPECTF-- +Warning: Invalid "upload_max_filesize" setting. Invalid quantity "1zz": unknown multiplier "z" in %s on line %d + +Warning: Invalid "post_max_size" setting. Invalid quantity "": no valid leading digits in %s on line %d +string(2) "2M" +string(2) "8M" diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c index 093683526d31..be4dbd592fb0 100644 --- a/Zend/zend_ini.c +++ b/Zend/zend_ini.c @@ -614,7 +614,12 @@ static const char *zend_ini_consume_quantity_prefix(const char *const digits, co return digits_consumed; } -static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zend_ini_parse_quantity_signed_result_t signed_result, zend_string **errstr) /* {{{ */ +static zend_ulong zend_ini_parse_quantity_internal( + const zend_string *value, + zend_ini_parse_quantity_signed_result_t signed_result, + zend_string **errstr, + bool strict +) /* {{{ */ { char *digits_end = NULL; const char *str = ZSTR_VAL(value); @@ -634,6 +639,18 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen while (digits < str_end && zend_is_whitespace(*(str_end-1))) {--str_end;} if (digits == str_end) { + if (strict) { + if (ZSTR_LEN(value) == 0) { + *errstr = zend_strpprintf(0, "Invalid quantity \"\": no valid leading digits"); + } else { + smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); + smart_str_0(&invalid); + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits", + ZSTR_VAL(invalid.s)); + smart_str_free(&invalid); + } + return 0; + } *errstr = NULL; return 0; } @@ -653,8 +670,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); smart_str_0(&invalid); - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits, interpreting as \"0\" for backwards compatibility", - ZSTR_VAL(invalid.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits", + ZSTR_VAL(invalid.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits, interpreting as \"0\" for backwards compatibility", + ZSTR_VAL(invalid.s)); + } smart_str_free(&invalid); return 0; @@ -690,8 +712,12 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen base = 2; break; default: - *errstr = zend_strpprintf(0, "Invalid prefix \"0%c\", interpreting as \"0\" for backwards compatibility", - digits[1]); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid prefix \"0%c\"", digits[1]); + } else { + *errstr = zend_strpprintf(0, "Invalid prefix \"0%c\", interpreting as \"0\" for backwards compatibility", + digits[1]); + } return 0; } digits += 2; @@ -702,8 +728,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); smart_str_0(&invalid); - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no digits after base prefix, interpreting as \"0\" for backwards compatibility", - ZSTR_VAL(invalid.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no digits after base prefix", + ZSTR_VAL(invalid.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no digits after base prefix, interpreting as \"0\" for backwards compatibility", + ZSTR_VAL(invalid.s)); + } smart_str_free(&invalid); return 0; @@ -744,8 +775,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); smart_str_0(&invalid); - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits, interpreting as \"0\" for backwards compatibility", - ZSTR_VAL(invalid.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits", + ZSTR_VAL(invalid.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no valid leading digits, interpreting as \"0\" for backwards compatibility", + ZSTR_VAL(invalid.s)); + } smart_str_free(&invalid); return 0; @@ -781,8 +817,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen smart_str_append_escaped(&chr, str_end-1, 1); smart_str_0(&chr); - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": unknown multiplier \"%s\", interpreting as \"%s\" for backwards compatibility", - ZSTR_VAL(invalid.s), ZSTR_VAL(chr.s), ZSTR_VAL(interpreted.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": unknown multiplier \"%s\"", + ZSTR_VAL(invalid.s), ZSTR_VAL(chr.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": unknown multiplier \"%s\", interpreting as \"%s\" for backwards compatibility", + ZSTR_VAL(invalid.s), ZSTR_VAL(chr.s), ZSTR_VAL(interpreted.s)); + } smart_str_free(&invalid); smart_str_free(&interpreted); @@ -815,8 +856,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen smart_str_append_escaped(&chr, str_end-1, 1); smart_str_0(&chr); - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\", interpreting as \"%s%s\" for backwards compatibility", - ZSTR_VAL(invalid.s), ZSTR_VAL(interpreted.s), ZSTR_VAL(chr.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": invalid characters before multiplier \"%s\"", + ZSTR_VAL(invalid.s), ZSTR_VAL(chr.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\", interpreting as \"%s%s\" for backwards compatibility", + ZSTR_VAL(invalid.s), ZSTR_VAL(interpreted.s), ZSTR_VAL(chr.s)); + } smart_str_free(&invalid); smart_str_free(&interpreted); @@ -833,8 +879,13 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen /* Not specifying the resulting value here because the caller may make * additional conversions. Not specifying the allowed range * because the caller may do narrower range checks. */ - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": value is out of range, using overflow result for backwards compatibility", - ZSTR_VAL(invalid.s)); + if (strict) { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": value is out of range", + ZSTR_VAL(invalid.s)); + } else { + *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": value is out of range, using overflow result for backwards compatibility", + ZSTR_VAL(invalid.s)); + } smart_str_free(&invalid); smart_str_free(&interpreted); @@ -850,16 +901,27 @@ static zend_ulong zend_ini_parse_quantity_internal(const zend_string *value, zen ZEND_API zend_long zend_ini_parse_quantity(const zend_string *value, zend_string **errstr) /* {{{ */ { - return (zend_long) zend_ini_parse_quantity_internal(value, ZEND_INI_PARSE_QUANTITY_SIGNED, errstr); + return (zend_long) zend_ini_parse_quantity_internal(value, ZEND_INI_PARSE_QUANTITY_SIGNED, errstr, false); } /* }}} */ ZEND_API zend_ulong zend_ini_parse_uquantity(const zend_string *value, zend_string **errstr) /* {{{ */ { - return zend_ini_parse_quantity_internal(value, ZEND_INI_PARSE_QUANTITY_UNSIGNED, errstr); + return zend_ini_parse_quantity_internal(value, ZEND_INI_PARSE_QUANTITY_UNSIGNED, errstr, false); } /* }}} */ +ZEND_API zend_result zend_ini_parse_quantity_strict(const zend_string *value, zend_long *result, zend_string **errstr) +{ + zend_long retval = (zend_long) zend_ini_parse_quantity_internal(value, ZEND_INI_PARSE_QUANTITY_SIGNED, errstr, true); + if (*errstr) { + return FAILURE; + } + + *result = retval; + return SUCCESS; +} + ZEND_API zend_long zend_ini_parse_quantity_warn(const zend_string *value, zend_string *setting) /* {{{ */ { zend_string *errstr; @@ -974,6 +1036,22 @@ ZEND_API ZEND_INI_MH(OnUpdateLong) /* {{{ */ } /* }}} */ +ZEND_API ZEND_INI_MH(OnUpdateLongStrict) +{ + zend_long tmp; + zend_string *errstr; + if (UNEXPECTED(zend_ini_parse_quantity_strict(new_value, &tmp, &errstr) == FAILURE)) { + zend_error(E_WARNING, "Invalid \"%s\" setting. %s", ZSTR_VAL(entry->name), ZSTR_VAL(errstr)); + zend_string_release(errstr); + return FAILURE; + } + + zend_long *p = ZEND_INI_GET_ADDR(); + *p = tmp; + + return SUCCESS; +} + ZEND_API ZEND_INI_MH(OnUpdateLongGEZero) /* {{{ */ { zend_long tmp = zend_ini_parse_quantity_warn(new_value, entry->name); diff --git a/Zend/zend_ini.h b/Zend/zend_ini.h index dbe650675b66..a181048ad9ab 100644 --- a/Zend/zend_ini.h +++ b/Zend/zend_ini.h @@ -142,6 +142,12 @@ ZEND_API zend_long zend_ini_parse_quantity(const zend_string *value, zend_string */ ZEND_API zend_ulong zend_ini_parse_uquantity(const zend_string *value, zend_string **errstr); +/** + * Strict variant of zend_ini_parse_quantity. Ill-formatted values fail instead + * of returning a backwards-compatible interpretation. + */ +ZEND_API zend_result zend_ini_parse_quantity_strict(const zend_string *value, zend_long *result, zend_string **errstr); + ZEND_API zend_long zend_ini_parse_quantity_warn(const zend_string *value, zend_string *setting); ZEND_API zend_ulong zend_ini_parse_uquantity_warn(const zend_string *value, zend_string *setting); @@ -207,6 +213,7 @@ END_EXTERN_C() BEGIN_EXTERN_C() ZEND_API ZEND_INI_MH(OnUpdateBool); ZEND_API ZEND_INI_MH(OnUpdateLong); +ZEND_API ZEND_INI_MH(OnUpdateLongStrict); ZEND_API ZEND_INI_MH(OnUpdateLongGEZero); ZEND_API ZEND_INI_MH(OnUpdateReal); /* char* versions */ diff --git a/ext/standard/http.c b/ext/standard/http.c index d65e7a8acaae..667eddaf0be0 100644 --- a/ext/standard/http.c +++ b/ext/standard/http.c @@ -247,17 +247,17 @@ PHP_FUNCTION(http_build_query) } /* }}} */ -static zend_result cache_request_parse_body_option(HashTable *options, zval *option, int cache_offset) +static zend_result cache_request_parse_body_option(zval *option, const char *option_name, int cache_offset) { if (option) { zend_long result; ZVAL_DEREF(option); if (Z_TYPE_P(option) == IS_STRING) { zend_string *errstr; - result = zend_ini_parse_quantity(Z_STR_P(option), &errstr); - if (errstr) { - zend_error(E_WARNING, "%s", ZSTR_VAL(errstr)); + if (UNEXPECTED(zend_ini_parse_quantity_strict(Z_STR_P(option), &result, &errstr) == FAILURE)) { + zend_value_error("Invalid \"%s\" value in $options argument: %s", option_name, ZSTR_VAL(errstr)); zend_string_release(errstr); + return FAILURE; } } else if (Z_TYPE_P(option) == IS_LONG) { result = Z_LVAL_P(option); @@ -290,7 +290,7 @@ static zend_result cache_request_parse_body_options(HashTable *options) #define CHECK_OPTION(name) \ if (zend_string_equals_literal_ci(key, #name)) { \ - if (cache_request_parse_body_option(options, value, REQUEST_PARSE_BODY_OPTION_ ## name) == FAILURE) { \ + if (cache_request_parse_body_option(value, #name, REQUEST_PARSE_BODY_OPTION_ ## name) == FAILURE) { \ return FAILURE; \ } \ continue; \ diff --git a/ext/standard/tests/http/request_parse_body/multipart_options_invalid_quantity.phpt b/ext/standard/tests/http/request_parse_body/multipart_options_invalid_quantity.phpt index b1efa0dbc91a..b5b43f9355db 100644 --- a/ext/standard/tests/http/request_parse_body/multipart_options_invalid_quantity.phpt +++ b/ext/standard/tests/http/request_parse_body/multipart_options_invalid_quantity.phpt @@ -3,15 +3,24 @@ request_parse_body() invalid quantity --FILE-- '1GB', - ]); -} catch (Throwable $e) { - echo get_class($e), ': ', $e->getMessage(), "\n"; +foreach ([ + ['upload_max_filesize', '1GB'], + ['upload_max_filesize', ''], + ['post_max_size', '1GB'], + ['post_max_size', ''], +] as [$option, $value]) { + try { + request_parse_body(options: [ + $option => $value, + ]); + } catch (Throwable $e) { + echo get_class($e), ': ', $e->getMessage(), "\n"; + } } ?> --EXPECTF-- -Warning: Invalid quantity "1GB": unknown multiplier "B", interpreting as "1" for backwards compatibility in %s on line %d -RequestParseBodyException: Request does not provide a content type +ValueError: Invalid "upload_max_filesize" value in $options argument: Invalid quantity "1GB": unknown multiplier "B" +ValueError: Invalid "upload_max_filesize" value in $options argument: Invalid quantity "": no valid leading digits +ValueError: Invalid "post_max_size" value in $options argument: Invalid quantity "1GB": unknown multiplier "B" +ValueError: Invalid "post_max_size" value in $options argument: Invalid quantity "": no valid leading digits diff --git a/main/main.c b/main/main.c index 6bda55ac8746..15bed5be9880 100644 --- a/main/main.c +++ b/main/main.c @@ -845,8 +845,8 @@ PHP_INI_BEGIN() STD_PHP_INI_ENTRY("open_basedir", NULL, PHP_INI_ALL, OnUpdateBaseDir, open_basedir, php_core_globals, core_globals) STD_PHP_INI_BOOLEAN("file_uploads", "1", PHP_INI_SYSTEM, OnUpdateBool, file_uploads, php_core_globals, core_globals) - STD_PHP_INI_ENTRY("upload_max_filesize", "2M", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLong, upload_max_filesize, php_core_globals, core_globals) - STD_PHP_INI_ENTRY("post_max_size", "8M", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLong, post_max_size, sapi_globals_struct,sapi_globals) + STD_PHP_INI_ENTRY("upload_max_filesize", "2M", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLongStrict, upload_max_filesize, php_core_globals, core_globals) + STD_PHP_INI_ENTRY("post_max_size", "8M", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLongStrict, post_max_size, sapi_globals_struct,sapi_globals) STD_PHP_INI_ENTRY("upload_tmp_dir", NULL, PHP_INI_SYSTEM, OnUpdateStringUnempty, upload_tmp_dir, php_core_globals, core_globals) STD_PHP_INI_ENTRY("max_input_nesting_level", "64", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLongGEZero, max_input_nesting_level, php_core_globals, core_globals) STD_PHP_INI_ENTRY("max_input_vars", "1000", PHP_INI_SYSTEM|PHP_INI_PERDIR, OnUpdateLongGEZero, max_input_vars, php_core_globals, core_globals)