chmod breaks ACLs

[orlitzky]

I just tried to enable this on one of our sites (I'm shopping around for a cache plugin) and it broke our ACLs. I mean this only in the most constructive way: it looks like you're using chmod() a little naively. There are several issues with blindly running, say, chmod 755.

First, it assumes that the permissions need to be adjusted at all. This is generally not the case. The user/administrator has several ways to ensure that the permissions are set correctly without each program having to manually fiddle with them: umask, setgid, posix ACLs, NFSv4 ACLs, etc. Generally you should aim only to reduce the default permission set, and only on sensitive files. For example, adding the world-readable bit to the existing umask if you're about to create a private file is OK, but setting the group bits to zero is not (it breaks both types of ACLs).

Second, it assumes that you know what the correct permissions should be. If I've set my umask such that directories get created with mode 750 for whatever reason, then the plugin running chmod 755 is wrong, and can create security vulnerabilities. Similarly, setting the group bits is only a sensible operation if you know what my groups mean, and there's no way for you to know that. If all of my vhosts share a group and everything is group-unreadable by default, the plugin changing that 0 to a 5 is asking for trouble.

Finally, it assumes that mode bits are the only method of access control in place. This is the one that bit me -- we use POSIX ACLs on all of our sites. There are several other access control mechanisms like POSIX ACLs (linux mainly) and NFSv4 ACLS (linux/mac) that could be used. Messing with the permission bits affects these in unpredictable ways. In my case, changing the group bits to 5 invalidates all of the ACLs that grant write access to our web developers.

It looks like the introduction of chmod() was relatively recent, and the result of a feature request? If so, I would suggest going back to see if there isn't an easier and more-standard way to address that request. For example, umask is nice if you're the admin, or there's Wordpress's own FS_CHMOD_DIR and FS_CHMOD_FILE if your hosting company handicaps you. But it depends on the nature of the problem. For https://github.com/keycdn/cache-enabler/pull/194, I think it would have sufficed to simply bypass wp_mkdir_p(), creating the directories yourself and letting the default permissions (i.e. not those of wp-content) be used.

Anyway, thanks for listening to my drive-by feedback :)

[orlitzky]

I've settled on cache-enabler as our cache plugin and have started to roll out a patched version. FWIW, the following is what I'm using to fix the ACL issue:

diff --git a/inc/cache_enabler_disk.class.php b/inc/cache_enabler_disk.class.php
index 38fa38d..62817f5 100644
--- a/inc/cache_enabler_disk.class.php
+++ b/inc/cache_enabler_disk.class.php
@@ -354,13 +354,6 @@ final class Cache_Enabler_Disk {
         $new_cache_file_created = file_put_contents( $new_cache_file, $page_contents, LOCK_EX );

         if ( $new_cache_file_created !== false ) {
-            clearstatcache();
-            $new_cache_file_stats = @stat( $new_cache_file_dir );
-            $new_cache_file_perms = $new_cache_file_stats['mode'] & 0007777;
-            $new_cache_file_perms = $new_cache_file_perms & 0000666;
-            @chmod( $new_cache_file, $new_cache_file_perms );
-            clearstatcache();
-
             $page_created_url = self::get_cache_url( $new_cache_file_dir );
             $page_created_id  = url_to_postid( $page_created_url );
             $cache_created_index[ $new_cache_file_dir ]['url'] = $page_created_url;
@@ -1315,14 +1308,6 @@ final class Cache_Enabler_Disk {
             return false;
         }

-        if ( $fs->getchmod( $parent_dir ) !== $mode_string ) {
-            return $fs->chmod( $parent_dir, $mode_octal, true );
-        }
-
-        if ( $fs->getchmod( $dir ) !== $mode_string ) {
-            return $fs->chmod( $dir, $mode_octal );
-        }
-
         return true;
     }