NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Process] Decouple Process::findExecutable() from open_basedir#54151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Luc45 wants to merge2 commits intosymfony:5.4fromLuc45:24-03/find-executable-open_basedir

Closed

[Process] Decouple Process::findExecutable() from open_basedir#54151

Luc45 wants to merge2 commits intosymfony:5.4fromLuc45:24-03/find-executable-open_basedir

Conversation

Copy link

Contributor

Luc45 commentedMar 4, 2024

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#41006
License	MIT

This PR ports the PR#47422 to Symfony Process 5.4

Port PRsymfony#47422to Symfony Process 5.4

f0e9c2c

carsonbot added Status: Needs Review Bug labels

Mar 4, 2024

carsonbot added this to the5.4 milestone

Mar 4, 2024

Patching file

c630308

Copy link

ContributorAuthor

Luc45 commentedMar 4, 2024

This PR has a failing test because we only run SIGCHILD tests on PHP 7.2.

I wonder if this is specific to 7.2, or if we would see the same failures if we ran it on PHP 8+, too...?

Copy link

Member

#47422 was merged as a feature into 6.4 which was still under development at that time. If you want us to reclassify this change as a bugfix, I think you need to elaborate on that. Otherwise, this PR will be closed soon because we don't backport features to LTS branches.

derrabus added Process Status: Waiting feedback and removed Status: Needs Review labels

Mar 5, 2024

carsonbot changed the title~~Port PR #47422 to Symfony Process 5.4~~[Process] Port PR #47422 to Symfony Process 5.4

Mar 5, 2024

Copy link

ContributorAuthor

Luc45 commentedMar 5, 2024

Got it. I believe this is a bug.

$finder =new \Symfony\Component\Process\ExecutableFinder();$finder->find('docker');// /usr/bin/dockerexec('which docker')// /user/bin/dockerini_set('open_basedir','/tmp/foo');$finder->find('docker');// nullexec('which docker')// /user/bin/docker

Copy link

ContributorAuthor

Luc45 commentedMar 5, 2024

Actually, this behavior is present on Process ^5, ^6, and ^7. PR#47422 doesn't solve it as I expected.

php foo.php                                                                                                                                                                                                          [24/03/5| 5:45]string(15) "/usr/bin/docker"string(15) "/usr/bin/docker"NULLstring(15) "/usr/bin/docker"

Test file:

<?phprequire__DIR__ .'/vendor/autoload.php';$finder =new \Symfony\Component\Process\ExecutableFinder();var_dump($finder->find('docker'));var_dump(exec('which docker'));ini_set('open_basedir','/tmp/foo');var_dump($finder->find('docker'));var_dump(exec('which docker'));

Copy link

ContributorAuthor

Luc45 commentedMar 5, 2024•
edited
Loading

It seems thatis_executable('/usr/bin/docker') is affected byopen_basedir restrictions and returns false, even thoughexec('/usr/bin/docker --version') works with the sameopen_basedir restrictions:

<?phprequire__DIR__ .'/vendor/autoload.php';$finder =new \Symfony\Component\Process\ExecutableFinder();$finder->find('docker');// /usr/bin/dockerexec('which docker');// /user/bin/dockeris_executable('/usr/bin/docker');// trueexec('/usr/bin/docker --version');// Docker version 25.0.2, build 29cf629222ini_set('open_basedir','/tmp/foo');$finder->find('docker');// nullexec('which docker');// /user/bin/dockeris_executable('/usr/bin/docker');// falseexec('/usr/bin/docker --version');// Docker version 25.0.2, build 29cf629222

ExecutableFinderusesis_executable, so it's affected by this. It seems like a PHP bug that should be reported upstream, but also ideally fixed and backported on Process if possible, do you agree?

Copy link

ContributorAuthor

Luc45 commentedMar 6, 2024•
edited
Loading

Actually, it's not thatis_executable is affected byopen_basedir when it shouldn't - it's thatexec isn't, and I would bet thatproc_open isn't, neither.

<?phprequire__DIR__ .'/vendor/autoload.php';$finder =new \Symfony\Component\Process\ExecutableFinder();exec('mkdir -p /tmp/bar && rm -rf /tmp/bar/*.test');touch('/tmp/bar/no-basedir-touch.test');// Created.exec('touch /tmp/bar/no-basedir-exec.test');// Created.ini_set('open_basedir','/tmp/foo');touch('/tmp/bar/touch-basedir.test');// Not created.exec('touch /tmp/bar/touch-basedir-exec.test');// Created.

Result:

➜  bar ls -Al                                                                                                                                                                                                                                                                                              -rw-r--r-- 1 lucas lucas 0 mar  6 11:38 no-basedir-exec.test-rw-r--r-- 1 lucas lucas 0 mar  6 11:38 no-basedir-touch.test-rw-r--r-- 1 lucas lucas 0 mar  6 11:38 touch-basedir-exec.test

So the bug, after all, is usingis_executable, which is bound byopen_basedir to invoke code that isn't bound byopen_basedir, such asexec andproc_open. Although, "fixing" this could also be an unwanted security issue for existing code.