Page MenuHomePhabricator
Differential D20205

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

Authored by epriestley on Feb 22 2019, 11:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 20, 3:48 PM
Unknown Object (File)
Sat, Apr 20, 3:31 PM
Unknown Object (File)
Sat, Apr 20, 9:10 AM
Unknown Object (File)
Sat, Apr 20, 5:42 AM
Unknown Object (File)
Wed, Apr 17, 5:11 PM
Unknown Object (File)
Wed, Apr 10, 1:43 PM
Unknown Object (File)
Tue, Apr 9, 9:26 PM
Unknown Object (File)
Fri, Apr 5, 5:02 AM
View All Files
Subscribers
None

Details

Reviewers
amckinley
Maniphest Tasks
T10853: Make `setCWD('/does/not/exist')` safer on ExecFuture
Commits
rPHU0679917944e1: Refine checks against "cwd" before "proc_open()"
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
Lint Not Applicable
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!
epriestley added a comment.Feb 22 2019, 11:47 PM

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. ^^^

epriestley updated this revision to Diff 48241.Feb 22 2019, 11:50 PM
  • 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!
epriestley updated this revision to Diff 48242.Feb 22 2019, 11:51 PM
  • Remove local code that made "arc diff" change the working copy's origin URL.
Harbormaster completed remote builds in B22099: Diff 48242.Feb 22 2019, 11:52 PM
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:

Screen Shot 2019-02-22 at 7.38.34 PM.png (530×982 px, 41 KB)

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
epriestley added a comment.Feb 23 2019, 1:17 PM

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.

epriestley added a comment.Feb 25 2019, 7:33 PM

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.
Harbormaster completed remote builds in B22120: Diff 48258.Feb 25 2019, 8:04 PM
Closed by commit rPHU0679917944e1: Refine checks against "cwd" before "proc_open()" (authored by epriestley). · Explain WhyFeb 25 2019, 8:14 PM
This revision was automatically updated to reflect the committed changes.
epriestley mentioned this in T13565: Filesystem write failure error messages lack tailoring, notably in DeferredLog.Aug 11 2020, 6:04 PM

Revision Contents
Changeset List

PathSize
src/
filesystem/
43 lines
future/
exec/
44 lines

Diff 48259

View Options

src/filesystem/Filesystem.php

Loading...
View Options

src/future/exec/PhutilExecutableFuture.php

Loading...
Phabricator · Built by Phacility