Live filesystem modified by tests (security)

[orlitzky]

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:

1. The "skip if root" test is flawed, since there are other reasons (than being root) why the test could fail to fail.
2. 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

[orlitzky]

Is there some way to prioritize this? We have a test that can make /etc world-writable.

[orlitzky]

A few more obscure ways this can go wrong:

• On linux, a process with CAP_FOWNER will be able to chmod these files even if it is not running as root
• NFSv4 ACLs have a separate permission for "allowed to change the mode of the file." These are implemented in NFSv4, obviously, but also in the linux kernel RichACLs patchset as write_acl, and on macOS as writesecurity.

[nielsdos]

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]

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 chown files 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 chmod these paths, and then we could change the 0777 to e.g. 0600 to 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 remaining var_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 +i on it (this requires root), then even root himself will not be able to call chmod() on it until chattr -i has 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.

[nielsdos]

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/dir should be prefixed with DIR or something like that...
I'll go over the tests, throw away what's useless and change root paths.