WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 7 years ago

#20069 closed defect (bug) (fixed)

Wordpress upgrade process removed executable bits on .php files and so broke Wordpress

Reported by: gerv Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: blocker Version: 3.3.1
Component: Filesystem API Keywords: has-patch needs-testing
Focuses: Cc:

Description

I just upgraded from 3.2 to 3.3.1 on my shared hosting box (hosted by http://www.mythic-beasts.com). This uses suexec, and so .php scripts need to have executable permissions otherwise it won't execute them. I am told that many secure multi-user setups work like this.

My original install had the PHP scripts set as executable, but I did the upgrade (via the web GUI) and suddenly I got 500 Server Errors everywhere. I had to chmod all .php files back to executable before my blog would work again.

Wordpress should check the permissions on .php files before upgrading and preserve them across the upgrade.

Gerv

Attachments (2)

20069.diff (589 bytes) - added by dd32 7 years ago.
20069.2.diff (605 bytes) - added by dd32 7 years ago.

Download all attachments as: .zip

Change History (22)

#1 @dd32
9 years ago

  • Component changed from General to Upgrade/Install

I have never heard of a non-executable file requiring the executable bit to be parsed by the server, Likewise, In the last few years, this is the first report of such a server configuration ever being mentioned in the tens of millions of installs.. and there's been some really weird edge-case setups that some people are using.

I say that the php files should not require the executable bit, due to the files not being executed directly themselves, instead, Apache (or whatever server software is in use) would pass the .php files to it's handler, presumably, a PHP CGI running in suexec mode, which itself, has the executable bit set as it's the process that's executed (as the server can actually execute the binary file, rather than the .php text file which is meaningless to anything but a PHP Binary)

That being said, WordPress currently writes files as 644, which is a "safe" permission set, which is more relaxed than may be needed by some servers, any tighter and some servers refuse, more relaxed, and some servers complain, and yet, we can't trust the existing file permissions in many cases due to them being set incorrectly to start with (world writable for example).

In your case, You can add the following define to your wp-config.php file to avoid this issue with core upgrades, and plugin/theme installs/upgrades:

define( 'FS_CHMOD_FILE', 0750 );

(I assume it's 0755 you need, but you might also need to use 754 or 755 - but your host should be aware of this - note, it must be specified with the leading zero, and must not be in a quoted string, exactly as above)


For reference, I've found this old article (scroll down to suexec at the bottom) on a configuration which requires the .php to be set to executable mode, but it also requires a Hashbang to be added to each file to specify the interprator, or, through a little known feature of the linux kernel to specify parsers for specific file types.

#2 @scribu
9 years ago

  • Keywords close added

#3 @gerv
9 years ago

Whoa, wait a second :-) Give us more than a few minutes to argue the case. I've asked the guy who runs our hosting company to drop in and testify to how common this is. If he doesn't, then you can close it. :-)

Gerv

#4 @scribu
9 years ago

  • Keywords reporter-feedback added

Sure.

#5 @Toby Goodwin
9 years ago

apache-2.2.21/support/suexec.c:

   580       * Error out if the program is not executable for the user.
   581       * Otherwise, she won't find any error in the logs except for
   582       * "[error] Premature end of script headers: ..."
   583       */
   584      if (!(prg_info.st_mode & S_IXUSR)) {
   585          log_err("file has no execute permission: (%s/%s)\n", cwd, cmd);
   586          exit(121);
   587      }

In combination with binfmt (which may be little known, but is supported by all the major Linux distros), mod_suexec provides a way for an apache-based hosting service to run php scripts under the uid of the website owner.

#t

#6 @gerv
9 years ago

  • Keywords reporter-feedback removed

Yep, that's what my host is using.

Gerv

#7 @Toby Goodwin
9 years ago

  • Cc Toby Goodwin added

#8 @c3mdigital
7 years ago

  • Resolution set to invalid
  • Severity changed from major to normal
  • Status changed from new to closed

Closing based on discussion above. Seems like an extreme edge case. If anyone else has any input please reopen.

#9 @dd32
7 years ago

  • Keywords close removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

Edgecase or not, 664/775 doesn't fit every hosting providers configuration, and if we can account for that, it'd be better.

Re-opening just so we've got a ticket tracking it.

@dd32
7 years ago

@dd32
7 years ago

#10 @dd32
7 years ago

Attachment attachment:20069.diff​ added

Attempts to work around this issue, by defaulting FS_CHMOD_DIR and FS_CHMOD_FILE to the same permission set as /wp-includes/ and /wp-load.php respectively. The & 0777 is there to limit it to the lower permission bits and not the entire filesystem mode settings.

The only issue i can see is if fileperms() returns incorrectly.. in which case, part of me wants to suggest that we should at least OR it with 0640 (R/W by owner, R by group), such as done in attachment:20069.2.diff

(note: attachment:20069.diff is missing a bracket in the 2nd case, so it'll fatal with that patch, The extra brackets in the define value are also unneeded, fileperms() & 0777 | 0640 works as expected)

Last edited 7 years ago by dd32 (previous) (diff)

#11 @dd32
7 years ago

  • Component changed from Upgrade/Install to Filesystem
  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 3.7

Moving to 3.7 for review.

#12 @dd32
7 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from reopened to closed

In 25469:

WordPress Upgrades: When defining the default filesystem permissions for files/directories, base the value on the existing ABSPATH & index.php file permissions - so as to respect the executable bit (if set) and not set global read if not required.
This sets a minimum permission set to 750 and 640 for directories and files, so any systems requring less permission than that will still need to define the constants themselves. Fixes #20069

#13 follow-up: @dd32
7 years ago

Switched to ABSPATH and ABSPATH/index.php at the last minute, and included a fallback/minimum to 0750 and 0640 for dirs and files respectively.

I can think of ways to misconfigure a server to cause this to break, but that would most likely cause PHP to break all together in the first place.. Lets give this a soak and see if any issues related to permissions come up before release.

We can increase the minimum to 0755 and 0644 if need be (which is what they were at previously), but I lowered it to 750/640 so as to accomodate those who have no need/want for non-owner/non-group users to have access to the files (not a far fetched scenario on shared hosts)

#14 in reply to: ↑ 13 @SergeyBiryukov
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to dd32:

We can increase the minimum to 0755 and 0644 if need be (which is what they were at previously)

[25469] broke one of my sites.

After upgrading the Audio Player plugin, I noticed that its JS files failed to load. Turned out the permissions for wp-content/plugins/audio-player were reset to 750 on upgrade. Once I changed them to 755 (the value used for other directories), the files started loading again.

I guess we should restore 0755/0644. For reference, the ABSPATH permissions on my hosting are:

fileperms( ABSPATH ): 0710
fileperms( ABSPATH . 'index.php' ): 0644

Looks like files in the root directory would have failed to load too, but somehow they all load fine. The issue only affects the directories inside ABSPATH.

#15 follow-up: @dd32
7 years ago

fileperms( ABSPATH ): 0710

and

wp-content/plugins/audio-player were reset to 750 on upgrade

should be compatible with each other, as long as the file ownerships of directories/files were correct. The extra 4 on the directory group permissions simply mean "You can modify files here, in addition to the ability to list files (1) )..

I'm however curious if your permissions were bad to begin with, What should ABSPATH actually be set to? Are you forcing the Direct transport? Were the file ownerships of the files correct? 755 would only be needed if the owner+group are completely different which shouldn't be the case.

For 3.7, we can update it to a minimum of 755/644 if required.

#16 @nacin
7 years ago

Not sure what's going on here, but seems like [25469] should be reverted now. If this broke another committer's site, chances are it will break a decent number of sites in the wild. This was a nice fix but we have a lot more to do for 3.7.

#17 follow-up: @dd32
7 years ago

Yeah, Lets wait and find out what caused this to break first. Then we can increase it to a min of 755/644.

#18 in reply to: ↑ 17 @nacin
7 years ago

  • Severity changed from normal to blocker

Replying to dd32:

Yeah, Lets wait and find out what caused this to break first. Then we can increase it to a min of 755/644.

Sure. Setting to blocker.

#19 in reply to: ↑ 15 @SergeyBiryukov
7 years ago

Replying to dd32:

I'm however curious if your permissions were bad to begin with, What should ABSPATH actually be set to? Are you forcing the Direct transport? Were the file ownerships of the files correct? 755 would only be needed if the owner+group are completely different which shouldn't be the case.

No, I'm not forcing the Direct transport, and I didn't change the default permissions set by the hosting provider. I can give you an SSH access if you'd like to take a closer look at file ownerships.

#20 @dd32
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 25739:

Upgrader: Create Directories with a minimum of 0755 and files with a minimum of 0644 when upgrading, which matches pre-3.7 behaviour. Fixes #20069

Note: See TracTickets for help on using tickets.