From 9ce3e98cd3fd38298bc17761095b7ee6f9057062 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 7 Aug 2024 01:23:48 +0100 Subject: [PATCH 1/5] ext/standard: Add some unserializing tests --- ..._allowed_classes_option_invalid_array.phpt | 55 +++++++++++++++++++ ...allowed_classes_option_invalid_value.phpt} | 0 2 files changed, 55 insertions(+) create mode 100644 ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt rename ext/standard/tests/serialize/{unserialize_error_001.phpt => unserialize_allowed_classes_option_invalid_value.phpt} (100%) 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 0000000000000..b7873263533fb --- /dev/null +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt @@ -0,0 +1,55 @@ +--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"; +} + +?> +--EXPECTF-- +Warning: Array to string conversion in %s on line %d +Error: Object of class stdClass could not be converted to string 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 From c1cff7e63276edf9622b7f68e3f5e04d31082675 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 7 Aug 2024 01:31:49 +0100 Subject: [PATCH 2/5] ext/standard: Add proper type checking for values of the allowed_classes option array --- ..._allowed_classes_option_invalid_array.phpt | 10 ++++++-- ext/standard/var.c | 25 +++++++++++-------- 2 files changed, 22 insertions(+), 13 deletions(-) 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 index b7873263533fb..7a0fe52e91202 100644 --- a/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt @@ -50,6 +50,12 @@ try { } ?> ---EXPECTF-- -Warning: Array to string conversion in %s on line %d +--EXPECT-- +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of strings Error: Object of class stdClass could not be converted to string diff --git a/ext/standard/var.c b/ext/standard/var.c index dd2bd86d91ee8..bcdc7f46731e5 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1370,25 +1370,28 @@ 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 strings", function_name); + goto cleanup; + } + zend_string *name = zval_try_get_string(entry); + if (UNEXPECTED(name == NULL)) { + 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); From d5ec5b2e8558f770f0eaa197770ef4430c0a2013 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 7 Aug 2024 01:41:19 +0100 Subject: [PATCH 3/5] ext/standard: Check that class names are somewhat sensible for the allowed_classes option array --- ..._allowed_classes_option_invalid_array.phpt | 14 +++--- ...ed_classes_option_invalid_class_names.phpt | 48 +++++++++++++++++++ ext/standard/var.c | 7 ++- 3 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_class_names.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 index 7a0fe52e91202..d703dca80749d 100644 --- a/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt @@ -51,11 +51,11 @@ try { ?> --EXPECT-- -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings -TypeError: unserialize(): Option "allowed_classes" must be an array of strings +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names +TypeError: unserialize(): Option "allowed_classes" must be an array of class names 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 0000000000000..74997939817d8 --- /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/var.c b/ext/standard/var.c index bcdc7f46731e5..05228fcbe285b 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1380,13 +1380,18 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(classes), 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 strings", function_name); + zend_type_error("%s(): Option \"allowed_classes\" must be an array of class names", function_name); 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(name, false); From 33a4464360dbfc00e5e5ca3b1f62397c74d994e8 Mon Sep 17 00:00:00 2001 From: Gina Peter Bnayard Date: Thu, 8 Aug 2024 15:54:37 +0200 Subject: [PATCH 4/5] Indicate type of value --- ...alize_allowed_classes_option_invalid_array.phpt | 14 +++++++------- ext/standard/var.c | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) 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 index d703dca80749d..a13334baacaa1 100644 --- a/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt +++ b/ext/standard/tests/serialize/unserialize_allowed_classes_option_invalid_array.phpt @@ -51,11 +51,11 @@ try { ?> --EXPECT-- -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names -TypeError: unserialize(): Option "allowed_classes" must be an array of class names +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/var.c b/ext/standard/var.c index 05228fcbe285b..7e51a23e3c029 100644 --- a/ext/standard/var.c +++ b/ext/standard/var.c @@ -1380,7 +1380,8 @@ PHPAPI void php_unserialize_with_options(zval *return_value, const char *buf, co ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(classes), 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", function_name); + 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); From 0e62448567f93a5870ac26241f343242a58d5360 Mon Sep 17 00:00:00 2001 From: Gina Peter Bnayard Date: Thu, 8 Aug 2024 16:00:54 +0200 Subject: [PATCH 5/5] Add test for Stringable objects --- ...lowed_classes_option_stringable_value.phpt | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) 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_stringable_value.phpt b/ext/standard/tests/serialize/unserialize_allowed_classes_option_stringable_value.phpt new file mode 100644 index 0000000000000..5868cf9e923f0 --- /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" +}