Re: [PHP-DEV] Removing support for the disable_classes INI setting
On Mon, 14 Aug 2023 at 16:18, G. P. B. wrote: > Hello internals, > > While working on some DNF type bugs, I discovered some major issues around > the disable_classes INI setting implementation. Such as: > > - A double-free of the type of a typed property > - A use after free (which segfaults) when trying to access a property > defined on a disabled class that was extended (e.g. disabling Exception) > - A use after free when var_dumping a disabled class that has its own > handler (as it will assume properties to be allocated) > - And likely more considering the lack of tests surrounding this feature > > This feature seems of dubious nature, and the only justification given when > adding support for this in the changelog of PHP 4.3.2 is: > > New "disable_classes" php.ini option to allow administrators to disable > certain classes for security reasons. [1] > > However, only classes defined by extensions can be disabled, and such a > class is critical for the correct operation of said extension. > As such, what one should do for security reasons is to not enable the > extension in the first place. > > Moreover, compared to the behaviour of disable_functions which, as of PHP > 8.0, removes the function declaration completely, disable_classes does not > remove the declaration of the class, but just "overloads" the object > creation process to not initialize the object and emit a warning. > Meaning, it is totally valid to instantiate a disabled class and pass it > around to functions for them to blow up when they try to use the object as > intended. > > Considering the major flaws in the implementation of said feature, the > dubious nature of it, and the seeming lack of usage of it (considering none > of the above breaking bugs have been reported). > I would like to remove this feature in PHP 8.3, even though I know we are > past feature freeze and close to the first RC. > > I have CCed the RMs for 8.3, and would like the opinion of other people on > if this removal makes sense to the majority of people > > Sincerely, > > George P. Banyard > > [1] https://externals.io/message/2076 I'm not against removing the disable_classes, but fuzzer SAPI is using this ini setting like this: https://github.com/php/php-src/blob/1fe7dc31ef149db20ea3813c92a45deff80c21a3/sapi/fuzzer/fuzzer-sapi.c#L60 >From the user point of view, it would otherwise be good to have a certain set of PHP functionality that they can always rely on. Checking if some class or function is disabled in some environment is kind of strange indeed. But from the PHP configuration point of view, more options to customize the PHP environment is maybe good to have.
Re: [PHP-DEV] Removing support for the disable_classes INI setting
On Mon, 14 Aug 2023 at 20:03, Larry Garfield wrote: > However, I think it should not get an exception for code-freeze. At best, > I could see a deprecation warning on it in 8.3 and remove in 8.4/9, but I > defer to the RMs on that front. > In its current state, this INI setting does nothing other than cause segfaults. This functionality is broken, and frankly has always been. The only reason I am asking for an exception is to remove broken functionality that doesn't work and cannot be easily fixed. George P. Banyard
Re: [PHP-DEV] Removing support for the disable_classes INI setting
On 14-8-2023 16:17, G. P. B. wrote: Hello internals, While working on some DNF type bugs, I discovered some major issues around the disable_classes INI setting implementation. Such as: - A double-free of the type of a typed property - A use after free (which segfaults) when trying to access a property defined on a disabled class that was extended (e.g. disabling Exception) - A use after free when var_dumping a disabled class that has its own handler (as it will assume properties to be allocated) - And likely more considering the lack of tests surrounding this feature This feature seems of dubious nature, and the only justification given when adding support for this in the changelog of PHP 4.3.2 is: New "disable_classes" php.ini option to allow administrators to disable certain classes for security reasons. [1] However, only classes defined by extensions can be disabled, and such a class is critical for the correct operation of said extension. As such, what one should do for security reasons is to not enable the extension in the first place. Moreover, compared to the behaviour of disable_functions which, as of PHP 8.0, removes the function declaration completely, disable_classes does not remove the declaration of the class, but just "overloads" the object creation process to not initialize the object and emit a warning. Meaning, it is totally valid to instantiate a disabled class and pass it around to functions for them to blow up when they try to use the object as intended. Considering the major flaws in the implementation of said feature, the dubious nature of it, and the seeming lack of usage of it (considering none of the above breaking bugs have been reported). I would like to remove this feature in PHP 8.3, even though I know we are past feature freeze and close to the first RC. I have CCed the RMs for 8.3, and would like the opinion of other people on if this removal makes sense to the majority of people Sincerely, George P. Banyard [1] https://externals.io/message/2076 Hi George, For what its worth: in my experience, the `disable_classes` and `disable_functions` ini directives are mostly used by hosting companies providing shared/virtual host environments and, most often for those environments, not editable by their users via a control panel or nor do these users have (edit) access to the php.ini file. Declaring an ini directive in the php.ini file which has been removed will instantly cause a fatal error on starting PHP, so I wonder what the impact will be when a user switches PHP version (which they often can choose to do via a control panel). Hosts should (of course) know what they are doing and this should be handled when the user initiates the PHP version switch, but if hosts knew what they were doing, they would make the extension unavailable (as you already suggested above), so I wonder how many shared hosting users will run into trouble if this is removed without deprecation period ? Would deprecating it in PHP 8.3 and removing it in PHP 9.0 be an option ? As for the lack of bug reports - the typical type of end users using those hosts will not know to report these type of issues to PHP (or even be able to properly identify the issue). Instead they will complain extensively to whatever open source project (read: WordPress) they are running on the shared hosting without providing enough information for the open source maintainers to even begin to identify the actual issue... ;-) Just my two cents. Smile, Juliette
Re: [PHP-DEV] Removing support for the disable_classes INI setting
On Mon, Aug 14, 2023, at 2:17 PM, G. P. B. wrote: > Hello internals, > > While working on some DNF type bugs, I discovered some major issues around > the disable_classes INI setting implementation. Such as: > > - A double-free of the type of a typed property > - A use after free (which segfaults) when trying to access a property > defined on a disabled class that was extended (e.g. disabling Exception) > - A use after free when var_dumping a disabled class that has its own > handler (as it will assume properties to be allocated) > - And likely more considering the lack of tests surrounding this feature > > This feature seems of dubious nature, and the only justification given when > adding support for this in the changelog of PHP 4.3.2 is: > > New "disable_classes" php.ini option to allow administrators to disable > certain classes for security reasons. [1] > > However, only classes defined by extensions can be disabled, and such a > class is critical for the correct operation of said extension. > As such, what one should do for security reasons is to not enable the > extension in the first place. > > Moreover, compared to the behaviour of disable_functions which, as of PHP > 8.0, removes the function declaration completely, disable_classes does not > remove the declaration of the class, but just "overloads" the object > creation process to not initialize the object and emit a warning. > Meaning, it is totally valid to instantiate a disabled class and pass it > around to functions for them to blow up when they try to use the object as > intended. > > Considering the major flaws in the implementation of said feature, the > dubious nature of it, and the seeming lack of usage of it (considering none > of the above breaking bugs have been reported). > I would like to remove this feature in PHP 8.3, even though I know we are > past feature freeze and close to the first RC. > > I have CCed the RMs for 8.3, and would like the opinion of other people on > if this removal makes sense to the majority of people > > Sincerely, > > George P. Banyard > > [1] https://externals.io/message/2076 I believe the intent for it was the same as disable_functions: A "seemed like a good idea at the time" attempt to let shared hosters lock down their environment. That's an increasingly small percentage of the PHP using market, though, so I am perfectly happy to lose disabled_classes entirely. However, I think it should not get an exception for code-freeze. At best, I could see a deprecation warning on it in 8.3 and remove in 8.4/9, but I defer to the RMs on that front. --Larry Garfield -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
[PHP-DEV] Removing support for the disable_classes INI setting
Hello internals, While working on some DNF type bugs, I discovered some major issues around the disable_classes INI setting implementation. Such as: - A double-free of the type of a typed property - A use after free (which segfaults) when trying to access a property defined on a disabled class that was extended (e.g. disabling Exception) - A use after free when var_dumping a disabled class that has its own handler (as it will assume properties to be allocated) - And likely more considering the lack of tests surrounding this feature This feature seems of dubious nature, and the only justification given when adding support for this in the changelog of PHP 4.3.2 is: New "disable_classes" php.ini option to allow administrators to disable certain classes for security reasons. [1] However, only classes defined by extensions can be disabled, and such a class is critical for the correct operation of said extension. As such, what one should do for security reasons is to not enable the extension in the first place. Moreover, compared to the behaviour of disable_functions which, as of PHP 8.0, removes the function declaration completely, disable_classes does not remove the declaration of the class, but just "overloads" the object creation process to not initialize the object and emit a warning. Meaning, it is totally valid to instantiate a disabled class and pass it around to functions for them to blow up when they try to use the object as intended. Considering the major flaws in the implementation of said feature, the dubious nature of it, and the seeming lack of usage of it (considering none of the above breaking bugs have been reported). I would like to remove this feature in PHP 8.3, even though I know we are past feature freeze and close to the first RC. I have CCed the RMs for 8.3, and would like the opinion of other people on if this removal makes sense to the majority of people Sincerely, George P. Banyard [1] https://externals.io/message/2076
