Make WordPress Core

Opened 12 years ago

Last modified 17 months ago

#21981 reopened enhancement

Securing the uploads directory

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

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 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @nacin
12 years ago

  • Milestone Awaiting Review deleted

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

#2 @nacin
12 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
12 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
12 years ago

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

#5 in reply to: ↑ 4 @dd32
12 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
12 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
12 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
10 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
10 years ago

  • Milestone set to Awaiting Review

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


10 years ago

#11 @chriscct7
9 years ago

  • Keywords needs-patch added

@Presskopp
8 years ago

#12 @Presskopp
8 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 8 years ago by Presskopp (previous) (diff)

#13 @Presskopp
8 years ago

Small correction:

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

to catch .php52 and such

#14 @Presskopp
8 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
6 years 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 years ago

Related: #36177

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#17 @Presskopp
17 months ago

In correction to my last approach:

<IfModule mod_rewrite.c>
RewriteEngine On
RewriteRule ^.*php.*$ - [F]
</IfModule>

to catch everything starting with .php

Note: See TracTickets for help on using tickets.