Make WordPress Core

Opened 12 years ago

Closed 10 years ago

#20069 closed defect (bug) (fixed)

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

Reported by: gerv's profile gerv Owned by: dd32's profile 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 11 years ago.
20069.2.diff (605 bytes) - added by dd32 11 years ago.

Download all attachments as: .zip

Change History (22)

#1 @dd32
12 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
12 years ago

  • Keywords close added

#3 @gerv
12 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
12 years ago

  • Keywords reporter-feedback added

Sure.

#5 @Toby Goodwin
12 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
12 years ago

  • Keywords reporter-feedback removed

Yep, that's what my host is using.

Gerv

#7 @Toby Goodwin
12 years ago

  • Cc Toby Goodwin added

#8 @c3mdigital
11 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
11 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
11 years ago

@dd32
11 years ago

#10 @dd32
11 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 11 years ago by dd32 (previous) (diff)

#11 @dd32
11 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
11 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
11 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
10 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
10 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
10 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
10 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
10 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
10 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
10 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.