Skip to content

Fix GH-11876: ini_parse_quantity() accepts invalid quantities #11910

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

Closed
wants to merge 1 commit into from
Closed

Fix GH-11876: ini_parse_quantity() accepts invalid quantities #11910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 51 additions & 0 deletions Zend/tests/zend_ini/gh11876.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
--TEST--
Invalid INI quantities, base prefix followed by stuff eaten by strtoull()
--EXTENSIONS--
zend_test
--FILE--
<?php

var_dump(zend_test_zend_ini_parse_quantity('0x0x12'));

var_dump(zend_test_zend_ini_parse_quantity('0b+10'));
var_dump(zend_test_zend_ini_parse_quantity('0o+10'));
var_dump(zend_test_zend_ini_parse_quantity('0x+10'));

var_dump(zend_test_zend_ini_parse_quantity('0b 10'));
var_dump(zend_test_zend_ini_parse_quantity('0o 10'));
var_dump(zend_test_zend_ini_parse_quantity('0x 10'));

var_dump(zend_test_zend_ini_parse_quantity('0g10'));
var_dump(zend_test_zend_ini_parse_quantity('0m10'));
var_dump(zend_test_zend_ini_parse_quantity('0k10'));

--EXPECTF--
Warning: Invalid quantity "0x0x12": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0b+10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0o+10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0x+10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0b 10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0o 10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0x 10": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0g10": unknown multiplier "0", interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0m10": unknown multiplier "0", interpreting as "0" for backwards compatibility in %s on line %d
int(0)

Warning: Invalid quantity "0k10": unknown multiplier "0", interpreting as "0" for backwards compatibility in %s on line %d
int(0)
40 changes: 40 additions & 0 deletions Zend/zend_ini.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,34 @@ typedef enum {
ZEND_INI_PARSE_QUANTITY_UNSIGNED,
} zend_ini_parse_quantity_signed_result_t;

static const char *zend_ini_consume_quantity_prefix(const char *const digits, const char *const str_end) {
const char *digits_consumed = digits;
/* Ignore leading whitespace. */
while (digits_consumed < str_end && zend_is_whitespace(*digits_consumed)) {++digits_consumed;}
if (digits_consumed[0] == '+' || digits_consumed[0] == '-') {
++digits_consumed;
}

if (digits_consumed[0] == '0' && !isdigit(digits_consumed[1])) {
/* Value is just 0 */
if ((digits_consumed+1) == str_end) {
return digits;
}

switch (digits_consumed[1]) {
case 'x':
case 'X':
case 'o':
case 'O':
case 'b':
case 'B':
digits_consumed += 2;
break;
}
}
return digits_consumed;
}

static zend_ulong zend_ini_parse_quantity_internal(zend_string *value, zend_ini_parse_quantity_signed_result_t signed_result, zend_string **errstr) /* {{{ */
{
char *digits_end = NULL;
Expand Down Expand Up @@ -634,6 +662,18 @@ static zend_ulong zend_ini_parse_quantity_internal(zend_string *value, zend_ini_
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));

smart_str_free(&invalid);
return 0;
}
if (UNEXPECTED(digits != zend_ini_consume_quantity_prefix(digits, str_end))) {
Copy link
Member

@devnexen devnexen Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

seems to me could be interpreted/simplified slightly as :

if (UNEXPECTED(digits == str_end) || UNEXPECTED(digits != zend_ini_consume_quantity_prefix(digits, str_end)) { goto invalid_quantity; }

eventually even having a const char *errmsg so line 614 can be addressed too.

unless I misread. What do you think ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The condition change makes sense, not sure I understand what you mean about line 614?

Copy link
Member

Choose a reason for hiding this comment

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

Line like 614 is just displaying the wrong case in the same fashion, only the error message changes compared to 665.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right 👍

/* Escape the string to avoid null bytes and to make non-printable chars
* visible */
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));

Expand Down