[wp-trac] [WordPress Trac] #21251: Media uploads ignore FS_CHMOD_FILE

[WordPress Trac]

#21251: Media uploads ignore FS_CHMOD_FILE
--------------------------+------------------------------
 Reporter:  mikewolf53    |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Upload        |     Version:  3.4.1
 Severity:  normal        |  Resolution:
 Keywords:                |
--------------------------+------------------------------
Changes (by dd32):

 * component:  Media => Upload


Old description:

> = To Reproduce: =
>
> 1.  Have Apache configured such that secure permissions are as follows:
> * 0710 directories
> * 0600 PHP files
> * 0640 All other files (anything that must be read by Apache rather than
> PHP)
>
> 2.  Set permissions on all files and directories as described above.
>
> 3.  Set the following in your wp-config.php
> {{{
> define('FS_CHMOD_DIR', (0710 & ~ umask()));
> define('FS_CHMOD_FILE', (0640 & ~ umask()));
> }}}
>
> 4.  Upload a file using your media library.
>
> 5.  Notice that the uploaded file has permissions of 0600 instead of 640.
>
> = Expected Result =
> Files uploaded should obey the FS_CHMOD_FILE directive, and the uploaded
> file should have permissions of 0640.
>
> = Actual Result =
> Wikipedia sets permissions of the file by taking its parent directory's
> permissions and stripping the executable bits, leaving the file
> unreadable to Apache.  The result is 0600.
>
> = Relevant Info =
>
> These files (and likely more) ignore FS_CHMOD_FILE when uploading files
> to the server:
>
> /wp-includes/functions.php
> {{{
>         // Set correct file permissions
>         $stat = @ stat( dirname( $new_file ) );
>         $perms = $stat['mode'] & 0007777;
>         $perms = $perms & 0000666;
>         @ chmod( $new_file, $perms );
> }}}
>
> /wp-includes/media.php
> {{{
>         // Set correct file permissions
>         $stat = stat( dirname( $destfilename ));
>         $perms = $stat['mode'] & 0000666; //same permissions as parent
> folder, strip off the executable bits
>         @ chmod( $destfilename, $perms );
> }}}
>

> /wp-admin/includes/file.php
> {{{
>         // Set correct file permissions
>         $stat = stat( dirname( $new_file ));
>         $perms = $stat['mode'] & 0000666;
>         @ chmod( $new_file, $perms );
> }}}
>
> This is problematic in the case where suEXEC+fcgid or suPHP are being
> used and Apache has group ownership on files/directories.  In this case,
> the secure permissions would be:
> * 0710 directories
> * 0600 PHP files
> * 0640 All other files (anything that must be read by Apache rather than
> PHP)
>
> The code in each of the files above causes files to be uploaded with
> permissions of 600, which is unreadable by Apache.

New description:

 = To Reproduce: =

 1.  Have Apache configured such that secure permissions are as follows:
 * 0710 directories
 * 0600 PHP files
 * 0640 All other files (anything that must be read by Apache rather than
 PHP)

 2.  Set permissions on all files and directories as described above.

 3.  Set the following in your wp-config.php
 {{{
 define('FS_CHMOD_DIR', (0710 & ~ umask()));
 define('FS_CHMOD_FILE', (0640 & ~ umask()));
 }}}

 4.  Upload a file using your media library.

 5.  Notice that the uploaded file has permissions of 0600 instead of 640.

 = Expected Result =
 Files uploaded should obey the FS_CHMOD_FILE directive, and the uploaded
 file should have permissions of 0640.

 = Actual Result =
 !WordPress sets permissions of the file by taking its parent directory's
 permissions and stripping the executable bits, leaving the file unreadable
 to Apache.  The result is 0600.

 = Relevant Info =

 These files (and likely more) ignore FS_CHMOD_FILE when uploading files to
 the server:

 /wp-includes/functions.php
 {{{
         // Set correct file permissions
         $stat = @ stat( dirname( $new_file ) );
         $perms = $stat['mode'] & 0007777;
         $perms = $perms & 0000666;
         @ chmod( $new_file, $perms );
 }}}

 /wp-includes/media.php
 {{{
         // Set correct file permissions
         $stat = stat( dirname( $destfilename ));
         $perms = $stat['mode'] & 0000666; //same permissions as parent
 folder, strip off the executable bits
         @ chmod( $destfilename, $perms );
 }}}


 /wp-admin/includes/file.php
 {{{
         // Set correct file permissions
         $stat = stat( dirname( $new_file ));
         $perms = $stat['mode'] & 0000666;
         @ chmod( $new_file, $perms );
 }}}

 This is problematic in the case where suEXEC+fcgid or suPHP are being used
 and Apache has group ownership on files/directories.  In this case, the
 secure permissions would be:
 * 0710 directories
 * 0600 PHP files
 * 0640 All other files (anything that must be read by Apache rather than
 PHP)

 The code in each of the files above causes files to be uploaded with
 permissions of 600, which is unreadable by Apache.

--

Comment:

 The `FS_CHMOD_FILE` and `FS_CHMOD_DIR` constants are specifically used by
 the `WP_Filesystem` classes,  and may not always compare directly to what
 the files should be set to when written directly to disk (as they're
 designed with the FTP subsystem in mind, something the Uploads do not
 utilise at all) - As a result, using these same constants for file uploads
 could cause more harm than good.

 For example, Files are set to 644 by default, this would be correct when
 uploaded via FTP (as WP_Filesystem) will be doing, however, when written
 to disk by apache running in an insecure mode (u/g apache/apache for
 example) requires the folder to be 777, and as a result, files as 666).
 I'm not saying it's not wrong to have a secure server, simply showing why
 re-using of those constants is not in the best interest.

 > 0710 directories
 It seems to me, that setting this to 0750 would result in !WordPress
 uploads working correctly (purely as a point of reference for a working
 configuration), and is the scenario that one would expect to find the
 directories configured for (Apache can Read the folder list, as well as
 change into the directory).

 for the less informed on the results of a 710:
 {{{
 7 - Owner - Read/Write/List folder contents/change into
 1 - Groups - Change into (But not list files)
 0 - Everyone - No access
 }}}

 Setting directories to 710 seems overkill to me, although, one which can
 be seen as a "highly secure" (in other words, highly limiting) permission
 level, I would argue that it should be Apache who should be configured to
 not allow directory listings in this case.

 Also, for what it's worth: I'm sure there are filters in the upload
 process which can be used to change the permissions of the created files,
 although, I don't know them off the top of my head (I'd check CDN plugins
 if you can't spot it in the code)

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/21251#comment:1>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software