WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 6 months ago

#21981 reopened enhancement

Securing the uploads directory

Reported by: japh Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Upload Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

Look at implementing something similar to an .htaccess file in the uploads directory with:

php_flag engine off

This may not work in every server scenario, but let's open the conversation, and some scenarios is probably better than none anyway.

Attachments (1)

php-regex.png (8.7 KB) - added by Presskopp 3 years ago.

Download all attachments as: .zip

Change History (17)

#1 @nacin
7 years ago

  • Milestone Awaiting Review deleted

Unfortunately, if AllowOverrides is set to not allow Options, this generates instant 500 errors.

#2 @nacin
7 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

Meant to close this. If there's a way to implement this without resulting in breakage, feel free to re-open.

#3 @dd32
7 years ago

In addition to the above, if PHP is running via fastCGI or any method other than mod_php, it will also create 500's.

Apache does not like data in it's .htaccess that it doesn't understand..

#4 follow-up: @sirzooro
7 years ago

What about wrapping these directives in <IfModule> </IfModule>?

#5 in reply to: ↑ 4 @dd32
7 years ago

Replying to sirzooro:

What about wrapping these directives in <IfModule> </IfModule>?

my only thoughts there are

  • mod_php.c vs mod_php5.c vs mod_suphp.c (although I'm sure 2 of them are more common than the other variations)
  • would mean different behaviour between different hosts
  • .php files shouldn't end up in the uploads directory to start with, if a select group of WordPress installations were to start having that behaviour, I can almost assure you that some plugins would rely upon it, leading to issues for others.

but more of that last one, rather than looking at the problem, solve the cause, why would .php files end up in the upload directory in most cases?

#6 @japh
7 years ago

A compromised user account can use scripts in /uploads/ to exploit the installation. Not sure of another way around that, besides not letting accounts be compromised in the first place, which is obviously preferable but not always avoidable.

#7 @dd32
7 years ago

A compromised user account can use scripts in /uploads/ to exploit the installation.

If they get access to an Administrator WordPress login, they'll have access to the Theme/Plugin editor on most hosts, but unless the site specifically has ALLOW_UNFILTERED_UPLOADS enabled (it's off by default) they won't be able to upload a .php file.

That being said, since WordPress doesn't do mime checking on the uploaded files, it's still possible with some poorly configured CGI environments to upload a .gif (or similar) which contains PHP code to be executed - .htaccess can't help that scenario though.

#8 @markjaquith
5 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Wait a second. We can forget about php_flag engine off and just use mod_rewrite if available to 403 requests to PHP files.

# Disallow access to PHP files in the uploads directory
<IfModule mod_rewrite.c>
RewriteEngine On
RewriteRule \.php[0-9]?$ - [F]
</IfModule>

The protection scenario here is a locked down environment where only the uploads directory is server-writable, and there is a file-writing compromise. It's a very safe place for an attacker to put PHP files because it is the one place almost guaranteed to be server writable.

#9 @ocean90
5 years ago

  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #core by mark. View the logs.


5 years ago

#11 @chriscct7
4 years ago

  • Keywords needs-patch added

@Presskopp
3 years ago

#12 @Presskopp
3 years ago

Not sure if it's within the bounds of possibility to upload them, but shouldn't we also consider .phtm(l)-files?

Another approach:

<FilesMatch "\.ph(p[0-9]?|tml?)$">
   deny from all
</Files>
Last edited 3 years ago by Presskopp (previous) (diff)

#13 @Presskopp
3 years ago

Small correction:

.ph(p[0-9]?[0-9]?|tml?

to catch .php52 and such

#14 @Presskopp
3 years ago

Ok, afaik now, we don't need the .phtml part. And so this looks like a good solution to me now:

<IfModule mod_rewrite.c>
RewriteEngine On
RewriteRule \php[0-9]?[0-9]?$ - [F]
</IfModule>

#15 @Presskopp
13 months ago

2 years now :( Here's a shorter/better(?) variant:

<IfModule mod_rewrite.c>
RewriteEngine On
RewriteRule \.php[0-9]+ - [F]
</IfModule>

#16 @SergeyBiryukov
6 months ago

Related: #36177

Last edited 6 months ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.