From 4b502d90a9b9506b28026c464ec048aa9a81f4e4 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 24 Aug 2023 14:25:12 +0100 Subject: [PATCH 1/4] core: Remove disable_classes INI setting As described in the email to the PHP internals list [1] this feature is fundamentally broken and pointless. Only internal classes can be disable which brings the following observation. On a minimal build of PHP, with only the mandatory extensions enabled, there are 148 classes/interfaces/traits defined. [2] Other than the SPL ones (and even then), disabling any of these classes will cause issues within the engine. Moreover, the SPL ones are not a security concern. Therefore, any other class that can be disabled must come from an extension that can be disabled altogether. And "disabling" a class from an extension without disabling said extension will render it useless anyway. If a hosting provided is concerned about an extension, then it should not enable it in the first place. Not break it ad hoc. Considering the above, I cannot see how this functionality was ever useful. This is in stark contrast to the disable_functions INI setting, which can be used to selectively remove functionality of an extension without breaking it overall. What makes this setting particularly broken is that it does not unregister the class, it only overwrites the create CE handler to emit a warning and purge the properties and function hashtables. This leads to various use after free, segfaults, and broken expectations for the engine and extensions which define said classes. On top of that, it is possible to actually instantiate such a class (and even classes which actually disallow this like ext/imap) in userland, and pass it to function that are typed against said class without raising a TypeError. However, when trying to do anything with said object stuff is going to explode in countless ways. [1] https://news-web.php.net/php.internals/120896 [2] https://gist.github.com/Girgias/63d55ba1e50b580412b004046daed02b --- Zend/tests/bug77494.phpt | 23 -------- Zend/tests/disable_classes_warning.phpt | 27 ++++++++++ Zend/tests/errmsg/errmsg_021.phpt | 17 ------ Zend/zend_API.c | 71 ------------------------- Zend/zend_API.h | 1 - main/main.c | 55 +++++-------------- main/php_globals.h | 1 - php.ini-development | 5 -- php.ini-production | 5 -- sapi/fpm/fpm/fpm_php.c | 36 ------------- sapi/fpm/www.conf.in | 5 +- 11 files changed, 43 insertions(+), 203 deletions(-) delete mode 100644 Zend/tests/bug77494.phpt create mode 100644 Zend/tests/disable_classes_warning.phpt delete mode 100644 Zend/tests/errmsg/errmsg_021.phpt diff --git a/Zend/tests/bug77494.phpt b/Zend/tests/bug77494.phpt deleted file mode 100644 index 7f808c2c0f52a..0000000000000 --- a/Zend/tests/bug77494.phpt +++ /dev/null @@ -1,23 +0,0 @@ ---TEST-- -Bug #77494 (Disabling class causes segfault on member access) ---EXTENSIONS-- -curl ---INI-- -disable_classes=CURLFile,ErrorException ---FILE-- -name); -$b = new ErrorException(); -var_dump($b->message); -?> ---EXPECTF-- -Warning: CURLFile() has been disabled for security reasons in %sbug77494.php on line 2 - -Warning: Undefined property: CURLFile::$name in %s on line %d -NULL - -Warning: ErrorException() has been disabled for security reasons in %s on line %d - -Warning: Undefined property: ErrorException::$message in %s on line %d -NULL diff --git a/Zend/tests/disable_classes_warning.phpt b/Zend/tests/disable_classes_warning.phpt new file mode 100644 index 0000000000000..c3692bf52972f --- /dev/null +++ b/Zend/tests/disable_classes_warning.phpt @@ -0,0 +1,27 @@ +--TEST-- +Check that warning is emitted when disabling classes +--INI-- +disable_classes=Exception +--FILE-- + +--EXPECTF-- +object(Exception)#1 (7) { + ["message":protected]=> + string(0) "" + ["string":"Exception":private]=> + string(0) "" + ["code":protected]=> + int(0) + ["file":protected]=> + string(%d) "%s" + ["line":protected]=> + int(2) + ["trace":"Exception":private]=> + array(0) { + } + ["previous":"Exception":private]=> + NULL +} diff --git a/Zend/tests/errmsg/errmsg_021.phpt b/Zend/tests/errmsg/errmsg_021.phpt deleted file mode 100644 index 7514e68359044..0000000000000 --- a/Zend/tests/errmsg/errmsg_021.phpt +++ /dev/null @@ -1,17 +0,0 @@ ---TEST-- -errmsg: disabled class ---INI-- -disable_classes=stdclass ---FILE-- - ---EXPECTF-- -Warning: test() has been disabled for security reasons in %s on line %d -Done diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 8ac140cdceffc..fa5365b776d2c 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -3692,77 +3692,6 @@ ZEND_API void zend_disable_functions(const char *function_list) /* {{{ */ } /* }}} */ -#ifdef ZEND_WIN32 -#pragma optimize("", off) -#endif -static ZEND_COLD zend_object *display_disabled_class(zend_class_entry *class_type) /* {{{ */ -{ - zend_object *intern; - - intern = zend_objects_new(class_type); - - /* Initialize default properties */ - if (EXPECTED(class_type->default_properties_count != 0)) { - zval *p = intern->properties_table; - zval *end = p + class_type->default_properties_count; - do { - ZVAL_UNDEF(p); - p++; - } while (p != end); - } - - zend_error(E_WARNING, "%s() has been disabled for security reasons", ZSTR_VAL(class_type->name)); - return intern; -} -#ifdef ZEND_WIN32 -#pragma optimize("", on) -#endif -/* }}} */ - -static const zend_function_entry disabled_class_new[] = { - ZEND_FE_END -}; - -ZEND_API zend_result zend_disable_class(const char *class_name, size_t class_name_length) /* {{{ */ -{ - zend_class_entry *disabled_class; - zend_string *key; - zend_function *fn; - zend_property_info *prop; - - key = zend_string_alloc(class_name_length, 0); - zend_str_tolower_copy(ZSTR_VAL(key), class_name, class_name_length); - disabled_class = zend_hash_find_ptr(CG(class_table), key); - zend_string_release_ex(key, 0); - if (!disabled_class) { - return FAILURE; - } - - /* Will be reset by INIT_CLASS_ENTRY. */ - free(disabled_class->interfaces); - - INIT_CLASS_ENTRY_INIT_METHODS((*disabled_class), disabled_class_new); - disabled_class->create_object = display_disabled_class; - - ZEND_HASH_MAP_FOREACH_PTR(&disabled_class->function_table, fn) { - if ((fn->common.fn_flags & (ZEND_ACC_HAS_RETURN_TYPE|ZEND_ACC_HAS_TYPE_HINTS)) && - fn->common.scope == disabled_class) { - zend_free_internal_arg_info(&fn->internal_function); - } - } ZEND_HASH_FOREACH_END(); - zend_hash_clean(&disabled_class->function_table); - ZEND_HASH_MAP_FOREACH_PTR(&disabled_class->properties_info, prop) { - if (prop->ce == disabled_class) { - zend_string_release(prop->name); - zend_type_release(prop->type, /* persistent */ 1); - free(prop); - } - } ZEND_HASH_FOREACH_END(); - zend_hash_clean(&disabled_class->properties_info); - return SUCCESS; -} -/* }}} */ - static zend_always_inline zend_class_entry *get_scope(zend_execute_data *frame) { return frame && frame->func ? frame->func->common.scope : NULL; diff --git a/Zend/zend_API.h b/Zend/zend_API.h index 78a30d630ae6e..f4a6fc5f50e9d 100644 --- a/Zend/zend_API.h +++ b/Zend/zend_API.h @@ -405,7 +405,6 @@ static zend_always_inline zend_result zend_register_class_alias(const char *name zend_register_class_alias_ex(ZEND_NS_NAME(ns, name), sizeof(ZEND_NS_NAME(ns, name))-1, ce, 1) ZEND_API void zend_disable_functions(const char *function_list); -ZEND_API zend_result zend_disable_class(const char *class_name, size_t class_name_length); ZEND_API ZEND_COLD void zend_wrong_param_count(void); ZEND_API ZEND_COLD void zend_wrong_property_read(zval *object, zval *property); diff --git a/main/main.c b/main/main.c index 8edf214bc3f9a..4ea24d7c35cf3 100644 --- a/main/main.c +++ b/main/main.c @@ -413,41 +413,6 @@ static PHP_INI_MH(OnSetLogFilter) } /* }}} */ -/* {{{ php_disable_classes */ -static void php_disable_classes(void) -{ - char *s = NULL, *e; - - if (!*(INI_STR("disable_classes"))) { - return; - } - - e = PG(disable_classes) = strdup(INI_STR("disable_classes")); - - while (*e) { - switch (*e) { - case ' ': - case ',': - if (s) { - *e = '\0'; - zend_disable_class(s, e-s); - s = NULL; - } - break; - default: - if (!s) { - s = e; - } - break; - } - e++; - } - if (s) { - zend_disable_class(s, e-s); - } -} -/* }}} */ - /* {{{ php_binary_init */ static void php_binary_init(void) { @@ -772,6 +737,17 @@ static PHP_INI_MH(OnChangeMailForceExtra) } /* }}} */ +/* Emit warning when using this INI setting as it is removed */ +static PHP_INI_MH(OnChangeDisableClasses) +{ + if (stage != PHP_INI_SYSTEM) { + return FAILURE; + } + php_error_docref("disable_clases", E_WARNING, "The disable_classes INI setting has been removed and has no effect"); + + return SUCCESS; +} + /* defined in browscap.c */ PHP_INI_MH(OnChangeBrowscap); @@ -872,7 +848,8 @@ PHP_INI_BEGIN() PHP_INI_ENTRY("sendmail_path", DEFAULT_SENDMAIL_PATH, PHP_INI_SYSTEM, NULL) PHP_INI_ENTRY("mail.force_extra_parameters",NULL, PHP_INI_SYSTEM|PHP_INI_PERDIR, OnChangeMailForceExtra) PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL) - PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, NULL) + // TODO Add warning when disabling classes + //PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, NULL) PHP_INI_ENTRY("max_file_uploads", "20", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) PHP_INI_ENTRY("max_multipart_body_parts", "-1", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) @@ -2105,9 +2082,6 @@ static void core_globals_dtor(php_core_globals *core_globals) ZEND_ASSERT(!core_globals->last_error_message); ZEND_ASSERT(!core_globals->last_error_file); - if (core_globals->disable_classes) { - free(core_globals->disable_classes); - } if (core_globals->php_binary) { free(core_globals->php_binary); } @@ -2372,9 +2346,8 @@ zend_result php_module_startup(sapi_module_struct *sf, zend_module_entry *additi } } - /* disable certain classes and functions as requested by php.ini */ + /* disable certain functions as requested by php.ini */ zend_disable_functions(INI_STR("disable_functions")); - php_disable_classes(); /* make core report what it should */ if ((module = zend_hash_str_find_ptr(&module_registry, "core", sizeof("core")-1)) != NULL) { diff --git a/main/php_globals.h b/main/php_globals.h index 94e5aa44fc59f..61df82ac99f5f 100644 --- a/main/php_globals.h +++ b/main/php_globals.h @@ -143,7 +143,6 @@ struct _php_core_globals { char *php_sys_temp_dir; - char *disable_classes; zend_long max_input_nesting_level; zend_long max_input_vars; diff --git a/php.ini-development b/php.ini-development index 5cbf6a54ee44c..9e7039e9d1bf0 100644 --- a/php.ini-development +++ b/php.ini-development @@ -327,11 +327,6 @@ serialize_precision = -1 ; https://php.net/disable-functions disable_functions = -; This directive allows you to disable certain classes. -; It receives a comma-delimited list of class names. -; https://php.net/disable-classes -disable_classes = - ; Colors for Syntax Highlighting mode. Anything that's acceptable in ; would work. ; https://php.net/syntax-highlighting diff --git a/php.ini-production b/php.ini-production index 697c7073ebde0..17752f80918cb 100644 --- a/php.ini-production +++ b/php.ini-production @@ -327,11 +327,6 @@ serialize_precision = -1 ; https://php.net/disable-functions disable_functions = -; This directive allows you to disable certain classes. -; It receives a comma-delimited list of class names. -; https://php.net/disable-classes -disable_classes = - ; Colors for Syntax Highlighting mode. Anything that's acceptable in ; would work. ; https://php.net/syntax-highlighting diff --git a/sapi/fpm/fpm/fpm_php.c b/sapi/fpm/fpm/fpm_php.c index 55b0377c1ad06..97d35d42bfe66 100644 --- a/sapi/fpm/fpm/fpm_php.c +++ b/sapi/fpm/fpm/fpm_php.c @@ -48,35 +48,6 @@ static int fpm_php_zend_ini_alter_master(char *name, int name_length, char *new_ } /* }}} */ -static void fpm_php_disable(char *value, int (*zend_disable)(const char *, size_t)) /* {{{ */ -{ - char *s = 0, *e = value; - - while (*e) { - switch (*e) { - case ' ': - case ',': - if (s) { - *e = '\0'; - zend_disable(s, e - s); - s = 0; - } - break; - default: - if (!s) { - s = e; - } - break; - } - e++; - } - - if (s) { - zend_disable(s, e - s); - } -} -/* }}} */ - #define FPM_PHP_INI_ALTERING_ERROR -1 #define FPM_PHP_INI_APPLIED 1 #define FPM_PHP_INI_EXTENSION_FAILED 0 @@ -121,13 +92,6 @@ int fpm_php_apply_defines_ex(struct key_value_s *kv, int mode) /* {{{ */ return FPM_PHP_INI_APPLIED; } - if (!strcmp(name, "disable_classes") && *value) { - char *v = strdup(value); - PG(disable_classes) = v; - fpm_php_disable(v, zend_disable_class); - return FPM_PHP_INI_APPLIED; - } - return FPM_PHP_INI_APPLIED; } /* }}} */ diff --git a/sapi/fpm/www.conf.in b/sapi/fpm/www.conf.in index 9689e4defab04..69df3e6630047 100644 --- a/sapi/fpm/www.conf.in +++ b/sapi/fpm/www.conf.in @@ -474,9 +474,8 @@ pm.max_spare_servers = 3 ; For php_*flag, valid values are on, off, 1, 0, true, false, yes or no. ; Defining 'extension' will load the corresponding shared extension from -; extension_dir. Defining 'disable_functions' or 'disable_classes' will not -; overwrite previously defined php.ini values, but will append the new value -; instead. +; extension_dir. Defining 'disable_functions' will not overwrite previously +; defined php.ini values, but will append the new value instead. ; Note: path INI options can be relative and will be expanded with the prefix ; (pool, global or @prefix@) From 4122e55e9f87007be25441c22c8e037ef16ab43f Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 24 Aug 2023 23:19:29 +0100 Subject: [PATCH 2/4] try to trigger a E_WARNING --- main/main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/main/main.c b/main/main.c index 4ea24d7c35cf3..f856e6ef06877 100644 --- a/main/main.c +++ b/main/main.c @@ -743,9 +743,9 @@ static PHP_INI_MH(OnChangeDisableClasses) if (stage != PHP_INI_SYSTEM) { return FAILURE; } - php_error_docref("disable_clases", E_WARNING, "The disable_classes INI setting has been removed and has no effect"); + php_error_docref("disable_classes", E_WARNING, "The disable_classes INI setting has been removed and has no effect"); - return SUCCESS; + return FAILURE; } /* defined in browscap.c */ @@ -848,8 +848,7 @@ PHP_INI_BEGIN() PHP_INI_ENTRY("sendmail_path", DEFAULT_SENDMAIL_PATH, PHP_INI_SYSTEM, NULL) PHP_INI_ENTRY("mail.force_extra_parameters",NULL, PHP_INI_SYSTEM|PHP_INI_PERDIR, OnChangeMailForceExtra) PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL) - // TODO Add warning when disabling classes - //PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, NULL) + PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, OnChangeDisableClasses) PHP_INI_ENTRY("max_file_uploads", "20", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) PHP_INI_ENTRY("max_multipart_body_parts", "-1", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) From 13c3ba19fde8ee84d122c445955421abd0c9a5f2 Mon Sep 17 00:00:00 2001 From: Gina Peter Bnayard Date: Sat, 25 May 2024 15:20:33 +0200 Subject: [PATCH 3/4] Fuzzer zts stuff --- sapi/fuzzer/fuzzer-sapi.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/sapi/fuzzer/fuzzer-sapi.c b/sapi/fuzzer/fuzzer-sapi.c index 3cffacae1d172..a81e533a6b5f0 100644 --- a/sapi/fuzzer/fuzzer-sapi.c +++ b/sapi/fuzzer/fuzzer-sapi.c @@ -56,8 +56,6 @@ static const char HARDCODED_INI[] = ",crypt" /* openlog() has a known memory-management issue. */ ",openlog" - /* Can cause long loops that bypass the executor step limit. */ - "\ndisable_classes=InfiniteIterator" ; static int startup(sapi_module_struct *sapi_module) @@ -128,6 +126,21 @@ static sapi_module_struct fuzzer_module = { STANDARD_SAPI_MODULE_PROPERTIES }; +static ZEND_COLD zend_object *disable_class_create_handler(zend_class_entry *class_type) /* {{{ */ +{ + zend_throw_error(NULL, "Cannot construct class %s, as it is disabled", ZSTR_VAL(class_type->name)); + return NULL; +} + +static void fuzzer_disable_classes(void) +{ + /* Overwrite built-in constructor for InfiniteIterator as it + * can cause long loops that bypass the executor step limit. */ + /* Lowercase as this is how the CE as stored */ + zend_class_entry *InfiniteIterator_class = zend_hash_str_find(CG(class_table), "infiniteiterator", strlen("infiniteiterator")); + InfiniteIterator_class->create_object = disable_class_create_handler; +} + int fuzzer_init_php(const char *extra_ini) { #ifdef __SANITIZE_ADDRESS__ @@ -183,6 +196,8 @@ int fuzzer_request_startup(void) SIGG(check) = 0; #endif + fuzzer_disable_classes(); + return SUCCESS; } From 335b454200ba4b7e7742545d0c7b1a46a3749091 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Sun, 24 Aug 2025 18:36:20 +0200 Subject: [PATCH 4/4] Use global removed INI diagnostic method rather than a custom INI setter --- main/main.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/main/main.c b/main/main.c index f856e6ef06877..eb842474240fd 100644 --- a/main/main.c +++ b/main/main.c @@ -737,17 +737,6 @@ static PHP_INI_MH(OnChangeMailForceExtra) } /* }}} */ -/* Emit warning when using this INI setting as it is removed */ -static PHP_INI_MH(OnChangeDisableClasses) -{ - if (stage != PHP_INI_SYSTEM) { - return FAILURE; - } - php_error_docref("disable_classes", E_WARNING, "The disable_classes INI setting has been removed and has no effect"); - - return FAILURE; -} - /* defined in browscap.c */ PHP_INI_MH(OnChangeBrowscap); @@ -848,7 +837,6 @@ PHP_INI_BEGIN() PHP_INI_ENTRY("sendmail_path", DEFAULT_SENDMAIL_PATH, PHP_INI_SYSTEM, NULL) PHP_INI_ENTRY("mail.force_extra_parameters",NULL, PHP_INI_SYSTEM|PHP_INI_PERDIR, OnChangeMailForceExtra) PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL) - PHP_INI_ENTRY("disable_classes", "", PHP_INI_SYSTEM, OnChangeDisableClasses) PHP_INI_ENTRY("max_file_uploads", "20", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) PHP_INI_ENTRY("max_multipart_body_parts", "-1", PHP_INI_SYSTEM|PHP_INI_PERDIR, NULL) @@ -2375,7 +2363,7 @@ zend_result php_module_startup(sapi_module_struct *sf, zend_module_entry *additi struct { const long error_level; const char *phrase; - const char *directives[18]; /* Remember to change this if the number of directives change */ + const char *directives[19]; /* Remember to change this if the number of directives change */ } directives[2] = { { E_DEPRECATED, @@ -2406,6 +2394,7 @@ zend_result php_module_startup(sapi_module_struct *sf, zend_module_entry *additi "safe_mode_protected_env_vars", "zend.ze1_compatibility_mode", "track_errors", + "disable_classes", NULL } }