From f8d1864bbbfbdc63d1b8e4e58934f8d8668f55bf Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Wed, 1 May 2024 18:32:07 +0200 Subject: [PATCH] Delay #[Attribute] arg validation until runtime Fixes GH-13970 Closes GH-14105 We cannot validate at compile-time for multiple reasons: * Evaluating the argument naively with zend_get_attribute_value can lead to code execution at compile time through the new expression, leading to possible reentrance of the compiler. * Even if the evaluation was possible, it would need to be restricted to the current file, because constant values coming from other files can change without affecting the current compilation unit. For this reason, validation would need to be repeated at runtime anyway. * Enums cannot be instantiated at compile-time (the actual bug report). This could be allowed here, because the value is immediately destroyed. But given the other issues, this won't be needed. Instead, we just move it to runtime entirely. It's only needed for ReflectionAttribute::newInstance(), which is not particularly a hot path. The checks are also simple. --- NEWS | 4 +++ ...021_attribute_flags_type_is_validated.phpt | 13 ++++++++-- ...22_attribute_flags_value_is_validated.phpt | 13 ++++++++-- .../023_ast_node_in_validation.phpt | 13 ++++++++-- .../032_attribute_validation_scope.phpt | 16 +++++++++--- ...gs_type_is_not_validated_at_comp_time.phpt | 12 +++++++++ Zend/zend_attributes.c | 26 ++++++++++++------- Zend/zend_attributes.h | 2 ++ ext/reflection/php_reflection.c | 13 +++------- ext/zend_test/tests/gh13970.phpt | 22 ++++++++++++++++ 10 files changed, 106 insertions(+), 28 deletions(-) create mode 100644 Zend/tests/attributes/033_attribute_flags_type_is_not_validated_at_comp_time.phpt create mode 100644 ext/zend_test/tests/gh13970.phpt diff --git a/NEWS b/NEWS index dc16e1fdb71..92d93bed27f 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,10 @@ PHP NEWS . Fixed buffer limit on Windows, replacing read call usage by _read. (David Carlier) +- Core: + . Fixed bug GH-13970 (Incorrect validation of #[Attribute] flags type for + non-compile-time expressions). (ilutov) + - DOM: . Fix crashes when entity declaration is removed while still having entity references. (nielsdos) diff --git a/Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt b/Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt index 01ab29c5efa..70e4fdb7f33 100644 --- a/Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt +++ b/Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt @@ -6,6 +6,15 @@ Attribute flags type is validated. #[Attribute("foo")] class A1 { } +#[A1] +class Foo {} + +try { + (new ReflectionClass(Foo::class))->getAttributes()[0]->newInstance(); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + ?> ---EXPECTF-- -Fatal error: Attribute::__construct(): Argument #1 ($flags) must be of type int, string given in %s +--EXPECT-- +Attribute::__construct(): Argument #1 ($flags) must be of type int, string given diff --git a/Zend/tests/attributes/022_attribute_flags_value_is_validated.phpt b/Zend/tests/attributes/022_attribute_flags_value_is_validated.phpt index 72433a9f139..efaa969af82 100644 --- a/Zend/tests/attributes/022_attribute_flags_value_is_validated.phpt +++ b/Zend/tests/attributes/022_attribute_flags_value_is_validated.phpt @@ -6,6 +6,15 @@ Attribute flags value is validated. #[Attribute(-1)] class A1 { } +#[A1] +class Foo { } + +try { + var_dump((new ReflectionClass(Foo::class))->getAttributes()[0]->newInstance()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + ?> ---EXPECTF-- -Fatal error: Invalid attribute flags specified in %s +--EXPECT-- +Invalid attribute flags specified diff --git a/Zend/tests/attributes/023_ast_node_in_validation.phpt b/Zend/tests/attributes/023_ast_node_in_validation.phpt index 332d83fe86f..063a6b7e815 100644 --- a/Zend/tests/attributes/023_ast_node_in_validation.phpt +++ b/Zend/tests/attributes/023_ast_node_in_validation.phpt @@ -6,6 +6,15 @@ Attribute flags value is validated. #[Attribute(Foo::BAR)] class A1 { } +#[A1] +class Bar { } + +try { + var_dump((new ReflectionClass(Bar::class))->getAttributes()[0]->newInstance()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + ?> ---EXPECTF-- -Fatal error: Class "Foo" not found in %s on line %d +--EXPECT-- +Class "Foo" not found diff --git a/Zend/tests/attributes/032_attribute_validation_scope.phpt b/Zend/tests/attributes/032_attribute_validation_scope.phpt index 039a427254f..d157c35929b 100644 --- a/Zend/tests/attributes/032_attribute_validation_scope.phpt +++ b/Zend/tests/attributes/032_attribute_validation_scope.phpt @@ -1,9 +1,19 @@ --TEST-- -Validation for "Attribute" does not use a scope when evaluating constant ASTs +Validation for "Attribute" uses the class scope when evaluating constant ASTs --FILE-- getAttributes()[0]->newInstance()); ?> ---EXPECTF-- -Fatal error: Cannot access "parent" when no class scope is active in %s on line %d +--EXPECT-- +object(x)#1 (0) { +} diff --git a/Zend/tests/attributes/033_attribute_flags_type_is_not_validated_at_comp_time.phpt b/Zend/tests/attributes/033_attribute_flags_type_is_not_validated_at_comp_time.phpt new file mode 100644 index 00000000000..76b29c65ba8 --- /dev/null +++ b/Zend/tests/attributes/033_attribute_flags_type_is_not_validated_at_comp_time.phpt @@ -0,0 +1,12 @@ +--TEST-- +Attribute flags type is not validated at compile time. +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/zend_attributes.c b/Zend/zend_attributes.c index 5a446c8c285..2471a6739e3 100644 --- a/Zend/zend_attributes.c +++ b/Zend/zend_attributes.c @@ -35,31 +35,39 @@ static zend_object_handlers attributes_object_handlers_sensitive_parameter_value static HashTable internal_attributes; void validate_attribute(zend_attribute *attr, uint32_t target, zend_class_entry *scope) +{ +} + +uint32_t zend_attribute_attribute_get_flags(zend_attribute *attr, zend_class_entry *scope) { // TODO: More proper signature validation: Too many args, incorrect arg names. if (attr->argc > 0) { zval flags; - /* As this is run in the middle of compilation, fetch the attribute value without - * specifying a scope. The class is not fully linked yet, and we may seen an - * inconsistent state. */ - if (FAILURE == zend_get_attribute_value(&flags, attr, 0, NULL)) { - return; + if (FAILURE == zend_get_attribute_value(&flags, attr, 0, scope)) { + ZEND_ASSERT(EG(exception)); + return 0; } if (Z_TYPE(flags) != IS_LONG) { - zend_error_noreturn(E_ERROR, + zend_throw_error(NULL, "Attribute::__construct(): Argument #1 ($flags) must be of type int, %s given", zend_zval_type_name(&flags) ); + zval_ptr_dtor(&flags); + return 0; } - if (Z_LVAL(flags) & ~ZEND_ATTRIBUTE_FLAGS) { - zend_error_noreturn(E_ERROR, "Invalid attribute flags specified"); + uint32_t flags_l = Z_LVAL(flags); + if (flags_l & ~ZEND_ATTRIBUTE_FLAGS) { + zend_throw_error(NULL, "Invalid attribute flags specified"); + return 0; } - zval_ptr_dtor(&flags); + return flags_l; } + + return ZEND_ATTRIBUTE_TARGET_ALL; } static void validate_allow_dynamic_properties( diff --git a/Zend/zend_attributes.h b/Zend/zend_attributes.h index fc02a7d656e..746155f5147 100644 --- a/Zend/zend_attributes.h +++ b/Zend/zend_attributes.h @@ -85,6 +85,8 @@ ZEND_API zend_attribute *zend_add_attribute( HashTable **attributes, zend_string *name, uint32_t argc, uint32_t flags, uint32_t offset, uint32_t lineno); +uint32_t zend_attribute_attribute_get_flags(zend_attribute *attr, zend_class_entry *scope); + END_EXTERN_C() static zend_always_inline zend_attribute *zend_add_class_attribute(zend_class_entry *ce, zend_string *name, uint32_t argc) diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 136d69b2864..f24b00a2857 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -6739,16 +6739,9 @@ ZEND_METHOD(ReflectionAttribute, newInstance) } if (ce->type == ZEND_USER_CLASS) { - uint32_t flags = ZEND_ATTRIBUTE_TARGET_ALL; - - if (marker->argc > 0) { - zval tmp; - - if (FAILURE == zend_get_attribute_value(&tmp, marker, 0, ce)) { - RETURN_THROWS(); - } - - flags = (uint32_t) Z_LVAL(tmp); + uint32_t flags = zend_attribute_attribute_get_flags(marker, ce); + if (EG(exception)) { + RETURN_THROWS(); } if (!(attr->target & flags)) { diff --git a/ext/zend_test/tests/gh13970.phpt b/ext/zend_test/tests/gh13970.phpt new file mode 100644 index 00000000000..f6b68f89702 --- /dev/null +++ b/ext/zend_test/tests/gh13970.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-13970 (Incorrect validation of #[\Attribute]'s first parameter) +--EXTENSIONS-- +zend_test +--FILE-- +getAttributes()[0]->newInstance()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} +?> +--EXPECT-- +Attribute::__construct(): Argument #1 ($flags) must be of type int, ZendTestUnitEnum given