Page MenuHomePhabricator

Refine checks against "cwd" before "proc_open()"
ClosedPublic

Authored by epriestley on Feb 22 2019, 11:40 PM.

Details

Summary

See PHI1094. Previously, see D15785. Fixes T10853. Upstream: https://bugs.php.net/bug.php?id=77656.

PHP allows you to proc_open() into any CWD, including one that does not exist, is not readable, and/or is not executable.

We attempt to catch this, but don't do enough checks, and allow you to attempt execution into a directory which exists but which you can't read or execute.

Add more checks to prevent this. Also, try to make assertExists() more clever about cases where a path really doesn't exist versus cases where we aren't allowed to tell if a path exists or not because of a permissions issue.

(It's technically legal to chdir() into a directory you only have +x on, although this is probably ill-advised.)

Test Plan

Used this script with various arguments:

<?php

require_once '/Users/epriestley/dev/phabricator/scripts/init/init-script.php';

$result = id(new ExecFuture('pwd && ls -alh'))
  ->setCWD($argv[1])
  ->resolvex();

var_dump($result);

In all cases, the directory name describes the permissions. subdir/ exists and is a directory we have "+r" and "+x" on.

epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable
[2019-02-22 15:34:14] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-readable", but that directory is not readable (the current process does not have "+r" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:156]
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable/subdir
array(2) {
  [0]=>
  string(163) "/Users/epriestley/scratch/not-readable/subdir
total 0
drwxr-xr-x  2 epriestley  staff    64B Feb 22 15:33 .
drwxr-x--x  3 root        wheel    96B Feb 22 15:33 ..
"
  [1]=>
  string(0) ""
}
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-executable
[2019-02-22 15:34:24] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-executable", but that directory is not executable (the current process does not have "+x" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:165]
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-executable/subdir
[2019-02-22 15:34:26] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/not-executable/subdir". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/not-executable/subdir" can not be accessed because a parent directory ("/Users/epriestley/scratch/not-executable") is not executable (the current process does not have "+x" permission). at [<phutil>/src/filesystem/Filesystem.php:1142]
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable-or-executable/
[2019-02-22 15:34:32] EXCEPTION: (Exception) Preparing to run a command in directory "/Users/epriestley/scratch/not-readable-or-executable/", but that directory is not readable (the current process does not have "+r" permission). at [<phutil>/src/future/exec/PhutilExecutableFuture.php:156]
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/not-readable-or-executable/subdir
[2019-02-22 15:34:33] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/not-readable-or-executable/subdir". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/not-readable-or-executable/subdir" can not be accessed because a parent directory ("/Users/epriestley/scratch/not-readable-or-executable") is not executable (the current process does not have "+x" permission). at [<phutil>/src/filesystem/Filesystem.php:1142]
epriestley@orbital ~/scratch $ php -f pwd2.php /Users/epriestley/scratch/does-not-exist
[2019-02-22 15:34:37] EXCEPTION: (PhutilProxyException) Unable to run a command in directory "/Users/epriestley/scratch/does-not-exist". {>} (FilesystemException) Filesystem path "/Users/epriestley/scratch/does-not-exist" does not exist. at [<phutil>/src/filesystem/Filesystem.php:1153]

Diff Detail

Repository
rPHU libphutil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 22 2019, 11:40 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 22 2019, 11:40 PM
Harbormaster failed remote builds in B22097: Diff 48240!

Builds are failing in a curious way so this probably breaks something, let me see if I can figure out what..

src/filesystem/Filesystem.php
1145

Actual example of ">>" working properly, above. ^^^

  • Set my "origin" back to the right value after testing git remote set-url commands with random values.
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 22 2019, 11:50 PM
Harbormaster failed remote builds in B22098: Diff 48241!
  • Remove local code that made "arc diff" change the working copy's origin URL.
epriestley requested review of this revision.Feb 22 2019, 11:52 PM
amckinley accepted this revision.Feb 23 2019, 2:39 AM

o_o

Also this:

This revision is now accepted and ready to land.Feb 23 2019, 2:39 AM
epriestley edited the summary of this revision. (Show Details)Feb 23 2019, 2:43 AM

I'm going to cut the release before trying to unleash this, and I also want to make sure is_executable($dir) works in a similar way on Windows.

It's possible the behavior is, say, "always return false", in which case I'll add if (windows) { do nothing } else { check execute } sorts of guards.

On Windows, I believe the call chain is is_executable(...)VCWD_ACCESS(..., X_OK)tsrm_win32_access(..., X_OK)php_win32_ioutil_access_w(..., X_OK)GetBinaryTypeW(...).

This method tests if a path is a .exe: https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getbinarytypew

This suggests is_executable(...) will always return false for directories on Windows. As far as I can tell, this is also the actual behavior.

I'm going to skip the is_executable(..) checks under Windows for now since they definitely don't work as written (as far as I can tell, they always fail). If we discover there's some secret alternative magic in the future, we can specialize things further.

epriestley updated this revision to Diff 48258.Feb 25 2019, 8:04 PM
  • On Windows, don't do "+x" checks against directories because these checks seem to always fail and not be meaningful anyway.
This revision was automatically updated to reflect the committed changes.