[wp-trac] [WordPress Trac] #48316: Changeset 46482 breaks upload when using ".." in upload_path.

[WordPress Trac]

#48316: Changeset 46482 breaks upload when using ".." in upload_path.
----------------------------+------------------------------
 Reporter:  xpoon           |       Owner:  (none)
     Type:  defect (bug)    |      Status:  reopened
 Priority:  normal          |   Milestone:  Awaiting Review
Component:  Filesystem API  |     Version:  5.2.4
 Severity:  normal          |  Resolution:
 Keywords:                  |     Focuses:
----------------------------+------------------------------
Changes (by peterwilsoncc):

 * severity:  major => normal


Comment:

 It's worth restating, if uploads directory is a sub-directory of
 `WP_CONTENT_DIR` or `ABSPATH` the ideal solution is to remove any path
 traversal from constants.

 If the `UPLOADS` directory truly needs to be outside of `ABSPATH`, then
 symlinks and redirects will be helpful too. It's an approach I've used in
 the path successfully.

 While valid, defining the uploads path in this manner is not a common
 configuration of WordPress, it's for advanced users. I've reduced the
 severity of the bug due to the advanced usage and availability of multiple
 work-arounds.

 @mpcube all this said, thank you very much for your efforts and helping a
 middle-ground be found to allow for advanced usage while considering path
 traversal should generally be avoided. I very much appreciate. it.

 ---

 Before this approach can be considered, I'm still seeking further feedback
 on whether it is safe from members of the security team.

 Presuming it's safe, the patch will need a little more work:

 * `realpath( /* UPLOAD directory */ )` will return false if the uploads
 directory doesn't exist. This is a fairly common situation on fresh
 installs as the call `mkdir_p( 'path/to/uploads/yyyy/mm' )` will create
 the directory.
 * The patch would need to check for `false === realpath()` before doing
 anything with the return value, otherwise WP could end up modifying/adding
 directories to the file systems root directory. This would be a downer ;)
 * Multisite defines `UPLOADS` in the function `ms_upload_constants()` so
 any code needs to ensure that value is trusted too
 * Unit tests would be very helpful too, need to figure out what they look
 like

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/48316#comment:28>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform