Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 4 years ago

#19235 closed task (blessed) (fixed)

Turn ms-files.php off by default

Reported by: nacin's profile nacin Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: Multisite Keywords: has-patch
Focuses: Cc:

Description

Similar to what we did with global terms, I suggest we disable ms-files.php handling for new installs. Reasoning follows.

Spam Control?

The primary intent of ms-files.php, I presumed, was to prevent images from being accessible for spammed, archived, or deleted blogs. Ultimately, this is not indicated in the UI, and if it goes away, most admins are not going to miss it. If a site needs to be deleted, it can just be deleted (for most networks). If you do need this kind of protection, then you're clearly running a more advanced network, and you should enable it manually.

Security?

ms-files.php additionally does mime-type checking. Contrary to what many may assume, though, it does not check against the upload file types option that is on the Network Settings page. Rather, it only uses wp_check_filetype(). There is a filter on wp_check_filetype() which could be used to add/remove mime types, but since plugins are not yet included, it cannot be used out of the box.

Performance?

Reading a file out via PHP is terrible for performance. While some caching headers are applied, it would be far more efficient if these are applied directly by the server. To load a typical blog page with 10-15 images in the posts might just require loading WordPress 11-16 times, depending on what is in browser cache. (Granted, SHORTINIT, but it's still bad to spin up a PHP process for this.)

If you want to leverage ms-files.php, you will need object caching (at the very least). But realistically, it's the only aspect of multisite that truly requires object caching out of the box. Removing it will make multisite easier to use and remove one of the biggest differences between single-site and multisite. Someone like otto42 who is simply running a few blogs that are statically cached has no necessity for ms-files.php.

We Can Be Backwards Compatible

ms-files.php is simply enabled by routing /files/ through the handler via mod_rewrite or web.config. Since we do not touch rewrite rules for network sites after installation, we can turn this off for new installs without any impact.

For implementation, we should just continue to use the /wp-content/uploads/ directory. We can simply create a new directory called /sites/ in /uploads/ — we'll already have permissions for wp_mkdir_p() — and then add blog ID folders in there. No more blogs.dir, no more blogs.php, no more ms-files.php.

For existing sites wanting to transition over, they will need special rules to rewrite /files/ to the new location. Since we will be keying things by blog ID on the FS level, we may need some sort of ms-files.php-like migration script that simply does the lookup then fires a 301 redirect. This of course would only be opt-in if they modify their rewrite rules.

Finally, we would need to modify (for new installs) the upload paths and URLs (fileupload_url, upload_path, etc.). We would probably need to introduce a function similar to global_terms_enabled(), such as ms_files_rewriting_enabled(), to handle such toggles.

Note, these new filesystem paths would expose blog IDs. I don't know of another instance where these are currently exposed, but I don't see a reason to care. It should be noted that blog IDs on WordPress.com are exposed assuming you realize how wp.me shortlinks are generated.


Is this going to be a pain? Yes. But in the end, it'll definitely be a worthwhile transition.

Attachments (6)

19235.diff (13.8 KB) - added by nacin 12 years ago.
First pass rips out everything without regard for an upgrade path or any kind of back compat. Meant for basic testing/review of the approach.
19235.2.diff (22.8 KB) - added by nacin 12 years ago.
19235.3.diff (27.2 KB) - added by nacin 12 years ago.
19235.4.diff (27.2 KB) - added by wpmuguru 12 years ago.
refresh of 19235.4.diff to current trunk
19235.5.diff (3.4 KB) - added by nacin 11 years ago.
19235-ut.diff (1.5 KB) - added by ryan 11 years ago.

Download all attachments as: .zip

Change History (74)

#1 @scribu
12 years ago

Completely agree that the performance penalty and added complexity far outweigh the benefit of ms-files.php in most cases.

#2 @wpmuguru
12 years ago

One of the original reasons for hiding the blog id probably was that the table prefix was hard coded to wp_ in MU.

I don't see any reason not to do this. By doing this we would also fix the media path/url generation while switched which technically works but is less than ideal.

#3 @andrea_r
12 years ago

  • Keywords dev-feedback 2nd-opinion removed

It would be one less step in setup, and also help with user perception. Many people think the blogs actually live in blogs.dir. Also I have seen issues with Apache needing tweaks to handle just the image rewrite. You can imagine how that goes over with shared hosts.

I'd love to see more chime in.

#4 @markjaquith
12 years ago

Yes, please.

#5 @wpmuguru
12 years ago

What if the sitemeta key were the path to the ms media root ex /wp-content/uploads/sites and that was used on blog/site creation to set the upload_path, etc. Then if the network owner wanted to split media across multiple folders they could just change that path. It also could be added to the create a network screen.

#6 @nacin
12 years ago

  • Keywords 3.4-early added

What if the sitemeta key were the path to the ms media root ex /wp-content/uploads/sites and that was used on blog/site creation to set the upload_path, etc.

That seems like a good idea. Then they can filter it and shard it to their heart's delight.

#7 follow-up: @ericmann
12 years ago

Gut instinct: I don't like it. But that's more due to all of the weird handling I've been doing lately to fix an issue with a particular image management plugin.

It was built with single installations in mind, so /wp-content/uploads is hard-coded in many places within the plugin. But the plugin alleges network compatibility and has started using /blogs.dir/X/ in many places as well ... and /files in yet others. I've spent a week now hacking the thing apart to make it more flexible - so it will work for single sites and networks.

So from a forward momentum perspective, I think this is a great proposal. It will save a lot of time in the future and prevent convoluted, poorly-coded systems like this. But a large part of me is wondering how many existing systems this will break? How many plugins and themes have hard-coded the existing schema already?

I'd be all for making this move for the future, so long as we test the crap out of any backwards compatibility that gets baked in.

#8 in reply to: ↑ 7 ; follow-up: @nacin
12 years ago

Replying to ericmann:

Gut instinct: I don't like it. But that's more due to all of the weird handling I've been doing lately to fix an issue with a particular image management plugin.

We'll be backwards compatible in core, that's for sure. But you can't fix stupid.

#9 in reply to: ↑ 8 @ericmann
12 years ago

Replying to nacin:

But you can't fix stupid.

:-)

#10 follow-up: @Otto42
12 years ago

I wouldn't miss it if it was gone, but I was kinda hoping to use it at some future point to do dynamic image resizing. Still, that can be done without ms-files.php.

#11 follow-up: @adambackstrom
12 years ago

Just to add to the discussion, allowing direct access to the files under nginx/php-fpm can allow remote code execution if the server is configured poorly:

http://wiki.nginx.org/Pitfalls#Pass_Non-PHP_Requests_to_PHP.

Under WordPress 3.2.1, I can upload a file "foo.jpg" that contains PHP, and an attacker could craft a URL that causes PHP to evaluate the contents of this file. There are several ways to protect yourself, and nginx/php-fpm is the less common server setup, but ms-blogs.php offers basic protection if you keep blogs.dir out of the document root. Felt like it should be part of the thread.

Last edited 12 years ago by scribu (previous) (diff)

#12 in reply to: ↑ 10 @nacin
12 years ago

Replying to Otto42:

I wouldn't miss it if it was gone, but I was kinda hoping to use it at some future point to do dynamic image resizing. Still, that can be done without ms-files.php.

Yeah — It'd actually be easier to implement dynamic image resizing without ms-files.php. Because plugins aren't loaded, nothing can alter ms-files.php. You'd have to create a new file to serve images and alter the rewriting.

The best way to deal with dynamic image resizing is to handle it on 404s within wp-content/uploads/*, which can then be processed to generate the image and place it where it belongs.

#13 in reply to: ↑ 11 @nacin
12 years ago

Replying to adambackstrom:

Under WordPress 3.2.1, I can upload a file "foo.jpg" that contains PHP, and an attacker could craft a URL that causes PHP to evaluate the contents of this file. There are several ways to protect yourself, and nginx/php-fpm is the less common server setup, but ms-blogs.php offers basic protection if you keep blogs.dir out of the document root. Felt like it should be part of the thread.

The solution for that would be to finally implement proper content sniffing on uploads, which is a separate matter. And making sure that PHP cannot be executed in blogs.dir or wp-content/uploads is best practice for performance, scaling, and security reasons anyway.

#14 @ryanhellyer
12 years ago

Looks good to me.

#15 @ocean90
12 years ago

  • Cc ocean90 added

#16 @johnbillion
12 years ago

  • Cc johnbillion@… added

#17 @wpmuguru
12 years ago

Related #12756 - Cover this base while doing the proper sniffing.

#18 @Ipstenu
12 years ago

From a setup/host perspective, yes, please. It would save us havign to say "No, no, blogs.dir is the name of the folder."

The blog ID was never a huge secret anyway, no more than wp-content/uploads was (of course, it will screw up the people who, against all recomendations, went and changed the uploads location manually - I keep telling people not to mess with it if they don't understand how ms-files works, but you can't fix stupid).

I don't think it would cause any more plugin problems than it does now, though this is something where perhaps we should look and see how many (if any) plugins are using ms-files.php...

I did a quick check and came up with these:

Two exploit scanners (which makes sense given how they work):

  • exploit-scanner
  • jw-cloud-sites-wp-scanner
  • ultimate-security-checker

Directions on how to edit your permalinks/htaccess:

  • nginx-compatibility
  • wcs-custom-permalinks-hotfix
  • wordpress-multi-site-enabler-plugin-v10

Role Scoper calls in rewrite-mu_rs.php (interesting):

  • role-scoper

The only one that surprises me is Role Scoper, though. The rest are nothing to worry about.

Last edited 12 years ago by Ipstenu (previous) (diff)

#19 @toscho
12 years ago

  • Cc info@… added

#20 @johnnytee
12 years ago

  • Cc johnnytee added

#21 @unsalkorkmaz
12 years ago

  • Cc unsalkorkmaz@… added

Related: #19030

#22 @nacin
12 years ago

  • Type changed from defect (bug) to enhancement

#23 @djeyewater
12 years ago

I would definitely appreciate this change as well. I don't think it should strictly be necessary to expose the blog id, you could probably use a rewrite map or otherwise a separate filepath structure and rewrite rule depending on whether multisite was set up to use path, domain, or subdomains.

#24 @PeteMall
12 years ago

  • Milestone changed from Awaiting Review to 3.4

#25 follow-up: @juliobox
12 years ago

  • Component changed from Multisite to Security
  • Severity changed from normal to critical
  • Version set to 3.3.1

About Security, my view :
Test: http://hollywoodpq.com/wp-content/blogs.dir/2/files/obm-gallery/widgetCache.php
Now just remove "wp-content/blogs.dir/2/" you got now:
New test: http://hollywoodpq.com/files/obm-gallery/widgetCache.php

Php files are downloadables ? Damn . . .
What do you think about that ?

ps: Demo site found with google.
Julio - Web Security Consultant - boiteaweb.fr

#26 in reply to: ↑ 25 ; follow-up: @wpmuguru
12 years ago

  • Component changed from Security to Multisite
  • Severity changed from critical to normal

Replying to juliobox:

About Security, my view :
Test: http://hollywoodpq.com/wp-content/blogs.dir/2/files/obm-gallery/widgetCache.php
Now just remove "wp-content/blogs.dir/2/" you got now:
New test: http://hollywoodpq.com/files/obm-gallery/widgetCache.php

Php files are downloadables ? Damn . . .
What do you think about that ?

ps: Demo site found with google.
Julio - Web Security Consultant - boiteaweb.fr

Why are you putting PHP files in your media folders? If you are going to upload PHP files to your media folders don't expect WP security to protect your site.

WP does not allow a user to upload PHP files to the media folder.

#27 in reply to: ↑ 26 ; follow-up: @juliobox
12 years ago

Replying to wpmuguru:

Replying to juliobox:

About Security, my view :
Test: http://hollywoodpq.com/wp-content/blogs.dir/2/files/obm-gallery/widgetCache.php
Now just remove "wp-content/blogs.dir/2/" you got now:
New test: http://hollywoodpq.com/files/obm-gallery/widgetCache.php

Php files are downloadables ? Damn . . .
What do you think about that ?

ps: Demo site found with google.
Julio - Web Security Consultant - boiteaweb.fr

Why are you putting PHP files in your media folders? If you are going to upload PHP files to your media folders don't expect WP security to protect your site.

WP does not allow a user to upload PHP files to the media folder.

I do not put PHP files, but people and plugins are doing it.
I found some plugins which copy some php files also, just google some dorks.

#28 @SergeyBiryukov
12 years ago

  • Version changed from 3.3.1 to 3.0

Version field indicates the earliest affected version. ms-files.php was copied from MU in 3.0.

#29 in reply to: ↑ 27 ; follow-up: @wpmuguru
12 years ago

Replying to juliobox:

I found some plugins which copy some php files also, just google some dorks.

If you want to report a plugin try http://wordpress.org/extend/kvetch/. Core trac is for WP core.

#30 in reply to: ↑ 29 @nacin
12 years ago

Replying to wpmuguru:

Replying to juliobox:

I found some plugins which copy some php files also, just google some dorks.

If you want to report a plugin try http://wordpress.org/extend/kvetch/. Core trac is for WP core.

Kvetch isn't monitored — try the support forums, or plugins@….

#31 @juliobox
12 years ago

So this i not a WP issue but a plugin issue ?
Ok, thank you all !

#32 @ocean90
12 years ago

  • Keywords needs-patch added

#33 @djeyewater
12 years ago

  • Cc djeyewater added

#34 @xyzzy
12 years ago

  • Cc dennen@… added

#35 @ryan
12 years ago

  • Milestone changed from 3.4 to Future Release

#36 @boonebgorges
12 years ago

  • Cc boonebgorges@… added

#37 @bluesplinter
12 years ago

  • Cc steve@… added

#38 @seancojr
12 years ago

  • Cc seancojr added

#39 @isaacchapman
12 years ago

  • Cc isaac@… added

#40 @djeyewater
12 years ago

If anyone's interested I'm using a work-around using rewrite rules or a rewrite map for my site. It's not really suitable if you have 100s of blogs on your site though.

#41 @momo360modena
12 years ago

  • Cc amaury@… added

#42 @nacin
12 years ago

  • Milestone changed from Future Release to 3.5
  • Type changed from enhancement to task (blessed)

#43 @wpmuguru
12 years ago

  • Keywords 3.4-early removed

I'll have time to contribute to this in the second half of August.

#44 follow-up: @braydonf
12 years ago

There are also streaming problems with using ms-files.php for ogg audio files on multi-site, so this will be good, to serve direct from the Apache/Nginx.

#45 @deltafactory
12 years ago

Re: streaming problems - This would also fix the default behavior for video streaming in iOS. The video player requires byte-range requests but mod_rewrite blocks that functionality.

#46 @kieranmasterton
12 years ago

  • Cc kieranmasterton added

@nacin
12 years ago

First pass rips out everything without regard for an upgrade path or any kind of back compat. Meant for basic testing/review of the approach.

#47 @nacin
12 years ago

19235.diff:

First pass rips out everything without regard for an upgrade path or any kind of back compat. Meant for basic testing/review of the approach.

@nacin
12 years ago

#48 @wpmuguru
12 years ago

That looks like it will work.

For non rewrite installs I had been thinking about the possibility of checking for ms_rewriting_enabled & if not enabled just use the upload_path option and bypass all the other checks.

@nacin
12 years ago

#49 @nacin
12 years ago

  • Keywords has-patch added; needs-patch removed

19235.3.diff — fully backwards compatible. Handles the upgrade seamlessly. Works great in my testing.

  • The URL becomes wp-content/uploads/sites/%d. Not the cleanest, but definitely the best option.
  • ms_files_rewriting becomes a network-wide option set in upgrade_network() and populate_network(). Defaults to 0 for new installs, 1 for existing networks, and new networks on an existing install inherit. If upgrade_network() hasn't run yet, all existing networks will get screwed up, so I use a filter to enforce, globally, the default of get_site_option( 'ms_files_rewriting' ) to be true until something is inserted into the DB.
  • All logic ends up in wp_upload_dir(). 'UPLOADS' is again a fully functional constant, regardless of is_main_site() or $switched. ($switched is simply there to ensure the old MS constants aren't lying, given that they hard-code a blog ID.)
  • upload_path for new sites (on a non-rewriting network) is whatever the main site's upload_path is, which is going to be empty. Note that "sites/%d" is always appended to upload_path (which in this case would translate to "wp-content/uploads"). Some direct DB queries in install_blog() are cleaned up as well.
  • wp_upload_dir() is changed to always return the data you wanted, even if it returns an error due to directory creation failing. Huzzah! _wp_relative_upload_path() is changed to always trim off the basedir, even when there is an error, as an enhancement.
  • Warnings for custom content directories are now only issued for subdirectory networks (thanks to the wp-(content|admin|includes) rewrite). Subdomain networks are now in the clear.

#50 @wpmuguru
12 years ago

19235.3.diff looks good.

#51 in reply to: ↑ 44 @SergeyBiryukov
12 years ago

Replying to braydonf:

There are also streaming problems with using ms-files.php for ogg audio files on multi-site

Related: #15552

@wpmuguru
12 years ago

refresh of 19235.4.diff to current trunk

#52 @wpmuguru
12 years ago

That should have been a refresh of 19235.3.diff. That worked fine for me for backward compatibility.

#53 @lightningspirit
12 years ago

  • Cc lightningspirit@… added

#54 @nacin
12 years ago

In [21822]:

Always return upload directory information from wp_upload_dir(), even if there is an error. Append the error to the array. see #19235.

#55 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21823]:

Remove ms-files.php rewriting from WordPress multisite. fixes #19235.

Keep existing networks compatible with a ms_files_rewriting network option.

#56 @nacin
12 years ago

In [21881]:

Attach the default ms_files_rewriting site option filter in ms-default-constants, as ms-default-filters is not included during SHORTINIT. see #19235.

#57 @nacin
12 years ago

In [21892]:

Account for the old 'main override' in wp_upload_dir() for the main site in a post-MU network by declining to define the UPLOADS constant.

Fixes uploads on the main site of a post-MU network that uses ms-files rewriting. see #19235.

#58 @nacin
11 years ago

In [22038]:

If a pre-3.0 (MU era) network disables ms-files manually, they don't need /sites/ in their wp-content/uploads directory. see #19235.

#60 @nacin
11 years ago

In [22106]:

Pass the current blog id to is_main_site() in wp_upload_dir(), because is_main_site() without arguments does not respond correctly on switch. see #19235.

#61 @nacin
11 years ago

For [22106], originally posted as comment:44:ticket:21459:

Before #19235, switching never worked because it relied on constants. Now, it calls is_main_site(). Unfortunately, is_main_site() uses $current_blog->blog_id rather than $blog_id or $wpdb->blogid, and therefore does not respond when the site is switched.

That means that when switched, is_main_site() returns a different value than is_main_site( get_current_blog_id() ).

That's enough to make me want to start switching $current_blog and $current_site, see #14190. Unfortunately, I have no idea what it'd break.

Switching to the cryptic, seemingly nonsensical, and unnecessary-but-necessary is_main_site( get_current_blog_id() ) should fix this.

#62 @ryan
11 years ago

Opened #22090.

#63 @nacin
11 years ago

On networks that haven't yet run the upgrade routine, this has the potential to cause a boatload of queries. This is because sitemeta options don't have a notoptions cache like regular options do, and wp_load_core_site_options() does not try to cache any kind of default values for those that don't exist. We may want to consider making improvements to either/both of those in the future.

@nacin
11 years ago

@ryan
11 years ago

#64 @ryan
11 years ago

19235-ut.diff passes with or without 19235.5.diff. It's hard to get full test coverage due to the constants.

#65 @nacin
11 years ago

In [22222]:

Have wp_upload_dir() account for blog switching, ms-files rewriting, and the UPLOADS constant properly. This type of logic needs a lot of code comments.

Prevents wp_upload_dir() from obeying the UPLOADS constant when ms-files rewriting is enabled and a blog is switched.

Reverts [22106] thanks to [22108].

see #19235.

#66 @SergeyBiryukov
11 years ago

#12496 was marked as a duplicate.

#67 @DrewAPicture
7 years ago

@nacin Welp, here's another one. ms_deprecated_blogs_file() was effectively soft-deprecated in [21823] for this ticket sans any sort of DocBlock or anything.

Is it possible that it wasn't "hard-deprecated" simply because it slipped through the cracks, or was there some other reasoning in play?

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


4 years ago

Note: See TracTickets for help on using tickets.