From 3059adae06d8c96b633a8d8de8d2cd7b70b5e3ae Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Tue, 20 Aug 2024 15:24:25 +0100 Subject: [PATCH] ext/standard: Improve checking of allowed_classes option (#15267) * ext/standard: Add some unserializing tests * ext/standard: Add proper type checking for values of the allowed_classes option array * ext/standard: Check that class names are somewhat sensible for the allowed_classes option array * Indicate type of value * Add test for Stringable objects --- ..._allowed_classes_option_invalid_array.phpt | 61 +++++++++++++++++++ ...ed_classes_option_invalid_class_names.phpt | 48 +++++++++++++++ ...allowed_classes_option_invalid_value.phpt} | 0 ...lowed_classes_option_stringable_value.phpt | 32 ++++++++++ ext/standard/var.c | 31 ++++++---- 5 files changed, 161 insertions(+), 11 deletions(-) create mode 100644 ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt create mode 100644 ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.phpt rename ext/standard/tests/serialize/{unserialize_error_001.phpt => unserialize_allowed_classes_option_invalid_value.phpt} (100%) create mode 100644 ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt diff --git a/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt new file mode 100644 index 00000000000..a13334baaca --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt @@ -0,0 +1,61 @@ +--TEST-- +Test unserialize() with array allowed_classes and nonsensical values +--FILE-- + [null]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [false]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [true]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [42]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [15.2]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [[]]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [STDERR]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [new stdClass]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, null given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, false given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, true given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, int given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, float given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, array given +TypeError: unserialize(): Option "allowed_classes" must be an array of class names, resource given +Error: Object of class stdClass could not be converted to string diff --git a/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.phpt b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.phpt new file mode 100644 index 00000000000..74997939817 --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.phpt @@ -0,0 +1,48 @@ +--TEST-- +Test unserialize() with array allowed_classes and nonsensical class names +--FILE-- + [""]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => ["245blerg"]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => [" whitespace "]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => ["name\nwith whitespace"]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => ['$dollars']]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} +try { + unserialize($s, ["allowed_classes" => ["have\0nul_byte"]]); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +ValueError: unserialize(): Option "allowed_classes" must be an array of class names, " whitespace " given +ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "name +with whitespace" given +ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "$dollars" given +ValueError: unserialize(): Option "allowed_classes" must be an array of class names, "have" given diff --git a/ext/standard/tests/serialize/unserialize_error_001.phpt b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_value.phpt similarity index 100% rename from ext/standard/tests/serialize/unserialize_error_001.phpt rename to ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_value.phpt diff --git a/ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt b/ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt new file mode 100644 index 00000000000..5868cf9e923 --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt @@ -0,0 +1,32 @@ +--TEST-- +Test unserialize() with Stringable object in allowed_classes +--FILE-- + [$o]])); +?> +--EXPECT-- +array(3) { + [0]=> + object(foo)#3 (1) { + ["x"]=> + string(3) "bar" + } + [1]=> + int(2) + [2]=> + string(1) "3" +} diff --git a/ext/standard/var.c b/ext/standard/var.c index dd2bd86d91e..7e51a23e3c0 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1370,25 +1370,34 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co goto cleanup; } - if(classes && (Z_TYPE_P(classes) == IS_ARRAY || !zend_is_true(classes))) { + if (classes && (Z_TYPE_P(classes) == IS_ARRAY || !zend_is_true(classes))) { ALLOC_HASHTABLE(class_hash); zend_hash_init(class_hash, (Z_TYPE_P(classes) == IS_ARRAY)?zend_hash_num_elements(Z_ARRVAL_P(classes)):0, NULL, NULL, 0); } - if(class_hash && Z_TYPE_P(classes) == IS_ARRAY) { + if (class_hash && Z_TYPE_P(classes) == IS_ARRAY) { zval *entry; - zend_string *lcname; ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(classes), entry) { - convert_to_string(entry); - lcname = zend_string_tolower(Z_STR_P(entry)); + ZVAL_DEREF(entry); + if (UNEXPECTED(Z_TYPE_P(entry) != IS_STRING && Z_TYPE_P(entry) != IS_OBJECT)) { + zend_type_error("%s(): Option \"allowed_classes\" must be an array of class names, %s given", + function_name, zend_zval_value_name(entry)); + goto cleanup; + } + zend_string *name = zval_try_get_string(entry); + if (UNEXPECTED(name == NULL)) { + goto cleanup; + } + if (UNEXPECTED(!zend_is_valid_class_name(name))) { + zend_value_error("%s(): Option \"allowed_classes\" must be an array of class names, \"%s\" given", function_name, ZSTR_VAL(name)); + zend_string_release_ex(name, false); + goto cleanup; + } + zend_string *lcname = zend_string_tolower(name); zend_hash_add_empty_element(class_hash, lcname); - zend_string_release_ex(lcname, 0); + zend_string_release_ex(name, false); + zend_string_release_ex(lcname, false); } ZEND_HASH_FOREACH_END(); - - /* Exception during string conversion. */ - if (EG(exception)) { - goto cleanup; - } } php_var_unserialize_set_allowed_classes(var_hash, class_hash);