Skip to content

Add a new requirement function to disable dangerous functions#23141

Open
orthagh wants to merge 7 commits intoglpi-project:11.0/bugfixesfrom
orthagh:feat/disablefunction
Open

Add a new requirement function to disable dangerous functions#23141
orthagh wants to merge 7 commits intoglpi-project:11.0/bugfixesfrom
orthagh:feat/disablefunction

Conversation

@orthagh
Copy link
Contributor

@orthagh orthagh commented Feb 17, 2026

If someone has an idea to test the thing, please suggest it.
(as the thing can only be enabled in php.ini)

Copilot AI review requested due to automatic review settings February 17, 2026 10:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new security requirement class DangerousFunctionsSecurity that checks whether potentially dangerous PHP functions are enabled. The requirement is integrated into both the system requirements manager and the Central dashboard to warn administrators about security risks.

Changes:

  • Added DangerousFunctionsSecurity requirement class that checks for 33 potentially dangerous PHP functions
  • Integrated the new requirement into RequirementsManager for system checks
  • Integrated the new requirement into Central dashboard to display security warnings

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/Glpi/System/Requirement/DangerousFunctionsSecurity.php New requirement class that validates dangerous PHP functions are disabled
src/Glpi/System/RequirementsManager.php Adds DangerousFunctionsSecurity to optional requirements list
src/Central.php Adds DangerousFunctionsSecurity to security requirements shown in Central dashboard

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good:

Image

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some posix_ functions can be dangerous too.

@orthagh orthagh force-pushed the feat/disablefunction branch from f3fa830 to e4c7fb6 Compare February 17, 2026 14:36
@orthagh
Copy link
Contributor Author

orthagh commented Feb 17, 2026

Some posix_ functions can be dangerous too.

List updated from wordpress recommendations (from a tier party website
Removed pcntl_async_signals, pcntl_signal, pcntl_signal_get_handler, pcntl_signal_dispatch as asked in glpi-project/docker-images#275

@AdrienClairembault
Copy link
Contributor

@orthagh
simpler message and enable check for cli

Is it right to enable it for CLI? Some configurations might have a different .ini config for CLI and web.

I think our own docker image allow these methods on the CLI but not on the web (at least this is what I heard last week, unsure if it was merged that way).

@orthagh
Copy link
Contributor Author

orthagh commented Feb 17, 2026

Is it right to enable it for CLI? Some configurations might have a different .ini config for CLI and web.

Asked by @cedric-anne in #23141 (comment)

Comment on lines +46 to +76
'pcntl_alarm',
'pcntl_fork',
'pcntl_waitpid',
'pcntl_wait',
'pcntl_wifexited',
'pcntl_wifstopped',
'pcntl_wifsignaled',
'pcntl_wifsignaled',
'pcntl_wifcontinued',
'pcntl_wexitstatus',
'pcntl_wtermsig',
'pcntl_wstopsig',
'pcntl_get_last_error',
'pcntl_strerror',
'pcntl_sigprocmask',
'pcntl_sigwaitinfo',
'pcntl_sigtimedwait',
'pcntl_exec',
'pcntl_getpriority',
'pcntl_setpriority',
'posix_ctermid',
'posix_getcwd',
'posix_getegid',
'posix_getgid',
'posix_getgrgid',
'posix_getgrnam',
'posix_getgroups',
'posix_getlogin',
'posix_getpgid',
'posix_getpgrp',
'posix_getpid',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could rely on get_defined_functions() + preg_grep() to get all pcntl/posix functions. It could permit to be sure that no function is forgotten and any new function introduced in the future would be detected.

}
}
$this->validation_messages[] = sprintf(
__('Functions "%s" are enabled. Please disable them in php.ini (see disable_functions directive) to avoid security risks.'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be disabled either from the php.ini directive or directly in the webserver configuration.

Suggested change
__('Functions "%s" are enabled. Please disable them in php.ini (see disable_functions directive) to avoid security risks.'),
__('Functions "%s" are enabled. You should disable them (see `disable_functions` PHP configuration directive) to avoid security risks.'),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one. It's clearer for people that don't know how to do it and for other we probably don't have to explain

@AdrienClairembault
Copy link
Contributor

Is it right to enable it for CLI? Some configurations might have a different .ini config for CLI and web.

Asked by @cedric-anne in #23141 (comment)

I know, just saying that our own docker image might not validate this requirement.

Co-authored-by: Cédric Anne <cedric.anne@gmail.com>
@cedric-anne
Copy link
Member

Is it right to enable it for CLI? Some configurations might have a different .ini config for CLI and web.

Asked by @cedric-anne in #23141 (comment)

I know, just saying that our own docker image might not validate this requirement.

It is not a problem if our development image does not fit this new requirement in CLI context, as it is optional. Our production docker image will be updated to fit them.

@cedric-anne
Copy link
Member

proc_open is used when the PHP method is used to send mail. IMHO, We should warn users that disabling proc_open can prevent the PHP native mail function to work and recommand to disable proc_open and switch to the SMTP method.

@trasher
Copy link
Contributor

trasher commented Feb 18, 2026

Should we really consider sending mail from sendmail ins unsafe?

@cedric-anne
Copy link
Member

Should we really consider sending mail from sendmail ins unsafe?

The problem is that it prevents to disable the proc_open function. It means that this function can be used to call any system executable. Thus, it makes it a bit useless to disable exec, shell_exec, paththru, ...

Comment on lines +94 to +102
'socket_accept',
'socket_bind',
'socket_clear_error',
'socket_close',
'socket_connect',
'socket_listen',
'socket_create_listen',
'socket_read',
'socket_create_pair',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should block them.

Socket could be use for example by plugin to interact with a realtime tool.

@froozeify
Copy link
Member

A new PHPStan rule has been created based on the configuration here.

I've run it locally and our codebase (for glpi 11.x) don't have call to those forbidden tools

glpi-project/phpstan-glpi#16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants