Repository navigation
Live filesystem modified by tests (security) #11808
Description
Description
I don't remember where I reported this, but I haven't heard anything about it since November of 2022, so here it is again: https://bugs.gentoo.org/839894
Several filesystem tests check for root privileges with,
$ cat ext/standard/tests/skipif_root.inc
<?php
// Skip if being run by root (files are always readable, writeable and executable)
$filename = @tempnam(__DIR__, 'root_check_');
if (!file_exists($filename)) {
die('WARN Unable to create the "root check" file');
}
$isRoot = fileowner($filename) == 0;
unlink($filename);
if ($isRoot) {
die('SKIP Cannot be run as root');
}and proceed to modify the live filesystem if the user is not root. Those modifications are expected to fail. For example,
$ cat ext/standard/tests/file/006_error.phpt
--TEST--
Test fileperms(), chmod() functions: error conditions
--SKIPIF--
<?php
if (substr(PHP_OS, 0, 3) == 'WIN') {
die('skip Not on Windows');
}
require __DIR__ . '/../skipif_root.inc';
?>
--FILE--
<?php
echo "*** Testing error conditions for fileperms(), chmod() ***\n";
/* With standard files and dirs */
var_dump( chmod("/etc/passwd", 0777) );
printf("%o", fileperms("/etc/passwd") );
echo "\n";
clearstatcache();
...
This test tries to make /etc/passwd world-writable, but it will be skipped if you are uid 0. Well, not only uid 0 can modify /etc/passwd. if there's an admins group, for example, its members may have uid 1000+ and still be able to add new users by modifying /etc/passwd. The user reporting the Gentoo bug is also able to write those files, and is not root (for some other reason). In cases like those, this test creates a security issue: the test will be run because the user is not root, and /etc/passwd will be made world-writable; afterwards, anyone can edit it.
There are two problems here:
- The "skip if root" test is flawed, since there are other reasons (than being root) why the test could fail to fail.
- If we're going to break the system in the event that the test is somehow not skipped, we should break it to be overly secure, and not less secure.
PHP Version
git HEAD
Operating System
No response
Activity
orlitzky commentedon Feb 27, 2024
Is there some way to prioritize this? We have a test that can make
/etcworld-writable.orlitzky commentedon Feb 27, 2024
A few more obscure ways this can go wrong:
CAP_FOWNERwill be able to chmod these files even if it is not running as rootwrite_acl, and on macOS aswritesecurity.ndossche commentedon Feb 28, 2024
I agree these tests are dangerous. I haven't dug into it deep enough to understand how to replace them with safe alternatives. Do you have suggestions?
orlitzky commentedon Feb 28, 2024
I can't think of a way to make it completely safe in a portable way. Ideally we'd like to mess with a file that we create ourselves, but we'll own any file we create, and non-root users can't
chownfiles away to another user. But, as the file's owner,chmod()will succeed, defeating the purpose of the test.We could try to check a long list of conditions in which the user might actually be able to
chmodthese paths, and then we could change the0777to e.g.0600to make them safer if we miss one, but honestly, I don't think these two tests add enough value to go to the trouble. If I were king of the world I would just delete the two that try to modify real paths. The remainingvar_dump( chmod("/no/such/file/dir", 0777) )could be improved a little by adding some random junk to the path, juuust in case.A potential consolation prize would be to run the test only on CI, and only as root. If you create a new file and run
chattr +ion it (this requires root), then even root himself will not be able to callchmod()on it untilchattr -ihas been used to clear the immutable bit. This isn't even a little bit portable, but if it only runs on the CI, it might be OK.ndossche commentedon Mar 1, 2024
I agree these tests on the root filesystem are not great. Also, what does that actually test anyway, checking if chmod works is already done in other tests... We just want to test the error path which can also be reached by trying to chmod a non-existent file. And yeah,
/no/such/file/dirshould be prefixed with DIR or something like that...I'll go over the tests, throw away what's useless and change root paths.
Fix phpGH-11808: Live filesystem modified by tests (security)
Fix GH-11808: Live filesystem modified by tests (security)
Merge branch 'PHP-8.2' into PHP-8.3
Merge branch 'PHP-8.3'