WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#13659 new defect (bug)

Incorrect permissions created by wp_mkdir_p() when safe mode is used

Reported by: cgrenier Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9.2
Component: Upload Keywords: reporter-feedback
Focuses: Cc:

Description

When safe mode and sgid directories are used, the function wp_mkdir_p() frim wp-includes/functions.php will failed to create to create a directory with correct permissions.

How to test the problem on wp_mkdir_p() function:

  • Create a directory, set the permission to 02777
  • Put the file named poc.php in this directory
  • safe_mode is on, safe_mode_gid is on
  • php (apache) is run by a user other than the file owner
  • call the poc.php script via an url

Result:
Sub-directory /1/2/3/4/ hasn't been created due to safe mode restriction.

Explanation:
Any file or directory created in a sgid directory inherits the group owner. When safe mode is used, chmod() cannot set the SUID, SGID and sticky bits and the chmod() calls will remove the expected permission.

Patchs:
There is a least two possibilities:

  • Don't call chmod at all (Need to check if non Unix system, Windows, is affected).
  • Don't call chmod if safe_mode is enable and sgid permission is present

Attachments (3)

poc.php (1.2 KB) - added by cgrenier 4 years ago.
wp_mkdir_p_p1.patch (538 bytes) - added by cgrenier 4 years ago.
Don't use chmod
wp_mkdir_p_p2.patch (634 bytes) - added by cgrenier 4 years ago.
Only call chmod if safe mode isn't used or sgid bit isn't set

Download all attachments as: .zip

Change History (9)

cgrenier4 years ago

cgrenier4 years ago

Don't use chmod

cgrenier4 years ago

Only call chmod if safe mode isn't used or sgid bit isn't set

comment:1 nacin4 years ago

For what it's worth, I'm pretty sure wp_mkdir_p() should be deprecated in favor of the WP_Filesystem API.

comment:2 follow-up: dd324 years ago

  • Component changed from General to Upload
  • Milestone changed from Unassigned to Future Release
  • Version set to 2.9.2

For what it's worth, I'm pretty sure wp_mkdir_p() should be deprecated in favor of the WP_Filesystem API.

Unfortunately thats impossible. (safe mode should be deprecated instead :P, which luckely, it is)

WordPress uploads write directly to the filesystem, asking for username/password every time would be over the top for simply uploading files.

This sounds like a PHP bug unfortunately, cgrenier, What versions of PHP are affected by this? Calling chmod shouldnt cause all permissions to be lost in the event that a security layer prevents it.. Shouldnt.

Setting the component to Upload, due to that being where this function is primarily used.

comment:3 in reply to: ↑ 2 cgrenier4 years ago

Replying to dd32:

For what it's worth, I'm pretty sure wp_mkdir_p() should be deprecated in favor of the WP_Filesystem API.

Unfortunately thats impossible. (safe mode should be deprecated instead :P, which luckely, it is)

WordPress uploads write directly to the filesystem, asking for username/password every time would be over the top for simply uploading files.

This sounds like a PHP bug unfortunately, cgrenier, What versions of PHP are affected by this? Calling chmod shouldnt cause all permissions to be lost in the event that a security layer prevents it.. Shouldnt.

Setting the component to Upload, due to that being where this function is primarily used.

It's not a bug in PHP but a documented function.

Extract from http://php.net/manual/en/function.chmod.php
"When safe mode is enabled, PHP checks whether the files or directories you are about to operate on have the same UID (owner) as the script that is being executed. In addition, you cannot set the SUID, SGID and sticky bits."

Why wordpress tries to reapplies the existing permission ?

comment:4 momo360modena4 years ago

  • Cc momo360modena added

comment:5 hakre4 years ago

Maybe there is some chmod(0) calling because of edge cases, like stat() not working due to safemode restrictions. I have a patch here that could probably deal deal with those cases: 14928.3.patch

Related: #14928

comment:6 hakre4 years ago

  • Keywords reporter-feedback added
Note: See TracTickets for help on using tickets.