WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 18 months ago

#19235 closed task (blessed) (fixed)

Turn ms-files.php off by default

Reported by: nacin Owned by: 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 21 months 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 21 months ago.
19235.3.diff (27.2 KB) - added by nacin 21 months ago.
19235.4.diff (27.2 KB) - added by wpmuguru 20 months ago.
refresh of 19235.4.diff to current trunk
19235.5.diff (3.4 KB) - added by nacin 19 months ago.
19235-ut.diff (1.5 KB) - added by ryan 19 months ago.

Download all attachments as: .zip

Change History (72)

comment:1 scribu2 years ago

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

comment:2 wpmuguru2 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.

comment:3 andrea_r2 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.

comment:4 markjaquith2 years ago

Yes, please.

comment:5 wpmuguru2 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.

comment:6 nacin2 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.

comment:7 follow-up: ericmann2 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.

comment:8 in reply to: ↑ 7 ; follow-up: nacin2 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.

comment:9 in reply to: ↑ 8 ericmann2 years ago

Replying to nacin:

But you can't fix stupid.

:-)

comment:10 follow-up: Otto422 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.

comment:11 follow-up: adambackstrom2 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 2 years ago by scribu (previous) (diff)

comment:12 in reply to: ↑ 10 nacin2 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.

comment:13 in reply to: ↑ 11 nacin2 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.

comment:14 ryanhellyer2 years ago

Looks good to me.

comment:15 ocean902 years ago

  • Cc ocean90 added

comment:16 johnbillion2 years ago

  • Cc johnbillion@… added

comment:17 wpmuguru2 years ago

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

comment:18 Ipstenu2 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
  • 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.

Version 0, edited 2 years ago by Ipstenu (next)

comment:19 toscho2 years ago

  • Cc info@… added

comment:20 johnnytee2 years ago

  • Cc johnnytee added

comment:21 unsalkorkmaz2 years ago

  • Cc unsalkorkmaz@… added

Related: #19030

comment:22 nacin2 years ago

  • Type changed from defect (bug) to enhancement

comment:23 djeyewater2 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.

comment:24 PeteMall2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:25 follow-up: juliobox2 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

comment:26 in reply to: ↑ 25 ; follow-up: wpmuguru2 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.

comment:27 in reply to: ↑ 26 ; follow-up: juliobox2 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.

comment:28 SergeyBiryukov2 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.

comment:29 in reply to: ↑ 27 ; follow-up: wpmuguru2 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.

comment:30 in reply to: ↑ 29 nacin2 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@….

comment:31 juliobox2 years ago

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

comment:32 ocean902 years ago

  • Keywords needs-patch added

comment:33 djeyewater2 years ago

  • Cc djeyewater added

comment:34 xyzzy2 years ago

  • Cc dennen@… added

comment:35 ryan2 years ago

  • Milestone changed from 3.4 to Future Release

comment:36 boonebgorges2 years ago

  • Cc boonebgorges@… added

comment:37 bluesplinter2 years ago

  • Cc steve@… added

comment:38 seancojr2 years ago

  • Cc seancojr added

comment:39 isaacchapman2 years ago

  • Cc isaac@… added

comment:40 djeyewater23 months 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.

comment:41 momo360modena22 months ago

  • Cc amaury@… added

comment:42 nacin21 months ago

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

comment:43 wpmuguru21 months ago

  • Keywords 3.4-early removed

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

comment:44 follow-up: braydonf21 months 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.

comment:45 deltafactory21 months 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.

comment:46 kieranmasterton21 months ago

  • Cc kieranmasterton added

nacin21 months 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.

comment:47 nacin21 months 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.

nacin21 months ago

comment:48 wpmuguru21 months 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.

nacin21 months ago

comment:49 nacin21 months 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.

comment:50 wpmuguru21 months ago

19235.3.diff looks good.

comment:51 in reply to: ↑ 44 SergeyBiryukov20 months ago

Replying to braydonf:

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

Related: #15552

wpmuguru20 months ago

refresh of 19235.4.diff to current trunk

comment:52 wpmuguru20 months ago

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

comment:53 lightningspirit20 months ago

  • Cc lightningspirit@… added

comment:54 nacin20 months 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.

comment:55 nacin20 months 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.

comment:56 nacin19 months 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.

comment:57 nacin19 months 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.

comment:58 nacin19 months 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.

comment:60 nacin19 months 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.

comment:61 nacin19 months 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.

comment:62 ryan19 months ago

Opened #22090.

comment:63 nacin19 months 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.

nacin19 months ago

ryan19 months ago

comment:64 ryan19 months ago

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

comment:65 nacin18 months 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.

comment:66 SergeyBiryukov18 months ago

#12496 was marked as a duplicate.

Note: See TracTickets for help on using tickets.