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
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 peopleSincerely,
George P. Banyard
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
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
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 peopleSincerely,
George P. Banyard
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
On Mon, 14 Aug 2023 at 20:04, Juliette Reinders Folmer <
php-internals_nospam@adviesenzo.nl> wrote:
Hi George,
For what its worth: in my experience, the
disable_classesand
disable_functionsini 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.
To be clear, I have no intention of deprecating or removing
disable_functions.
This INI setting works very well and does not cause any issue within the
engine.
[...]
Would deprecating it in PHP 8.3 and removing it in PHP 9.0 be an option ?
One option is to "keep" the INI setting, but have it basically do nothing.
Is this what you had in mind?
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... ;-)
On a minimal build of PHP, with only the mandatory extensions enabled,
there are 148 classes/interfaces/traits defined. [1]
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 disable_functions, which can be used to
selectively remove functionality of an extension without breaking it
overall.
George P. Banyard
[1] https://gist.github.com/Girgias/63d55ba1e50b580412b004046daed02b
On Mon, 14 Aug 2023 at 20:04, Juliette Reinders Folmer <
php-internals_nospam@adviesenzo.nl> wrote:Would deprecating it in PHP 8.3 and removing it in PHP 9.0 be an option ?
One option is to "keep" the INI setting, but have it basically do nothing.
Is this what you had in mind?
That sounds iffy as well in a way as people (hosts) may then "think" it
works, while it doesn't.
Disabling the actually functionality (as broken) and then throwing a
warning could possibly be a middle-of-the-road solution ?
Note - warning, not deprecation if the functionality is turned off in
PHP 8.3.
With any luck, the hosts' error logs would fill up quickly enough for
them to take notice ;-)
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... ;-)
On a minimal build of PHP, with only the mandatory extensions enabled,
there are 148 classes/interfaces/traits defined. [1]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.
100% agreed. Unfortunately (in my experience), the hosts using this ini
setting are generally not what anyone would consider high quality hosts
who know what they are doing.... more the penny-and-dime teenager with a
server in the attic type... (which is hopefully a dying breed, but I've
seen too many of them come and go over the years - oh and apologies to
the potential
teenager-host-exception-to-the-rule-who-does-know-what-they-are-doing
who may be reading this... ).
And the type of end-user I was talking about would describe it as "You
broke my website"... (not realizing that it's not the open source
package, but the hosting which broke it)
Considering the above, I cannot see how this functionality was ever useful.
I have absolutely no doubts at all about your analysis. My only remarks
were about how to communicate this effectively to the type of
ill-informed hosts/end-users who will be affected by the removal.
Smile,
Juliette
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).
Are you sure about that? Setting ini entries that don't exist are simply ignored.
For example this will generate no warning, and certainly not a fatal error:
php -d elephpant.smells=no your-script.php
cheers
Derick
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).
Are you sure about that? Setting ini entries that don't exist are simply ignored.For example this will generate no warning, and certainly not a fatal error:
php -d elephpant.smells=no your-script.php
cheers
Derick
I though the same, then I ran some tests locally.... (before sending the
mail) don't tell me I managed to test with the one ini setting which
does cause a fatal ?
php -d allow_call_time_pass_reference=true your-script.php
Smile,
Juliette
Hi
Den man. 14. aug. 2023 kl. 22.05 skrev Juliette Reinders Folmer
php-internals_nospam@adviesenzo.nl:
Hi George,
For what its worth: in my experience, the
disable_classesand
disable_functionsini 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).
This depends on whether we choose to explicitly mark the ini directive
as one such. As the original implementor of that logic, it was meant
as an upgrade tool for PHP 5.3.0 when we also reorganized the php.ini
files, rather than have developers just copy over their old,
non-working files over to cause unexpected features to still be
available.
So there is always the option to not enlist it there, though it would
be ideal to do so to prevent developers from getting a false sense of
"security", but that is just my personal take.
--
regards,
Kalle Sommer Nielsen
kalle@php.net
Hi
Den man. 14. aug. 2023 kl. 22.05 skrev Juliette Reinders Folmer
php-internals_nospam@adviesenzo.nl:Hi George,
For what its worth: in my experience, the
disable_classesand
disable_functionsini 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).
This depends on whether we choose to explicitly mark the ini directive
as one such. As the original implementor of that logic, it was meant
as an upgrade tool for PHP 5.3.0 when we also reorganized the php.ini
files, rather than have developers just copy over their old,
non-working files over to cause unexpected features to still be
available.So there is always the option to not enlist it there, though it would
be ideal to do so to prevent developers from getting a false sense of
"security", but that is just my personal take.
Ah, that explains it! Thanks for adding that context Kalle!
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 peopleSincerely,
George P. Banyard
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.