Stop chmodding files and folders

[malthert]

WP's filesystem handler has a chmod function, that is used e.g. when updating,...

To conform with standards, enforce proper usage of umask by the server admin as well as avoid errors when the file owner is not the same as the user running WP, WP should not be chmodding files whatsoever.

Linux, for obvious security reasons, only allows chmod for the owner of the file (independent of permissions, except root).
Thus, it makes sense to have the WP files owned by user A, but run php(-fpm) by user B.

When WP now tries to chmod, which it shouldnt, as we have established that may cause a security issue, it will obviously create a PHP error.

[helen]

I am clearing the version here as I don't think it's new to trunk (5.6 beta), please feel free to change back and comment with more info if it is new so it can be investigated appropriately.

[p00ya]

I'll add another use case for when chmod is bad: some plugins support uploading to cloud storage (instead of filesystem wp-content/uploads) via stream wrappers. Stream wrappers don't necessarily support chmod, see:
​https://www.php.net/manual/en/streamwrapper.stream-metadata.php

While these errors aren't fatal, they do confuse users of these plugins, since they're the most visible things in the logs.

[l3rady]

I too use streawrappers and Chmod is not implemented. A fix would be simply adding error suppression in the form of

@chmod(...

This is done in many areas of the WP codebase but has been missed here. I'm not sure at what version this was introduced but I can see the error back in WordPress 5.3 but don't see it in WP 4.9 so was introduced at some point between them.

[SergeyBiryukov]

Replying to l3rady:

This is done in many areas of the WP codebase but has been missed here. I'm not sure at what version this was introduced but I can see the error back in WordPress 5.3 but don't see it in WP 4.9 so was introduced at some point between them.

It looks like the error suppression for chmod() was intentionally removed in [45611] / #47632 per the coding standards, setting the version accordingly.

Could you share the exact error message? There are at least 10 instances of chmod() in core, so it's not quite clear yet whether the ticket applies to the one in WP_Filesystem_Direct or any others too.

[l3rady]

Replying to SergeyBiryukov:

Could you share the exact error message? There are at least 10 instances of chmod() in core, so it's not quite clear yet whether the ticket applies to the one in WP_Filesystem_Direct or any others too.

ErrorException: chmod(): Aws\S3\StreamWrapper::stream_metadata is not implemented!
#6 includes/file.php(892): handleError
#5 /var/www/*******/releases/20210210123136/vendor/sentry/sentry/lib/Raven/ErrorHandler.php(0): chmod
#4 includes/file.php(892): _wp_handle_upload
#3 includes/file.php(953): wp_handle_upload
#2 includes/media.php(301): media_handle_upload
#1 includes/ajax-actions.php(2542): wp_ajax_upload_attachment
#0 async-upload.php(33): null

In our case, we use AWS S3 stream wrapper and it doesn't support things like chmod. We have worked around the issue by extending Aws\S3\StreamWrapper and adding a stream_metadata function that returns false. However, that only fixes for our use case.

There are many other filesystems out there and not all of them support thinks like chmod, touch and mkdir. Things were fine in WP land as all filesystem calls were error suppressed with @.

It looks like someone has made the decision to not suppress errors as it feels very janky to do so, but when it comes to filesystem calls I think it is perfectly reasonable to do it.

There is still the argument to be made if WordPress should be making chmod calls in the first place as per the OP's argument.

I should have maybe posted my response as a new ticket however both the OP and my input has been triggered by the removal of error suppression.

[costdev]

There appear to be at least three paths forward here:

1. Stop using chmod() altogether.
   • This would rely on server admins implementing umask properly and, I imagine, cause quite a bit of disruption until resolved.
2. Add back error suppression for chmod() calls.
   • This goes against the aim of reducing error suppression as much as possible in Core. It may not be desirable, even though this ticket offers use cases where error suppression would be helpful.
3. A constant, or filter, that allows server admins to disable chmod() calls.
   • The ::chmod() method in the filesystem abstraction classes could check this value, and return true immediately. This would allow if ( $wp_filesystem->chmod() ) {} checks to pass while avoiding the chmod() calls on such systems mentioned earlier in this ticket.
   • A quick search shows there are ~12 instances of chmod() outside of the ::chmod() methods in Core.

Given the above, I'd welcome thoughts on the preferred path forward. Adding dev-feedback to gather thoughts on these as well as additional ideas.

[kkmuffme]

1) is the way to go. As a first step, we could just give a doing_it_wrong for cases where we would run into an error, to force people to get their umask correctly set up.

2) agree, this is a bad idea

3) a filter could be overwritten, so this isn't "save".
A constant would be a possibility, similar to the existing DISALLOW_FILE_MODS
Maybe even use DISALLOW_FILE_MODS?

[azaozz]

Replying to costdev:

Just adding my 2c here.

1. Stop using chmod() altogether.

Would be nice but doesn't seem possible. Can add some error message when WP_DEBUG is true but not doing_it_wrong. That is reserved for developers that are doing something wrong, not for servers that may need better configuration.

2. Add back error suppression for chmod() calls.

This may work combined with 1 above, i.e. depend on WP_DEBUG.

3. A constant, or filter, that allows server admins to disable chmod() calls.
   • The ::chmod() method in the filesystem abstraction classes could check this value, and return true immediately.

Yea, thinking a "short-circuit" filter is probably the best option. It offers more flexibility and a bit more functionality. For example chmod() calls from different places/functions can be handled differently.

• A quick search shows there are ~12 instances of chmod() outside of the ::chmod() methods in Core.

This implies these will be a wp_chmod() function to run the short-circuit filter. Thinking that having more context there would be helpful, so it could be something like:

function wp_chmod( $path, $permissions, $context = '' ) {
    // Do something to validate/verify permissions?
    // Needs to be octal, see https://www.php.net/manual/en/function.chmod.php
    ....

    $changed = apply_filters( 'wp_chmod', false, $path, $permissions, $context );

    if ( false !== $changed ) {
        return true;
    }

    return chmod( $path, $permissions );
}

where $context would be the name of the calling class/function and will be passed to the filter.

Alternatively the last bit can be:

    if ( WP_DEBUG ) {
        return chmod( $path, $permissions );
    } else {
        return @chmod( $path, $permissions );
    }

[kkmuffme]

Atm when you have chmod (and chown) in your php.ini disable_functions directive, you run into errors all the time.

This really needs to be resolved with WP 6.3 at latest.

[costdev]

Thanks for the feedback so far @kkmuffme and @azaozz!

Atm when you have chmod (and chown) in your php.ini disable_functions directive, you run into errors all the time.

In that case, we could just guard calls to these functions with function_exists() checks. Where return chmod() / return chown() are used in Core, we would return true if the functions are disabled.

That way:

• If a server admin wants to disable these functions because of their preferred configuration, Core won't output any errors.
• It removes the need to add a filter or constant for each site on the server.
  • May need a php.ini for each WordPress site on the server if chmod()/chown() are desired elsewhere for some reason.
• It means Core doesn't have to maintain a constant/filter for this purpose.

What do you both think?

---

For reference, uses of chmod() outside of the Filesystem API's ::chmod() methods are:

• ​setup-config.php
• ​_wp_handle_upload()
• ​insert_with_markers()
• ​make_site_theme_from_oldschool()
• ​make_site_theme_from_default() 1
• ​make_site_theme_from_default() 2
• ​WP_Image_Editor_GD::_save()
• ​WP_Image_Editor_Imagick::_save()
• ​wp_mkdir_p()
• ​wp_upload_bits()

[azaozz]

Replying to costdev:

In that case, we could just guard calls to these functions with function_exists() checks.
...we would return true if the functions are disabled.
...
It removes the need to add a filter or constant for each site on the server.

Hmmm, not sure if returning true when chmod() was not run is good? Seems that having a wp_chmod() and a filter would make that more flexible, and perhaps more future-proof. Then plugins would be able to decide what to do when the PHP chmod() is disabled, or if permissions need to be changed at all when chmod() is not disabled. See: https://core.trac.wordpress.org/ticket/51340?replyto=12#comment:6.

For reference, uses of chmod() outside of the Filesystem API's ::chmod() methods are:

Yep, having a WP function will also reduce some code duplication, if nothing else :)

[azaozz]

Thinking some more about this: would it make sense to check the permissions before attempting to set them? Would make setting permissions a bit slower, but would avoid unnecessary changes. May also help in cases where chmod() has been disabled.

Also perhaps instead of checking the exact permissions is_readable() or is_writable() may make more sense in some cases? This can also be in a plugin assuming there is wp_chmod() and a short-circuit filter.

[kkmuffme]

Also perhaps instead of checking the exact permissions is_readable() or is_writable() may make more sense in some cases?

I think that would make sense, otherwise 99% of plugin developers would think there was an error when there was none when chmod is disabled via php.ini.

A function exists check is a must in any way and I think this is something that could be implemented independently of this specific ticket/filters/constants, since it doesn't really change any behavior.

---

I'm not sure if a filter is a good idea though (but - already now there is tons of WP malware, that will even modify the wp-config.php to remove any DISALLOW_FILE_MODS - so it's not far fetched that any filters would just be overwritten by malware with a late priority. Using a constant (that could be set via an auto prepend file in php.ini) would be safer.
However, in the end this again means: the only save way is to disable those functions via php.ini disable_functions

[costdev]

@kkmuffme I see your point regarding malware, however it's likely that errors due to chmod() calls will be the least of the concerns if malware has had that level of access.

@azaozz Hmmm, not sure if returning true when chmod() was not run is good?

In the wp_chmod() suggestion you posted, hooking wp_chmod and returning a non-false value would return true. In most if not all cases, a check would be on if ( ! wp_chmod() ) { return new WP_Error() }, so I think true is an appropriate return value, whether for a short-circuit filter, or if chmod() was disabled.

While a filter would add flexibility for those who need it, I'm mindful that this same flexibility would mean that the purpose of this ticket - Not running chmod() at all - could be overwritten by a callback with a later priority. What are your thoughts on this @azaozz?

is_readable(), is_writable() and function_exists() may be sufficient in some circumstances. However, other cases such as WP_Filesystem_Direct::chmod(), which accepts permissions as an octal integer, won't know what is being requested, and is_readable() wouldn't account for all possible read permissions that might be desired.

---

With the above in mind, I wonder if something like this would be appropriate:

<?php

function wp_chmod( $path, $permissions, $context = '' ) {
    /*
     * If chmod() is not available, assume this has been disabled
     * due to alternative server configuration, such as umask,
     * and that server administrators have configured things correctly.
     *
     * Return true to ensure that boolean checks treat the operation
     * as a success.
     */
    if ( ! function_exists( 'chmod' ) ) {
        return true;
    }

    // 511 = 8^3. PHP treats 0777 as strictly equal to 511.
    if ( ! is_int( $permissions ) || $permissions < 0 || $permissions > 511 ) {
        trigger_error(
            sprintf(
                /* translators: %s: The '$permissions' argument. */
                __( 'The %s argument must be a valid octal integer' ),
                '$permissions'
            )
        );

        return false;
    }

    $changed = apply_filters( 'wp_chmod', false, $path, $permissions, $context );

    if ( false !== $changed ) {
        return true;
    }

    return chmod( $path, $permissions );
}

• If the server admin doesn't want chmod() to run, they can disable chmod() in the disable_functions directive.
• If a plugin wants to have its own management of this, it can hook wp_chmod and act accordingly.
• If nothing is otherwise blocking chmod(), run chmod().

[l3rady]

@costdev this example code works for me where only the UPLOADS dir is the place where it is mapped to a filesystem that doesn't support chmod.

I can use the filter and check the path and if the path is in UPLOADS I can return true and stop chmod from running on a filesystem that doesn't support it while keeping chmod working for all other areas.

[kkmuffme]

Lots of places started using FS_CHMOD_FILE in the meantime, which is a good first start - just need to check there that we won't chmod if file already has correct permissions and also check function_exists.

wp_mkdir_p has the best implementation so far, as it will only chmod if it doesn't match the umask.