Make WordPress Core

Opened 12 years ago

Closed 8 years ago

#17227 closed feature request (maybelater)

wp should work around bug in move_uploaded_file for tighter security

Reported by: chrishecker's profile chrishecker Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2.1
Component: Security Keywords: dev-feedback
Focuses: Cc:


Andrew Nacin said I should put this in a new ticket so others can weigh in. Here's the email with the description:

Hey guys, so I'm trying to harden up (!) my wordpress installation, and the whole world-writable wp-content/uploads thing is avoidable by creating a group that includes me and apache (call it "checkersites"), and making wp-content/uploads et al group writable, group checkersites, and the directories group sticky. So, any new directories and files created are group checkersites so I can toast them, even though apache is the owner.

However, there's a bug in php's move_uploaded_file that it doesn't obey the directory group sticky bit, so any files uploaded and run through move_uploaded_file are apache:apache, which then breaks everything with this scheme (meaning, the files still work, but now I can't modify the them without su'ing, etc.). This has been recorded on the php docs for move_uploaded_files since 2008 (note here), so it looks like they just don't care. I was thinking about patching wordpress to work around this by checking if the destination directory's group sticky bit is set and changing the group to that if so. Would you guys be interested in the patch?

As far as I can tell, this is the only thing that forces non-root users to make directories world writable (or even readable, assuming the admin will set up the shared group for them). Seems like it's worth fixing.

Change History (5)

#1 @dd32
12 years ago

IMO, We can probably ignore the sticky bit, and attempt to set the group to that of the uploads directory in all attempts.

PHP's chgrp() command can only change the group to group's it's within, the uploads directory should be either a directory created by apache (and therefor, default group), or a directory created by the user (the users group). If it's the first case, we don't gain anything. If it's the second case, we no longer need the uploads directory to be world-readable.. which could increase security.

#2 in reply to: ↑ description @chrishecker
12 years ago

Replying to chrishecker:

However, there's a bug in php's move_uploaded_file that it doesn't obey the directory group sticky bit

I did some more testing, and bash's cp obeys it, and mv does not, so maybe calling it a bug is a stretch (and maybe this is why the php folks haven't fixed it, saying it should behave like mv), but the behavior certainly prevents this security hardening technique, which would seem to be a win.


#3 @kurtpayne
12 years ago

  • Type changed from defect (bug) to feature request
  • Version set to 3.2.1

#4 @chriscct7
9 years ago

  • Keywords dev-feedback added

Does anyone still wish to pursue this, or close as maybelater?

Last edited 9 years ago by chriscct7 (previous) (diff)

#5 @wonderboymusic
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

maybelater was suggested 11 months ago

Note: See TracTickets for help on using tickets.