Everything

php

Improve (disable|enable)_functions #65386

php.internals

Ben RUBSON

9 years ago

Hello,

As proposed by cmb (thank you !), I open a discussion regarding req #65386 :
https://bugs.php.net/bug.php?id=65386

It summarizes requests around disable_functions directive :
- modification of disable_functions to be a PHP_INI_SYSTEM directive ;
- implementation of enable_functions as a PHP_INI_SYSTEM directive ;
- support of wildcards in these 2 directives.

As a result, we would then for example be able to disable functions in php.ini,
and re-enable some of them in some of the virtualhosts of the httpd.conf.
And using wildcards, to quickly/exhaustively work on a bunch of func : pcntl_*

Thank you very much for your interest and support !

Best regards,

Ben

Ben RUBSON

9 years ago

> On 19 Jan 2017, at 12:18, Ben RUBSON <ben.rubson@gmail.com> wrote:
>
> Hello,
>
> As proposed by cmb (thank you !), I open a discussion regarding req #65386 :
> https://bugs.php.net/bug.php?id=65386
>
> It summarizes requests around disable_functions directive :
> - modification of disable_functions to be a PHP_INI_SYSTEM directive ;
> - implementation of enable_functions as a PHP_INI_SYSTEM directive ;
> - support of wildcards in these 2 directives.
>
> As a result, we would then for example be able to disable functions in php.ini,
> and re-enable some of them in some of the virtualhosts of the httpd.conf.
> And using wildcards, to quickly/exhaustively work on a bunch of func : pcntl_*
>
> Thank you very much for your interest and support !
>
> Best regards,
>
> Ben

Hi,

Any thoughts ?

Many thanks !

Ben

Sara Golemon

9 years ago

On Thu, Jan 19, 2017 at 6:18 AM, Ben RUBSON <ben.rubson@gmail.com> wrote:

> As proposed by cmb (thank you !), I open a discussion regarding req #65386 :
> https://bugs.php.net/bug.php?id=65386
>
> It summarizes requests around disable_functions directive :
> - modification of disable_functions to be a PHP_INI_SYSTEM directive ;
>

Could you clarify? `disable_functions` *IS* a PHP_INI_SYSTEM directive:

PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL)

> - implementation of enable_functions as a PHP_INI_SYSTEM directive ;
>

I'm not a big fan of a whitelist for weakening/overriding a blacklist setting.

There's also a technical hurdle here due to the way that functions are
(currently) disabled.  It's INI_SYSTEM because enabling/disabling on a
per-request (per vhost essentially means per request) basis means a
lot more heavy lifting than disabling on a system-wide level (we just
replace the function implementation in the global table with a STFU
message).

func->handler = ZEND_FN(display_disabled_function);

> - support of wildcards in these 2 directives.
>

I could potentially get down with wildcards.  It's way easier to
exhaustively cover an entire class of functions, but if the goal is to
disable an entire extension's worth of functions, wouldn't one
just.... not load that extension?

I understand this part makes more sense with the `enable_functions`
idea, but... see above.

-Sara

Ben RUBSON

9 years ago

> On 09 Feb 2017, at 18:12, Sara Golemon <pollita@php.net> wrote:

Thank you very much Sara for your feedback !

> On Thu, Jan 19, 2017 at 6:18 AM, Ben RUBSON <ben.rubson@gmail.com> wrote:
>> As proposed by cmb (thank you !), I open a discussion regarding req #65386 :
>> https://bugs.php.net/bug.php?id=65386
>>
>> It summarizes requests around disable_functions directive :
>> - modification of disable_functions to be a PHP_INI_SYSTEM directive ;
>>
> Could you clarify? `disable_functions` *IS* a PHP_INI_SYSTEM directive:
>
> PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL)

However, as per all the bugs referenced by #65386,
using "php_admin_value disable_functions" in httpd.conf does not correctly work.
The main issue certainly is that it can't be used per vhost.
(note that it however still affects their local ini value)

>> - implementation of enable_functions as a PHP_INI_SYSTEM directive ;
>>
> I'm not a big fan of a whitelist for weakening/overriding a blacklist setting.

(*)

> There's also a technical hurdle here due to the way that functions are
> (currently) disabled.  It's INI_SYSTEM because enabling/disabling on a
> per-request (per vhost essentially means per request) basis means a
> lot more heavy lifting than disabling on a system-wide level (we just
> replace the function implementation in the global table with a STFU
> message).

Sounds like it would then require some dev to achieve this behaviour.
Would it however hurt requests performance ?

> func->handler = ZEND_FN(display_disabled_function);
>
>
>> - support of wildcards in these 2 directives.
>>
> I could potentially get down with wildcards.  It's way easier to
> exhaustively cover an entire class of functions,

Yep, as it's quite hard to maintain an exhaustive list of functions...

> but if the goal is to
> disable an entire extension's worth of functions, wouldn't one
> just.... not load that extension?

Because one would certainly want to re-enable one of these functions in
one of its vhosts (or even globally in php.ini) :)

> I understand this part makes more sense with the `enable_functions`
> idea, but... see above.

Yes, this would really be convenient :
- in php.ini           : disable_functions = *socket*
- in a "trusted" vhost : enable_functions = stream_socket_client

And as only the server administrator can tune these parameters, there is
no weakening (*) risk :)

Thank you !

Ben

Sara Golemon

9 years ago

On Fri, Feb 10, 2017 at 4:02 AM, Ben RUBSON <ben.rubson@gmail.com> wrote:

>> On 09 Feb 2017, at 18:12, Sara Golemon <pollita@php.net> wrote:
>> Could you clarify? `disable_functions` *IS* a PHP_INI_SYSTEM directive:
>>
>> PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL)
>
> However, as per all the bugs referenced by #65386,
> using "php_admin_value disable_functions" in httpd.conf does not correctly work.
> The main issue certainly is that it can't be used per vhost.
> (note that it however still affects their local ini value)
>

That's not surprising.  When disable_functions processes an INI
change, it's a one-way affair.  Any functions listed in the INI
setting are clobbered and their original handlers are lost (on
purpose, I imagine).

When a new setting comes in to override it (via php_admin_value) it
can only add to the list of disabled functions, not restore previously
disabled functions.

Again, I'm not certain, but my gut says this was intentional.

>> There's also a technical hurdle here due to the way that functions are
>> (currently) disabled.  It's INI_SYSTEM because enabling/disabling on a
>> per-request (per vhost essentially means per request) basis means a
>> lot more heavy lifting than disabling on a system-wide level (we just
>> replace the function implementation in the global table with a STFU
>> message).
>
> Sounds like it would then require some dev to achieve this behaviour.
> Would it however hurt requests performance ?
>

TL;DR - Yes.

If you want a single process to serve some requests with function X
disabled and some to serve it with that function enabled, then SOME
amount of per-request processing has to be done.  Maybe it's done on
RINIT to shuffle around the function table, maybe it's done at
function call time via a flag, maybe it's done by compiling to a
different instruction for "potentially disabled function call".  I'm
not sure as I haven't thought very far into the problem.

>> but if the goal is to
>> disable an entire extension's worth of functions, wouldn't one
>> just.... not load that extension?
>
> Because one would certainly want to re-enable one of these functions in
> one of its vhosts (or even globally in php.ini) :)
>

Is running separate fcgi instances not an option here?

-Sara

Ben RUBSON

9 years ago

> On 10 Feb 2017, at 18:25, Sara Golemon <pollita@php.net> wrote:
>
> On Fri, Feb 10, 2017 at 4:02 AM, Ben RUBSON <ben.rubson@gmail.com> wrote:
>>> On 09 Feb 2017, at 18:12, Sara Golemon <pollita@php.net> wrote:
>>> Could you clarify? `disable_functions` *IS* a PHP_INI_SYSTEM directive:
>>>
>>> PHP_INI_ENTRY("disable_functions", "", PHP_INI_SYSTEM, NULL)
>>
>> However, as per all the bugs referenced by #65386,
>> using "php_admin_value disable_functions" in httpd.conf does not correctly work.
>> The main issue certainly is that it can't be used per vhost.
>> (note that it however still affects their local ini value)
>>
> That's not surprising.  When disable_functions processes an INI
> change, it's a one-way affair.  Any functions listed in the INI
> setting are clobbered and their original handlers are lost (on
> purpose, I imagine).
>
> When a new setting comes in to override it (via php_admin_value) it
> can only add to the list of disabled functions, not restore previously
> disabled functions.

I was talking more specifically about the following :
"php_admin_value disable_functions mydisfunction"
phpinfo() shows mydisfunction as locally disabled,
but in reality can still be used.

So "php_admin_value disable_functions" should not affect "Local Value".
(as per point 1 of #65386)
But should also be solved by allowing "php_admin_value disable_functions"
to really disable functions :)
(as per point 2 of #65386)

> Again, I'm not certain, but my gut says this was intentional.
>
>>> There's also a technical hurdle here due to the way that functions are
>>> (currently) disabled.  It's INI_SYSTEM because enabling/disabling on a
>>> per-request (per vhost essentially means per request) basis means a
>>> lot more heavy lifting than disabling on a system-wide level (we just
>>> replace the function implementation in the global table with a STFU
>>> message).
>>
>> Sounds like it would then require some dev to achieve this behaviour.
>> Would it however hurt requests performance ?
>>
> TL;DR - Yes.
>
> If you want a single process to serve some requests with function X
> disabled and some to serve it with that function enabled, then SOME
> amount of per-request processing has to be done.  Maybe it's done on
> RINIT to shuffle around the function table, maybe it's done at
> function call time via a flag, maybe it's done by compiling to a
> different instruction for "potentially disabled function call".  I'm
> not sure as I haven't thought very far into the problem.

Perhaps initial init time could be a little bit longer, but to the benefit
of non-slowed requests.
OK, understood, thank you !

Any way to "officially" add these requests (#65386) to the development list ?

>>> but if the goal is to
>>> disable an entire extension's worth of functions, wouldn't one
>>> just.... not load that extension?
>>
>> Because one would certainly want to re-enable one of these functions in
>> one of its vhosts (or even globally in php.ini) :)
>>
> Is running separate fcgi instances not an option here?

Of course running separate fcgi instances could be a solution to run
different configurations.
But even here, the wildcards with the ability to use enable_functions
would really be helpful.

Thank you again !

Ben

Sara Golemon

9 years ago

On Fri, Feb 10, 2017 at 4:09 PM, Ben RUBSON <ben.rubson@gmail.com> wrote:

> I was talking more specifically about the following :
> "php_admin_value disable_functions mydisfunction"
> phpinfo() shows mydisfunction as locally disabled,
> but in reality can still be used.
>

Uh... that sounds like a bug.  That should absolutely not be the case
based on the code I'm looking at here.
Unless you're thinking disable_functions should work on userspace
code.  It's only meant for disabling internal/extension functions.

> Perhaps initial init time could be a little bit longer, but to the benefit
> of non-slowed requests.
> OK, understood, thank you !
>

Perhaps.  Again, not dug in enough to be confident we could get where
you want to be without slowing down a hot codepath.

> Any way to "officially" add these requests (#65386) to the development list ?
>

That's what the bug report is.  If someone comes up with a good
solution, I'm sure it'll be RFC'd, but we need a good solution first.

-Sara