Create the batch temp directory privately and fail if it exists - #127
Merged
Conversation
The batch scan built its temp root from uniqid(), ignored the mkdir() return value, and created every directory with the default 0777 mask. uniqid() is derived from the current microtime, so the path is guessable. On a shared machine another local user could pre-create the predicted directory with 'new' and 'old' as symlinks; because the failed mkdir() was ignored, the batch would then write git or svn file contents through those symlinks, or tamper with the copies between creation and the single phpcs run that gates a commit. Even with no attacker present, the 0777 directories exposed copies of the scanned source to other local users for the duration of the scan. Name the directory from random_bytes() instead, create it and its nested directories with mode 0700, and treat a failed mkdir() as fatal rather than continuing into a directory we do not own. Note that the temp files themselves are still created by shell redirection and so land at the umask default; the 0700 parent is what keeps them private. Fixes #120
assertMatchesRegularExpression() was added in PHPUnit 9.1, but the test matrix runs PHP 7.2 on PHPUnit 8.5, where it does not exist and the test errored. Assert on preg_match() directly rather than switching to assertRegExp(): that works on every PHPUnit version composer.json allows (^6.4 through ^9.5) and does not depend on an assertion PHPUnit 10 removes.
Windows ignores mkdir()'s mode argument and reports 0777 for every directory, since access there is governed by inherited ACLs rather than POSIX permission bits, so the assertion failed on the Windows matrix. Skip that test on Windows using the same PHP_OS check UnixShellTest already uses. The 0700 mode is what protects the scanned files' temp copies on the multi-user Unix machines the issue is about, so the assertion is still worth making where it applies. The temp directory naming test is platform independent and keeps running everywhere.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #120.
The batch scan built its temp root from
uniqid(), ignored themkdir()return value, and created every directory with the default0777 & ~umask:uniqid()is derived from the current microtime, so the path is guessable. On a shared machine (CI runner, shared build box) another local user can pre-create the predicted directory withnewandoldas symlinks; because the failedmkdir()was ignored, the batch then writes git or svn file contents through those symlinks, or tampers with the copies between creation and the single phpcs run that gates a commit. Even with no attacker present, the0777directories exposed copies of the scanned source to other local users for the duration of the scan. CWE-377 / CWE-378.Changes
Both batch entry points now share a
createTempDir()helper that names the directory fromrandom_bytes(16), creates it with mode0700, and throws aShellExceptionifmkdir()fails rather than continuing into a directory it does not own.writeTempFile()'s nestedmkdir($dir, 0777, true)is likewise0700and checked.The temp files themselves are still created by shell redirection and so land at the umask default; the
0700parent is what keeps them private.Testing
Two tests in
GitWorkflowTest, driven through the existing mocked-shell batch path:testFullGitWorkflowCreatesBatchTempDirsPrivateToTheCurrentUser— asserts every directory in the batch tree is0700.testFullGitWorkflowNamesTheBatchTempDirUnpredictably— asserts the root matchesphpcs-changed-[0-9a-f]{32}.The batch deletes the whole tree in a
finallybefore returning, so the directories cannot be inspected after the fact.TestShell/WindowsTestShellnow record each temp directory's permissions aswriteCommandOutputToFileis called, which is the point where the tree exists. Both tests fail on trunk and pass with this change.composer precommitpasses: 165 tests / 270 assertions, phpcs clean, psalm no errors.Follow-up, not fixed here
Temp paths are built as
$tempDir . '/new/' . ltrim($fileName, '/'), and file names reach that point exactly as typed on the command line — nothing callsrealpath()on them. A relative argument containing..therefore places the temp copy outside the batch directory:which resolves to
/tmp/evil.php— outside the treecleanupTempDir()removes, and able to clobber an existing file there. It is not a privilege boundary, since the user is scanning their own files as themselves, but it writes and leaks files outside the temp dir. Filed separately.