WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#11306 closed feature request (fixed)

Option to disable theme/plugin editor

Reported by: kchrist Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: has-patch
Focuses: Cc:

Description

Allowing editing of executable code via a web interface is a potential security risk.

In a suexec/suphp environment where the code runs as the user who owns it, if a site's admin password has been compromised, an attacker can modify theme/plugin files to execute arbitrary code. This can range from things like adding spam links up to performing attacks on the server, modifying/deleting other files owned by the same user, and so on.

This risk can be prevented by using mod_php instead of CGI, but that's becoming rare in shared hosting environments. It can also be mitigated by using strong passwords and following taking security precautions but let's be honest, the vast majority of people don't.

I'd like to see a config option one could add to wp-config.php, something like WP_DISABLE_CODE_EDITOR or whatever. Disabling the editor via a plugin is useless because if an attacker has access to the WP admin, they can disable plugins at will.

Attachments (4)

11306-WP_CODE_EDITOR-constant.diff (1.1 KB) - added by nacin 5 years ago.
define('WP_CODE_EDITOR', false) to force disabling of the theme and plugins editors.
11306_using_map_meta_cap.diff (786 bytes) - added by nacin 5 years ago.
11306.diff (775 bytes) - added by Denis-de-Bernardy 5 years ago.
(untested)
11306.4.diff (1.0 KB) - added by nacin 5 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 @kchrist5 years ago

  • Cc kchrist added

comment:2 @scribu5 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to 3.0

comment:3 @nacin5 years ago

First take: I would suggest that you simply take a user's edit_plugins and edit_themes capabilities. Since you don't want it in a plugin, you could do it via your theme's functions.php or mu-plugins. Or you can hard-code $wp_user_roles.

Assuming none of that suits you, I suppose a WP_CODE_EDITOR constant check on theme-editor.php and plugins-editor.php couldn't hurt. You'd still need to restrict the capabilities via a plugin, however, that way the user doesn't see any relevant navigation and such.

@nacin5 years ago

define('WP_CODE_EDITOR', false) to force disabling of the theme and plugins editors.

comment:4 @nacin5 years ago

  • Keywords has-patch added; needs-patch removed

comment:5 @ryan5 years ago

We could do this in map_meta_cap() the way we did with ALLOW_UNFILTERED_UPLOADS. I suggest using a [DIS]ALLOW_* naming convention. DISALLOW_FILE_EDIT, perhaps.

comment:6 @nacin5 years ago

Patch using map_meta_cap()...

comment:7 follow-ups: @Denis-de-Bernardy5 years ago

I disagree on the DISALLOW_FILE_EDIT.

It should be ALLOW_FILE_EDIT. i.e. off by default.

(And this should go in 2.9.)

@Denis-de-Bernardy5 years ago

(untested)

comment:8 in reply to: ↑ 7 @scribu5 years ago

Replying to Denis-de-Bernardy:

I disagree on the DISALLOW_FILE_EDIT.

It should be ALLOW_FILE_EDIT. i.e. off by default.

(And this should go in 2.9.)

Why should it be off by default?

comment:9 in reply to: ↑ 7 @lloydbudd5 years ago

Replying to Denis-de-Bernardy:

I disagree on the DISALLOW_FILE_EDIT.

It should be ALLOW_FILE_EDIT. i.e. off by default.

Wow, now that would be a regression!

I'm always surprised that 4 out of 5 people I sit down with to fix something on their site use the built in code editor -- it's one of WordPress's killer features. It makes me feel dirty as well ;-)

comment:10 @dd325 years ago

  • Version set to 2.9

Wow, now that would be a regression!

I quite agree. Whilst i'm all for tightening security, Thats probably a step too far. I thought the same about the WP_ALLOW_UNFILTERED_UPLOADS, IMO, It should've only defaulted to blacklist potentially harmful content. This is the same, Whilst it can be abused, its also a very powerful tool.

comment:11 @nacin5 years ago

When I saw "Wow, now that would be a regression!" via wp-trac, I thought that was in reply to DISALLOW_FILE_EDIT and I was confused. Now I see what the fuss was about.

The file editors are secure. What isn't necessarily secure is a user's account, simply due to the problems that exist between keyboards and chairs. That's no reason to disable a feature. It's a slippery slope when features are considered in that context.

If we're going to do ALLOW_FILE_EDIT, then we should also add similar hidden constants to prevent, by default, other controversial features accessible with a user account password, including the ability to delete posts, manage plugins, and import content.

comment:12 follow-up: @Denis-de-Bernardy5 years ago

I realize my opinion is minority (there was a very similar ticket about removing the editor entirely), but it really depends on the audience. The most sophisticated users tend to use SSH, and VI or Emacs. Moderately sophisticated users like and use the editor. Unsophisticated users who even try to use it are just asking for trouble. And WP.com users have absolutely no access to it. :-)

So, it really goes down to "Why show them the gory stuff?" when we could stick to showing the sexy. wp-admin/options.php appears nowhere in the UI, and yet it's just as useful/useless.

comment:13 in reply to: ↑ 12 ; follow-up: @scribu5 years ago

Replying to Denis-de-Bernardy:

So, it really goes down to "Why show them the gory stuff?" when we could stick to showing the sexy. wp-admin/options.php appears nowhere in the UI, and yet it's just as useful/useless.

options.php is still useful because it's just hidden, not disabled. ;)

comment:14 in reply to: ↑ 13 @Denis-de-Bernardy5 years ago

Replying to scribu:

options.php is still useful because it's just hidden, not disabled. ;)

fair enough. ;-)

@nacin5 years ago

comment:16 @nacin5 years ago

Patch refreshed -- [12630] made some changes when running multisite.

comment:17 @dd325 years ago

  • Keywords needs-patch added; has-patch removed

IMO, that constant should be checked in map_meta_cap directly.

It can be disabled with a simple plugin which filters map_meta_cap even..

comment:18 @nacin5 years ago

The diff does patch map_meta_cap().

comment:19 @dd325 years ago

  • Keywords has-patch added; needs-patch removed

Hm, oops. I somehow managed to only view the first diff.

Carry on :)

comment:20 @scribu5 years ago

Closed #12176 as dup.

We also need to consider the multisite aspect, now.

comment:21 @automattor5 years ago

(In [13034]) Introduce DISALLOW_FILE_EDIT flag for enabling/disabling the theem and plugin editors. Props nacin. see #11306

comment:22 @ryan5 years ago

Looks like 3.0 now shows the editor for multi-site super admins. This is a change from previous MU versions. Should DISALLOW_FILE_EDIT default to true for multisite and false for single site when it is not set?

comment:23 @nacin5 years ago

dd32 turned on the editors in [12984]. We would need to make a decision there.

The other issue we need to address of course is that current_user_can() has an early escape hatch giving multisite super admins all caps. So we're going to have problems with DISALLOW_FILE_EDIT and the create_users cap (#12098) because we can't map them properly -- #12109.

Until the capabilities system changes, we could possibly create an array of capabilities that would skip the if ( is_multisite() && is_super_admin() ) return true; check and allow that to progress to map_meta_cap.

comment:24 @dd325 years ago

dd32 turned on the editors in [12984]. We would need to make a decision there.

Yeah, I just realised i made an error there. Once again, that was because of #12109. If you're a super admin and accessed the file directly, you had full access.

[12984] can be reverted if the editor wants to be completely disabled, but that'll require a if ( is_multisite() || ( defined('DISALLOW_FILE_EDIT') && DISALLOW_FILE_EDIT ) ) wp_die('Disabled'); in the editors to prevent super admins accessing it directly too.

comment:25 follow-up: @dd325 years ago

That being said, Is there a reason why the file editors should be disabled for super admins?

While it was prevented in previous MU versions, "MS Super admins by definition have access to everything", is there a -need- to limit what the super admin can access? (Given that they have the capability to do everything)

comment:26 @janeforshort5 years ago

I think if someone is a super admin, they should be able to do everything. If there needs to be some kind of intermediate role, like a network-wide editor, with certain privileges disabled, I'd rather see that than have superadmins not have true network admin access.

comment:27 @nacin5 years ago

Reverting [12984] and making those changes still require us to then check again in menu.php. I think hacking up current_user_can() to allow some caps to go through to map_meta_cap() would solve a lot of our immediate problems.

Alternatively, we could prevent this from going through a filter and map_meta_cap() and do a straight check for DISALLOW_FILE_EDIT and also get_site_option('add_new_users') right in current_user_can(), at least until/when/if we move super admins more towards the roles/cap system.

That being said, Is there a reason why the file editors should be disabled for super admins?

No, I don't think so, but I think DISALLOW_FILE_EDIT should override is_super_admin(). (If one doesn't want it to override, they should be using the existing filters provided and simply remove the caps anyway. Indeed, the ability to do that kind of makes this whole constant nothing more than a convenience -- it could still be done before.)

That said, while I generally otherwise agree with Jane that if someone is a super admin they can do everything, we do use the create_users cap (#12098) to check a site option. I for one cannot remember how many times I've clicked the Users > Add New in MU <= 2.9 only for it to frustrating not take me anywhere. Super admins should not have the Users > Add New menu if they have it set up so users can only be added via Network Admin > Users.

comment:28 @dd325 years ago

No, I don't think so, but I think DISALLOW_FILE_EDIT should override is_super_admin().

Agreed.

I for one cannot remember how many times I've clicked the Users > Add New in MU <= 2.9 only for it to frustrating not take me anywhere.

That was the whole purpose of [12908] which was then reverted in [12916]

But if the super_admin capability mess can be at least tightened up a little bit to make it not needed..

comment:29 in reply to: ↑ 25 @kchrist5 years ago

Replying to dd32:

That being said, Is there a reason why the file editors should be disabled for super admins?

Yes, for the same reason that someone would want it disabled on a single-user site:

Allowing editing of executable code via a web interface is a potential security risk.

With code editing enabled, an attacker who compromises a super admin password will be able to execute arbitrary code on the server, just as they could by compromising the admin account on a regular, single-site WP install.

The idea of this isn't to revoke permissions from certain users, as defined in WP; it's to close a potential attack vector. The option I'm requesting should override any WP capabilities by flat-out saying no file editing is enabled, full stop.

comment:30 @dd325 years ago

Yes, for the same reason that someone would want it disabled on a single-user site:

And thats fine, What i was referring to, Was a separate matter of the code editors being disabled for all users in a MultiSite install (What was previously, MU installs), rather than, such as in a normal WordPress install where the Code editors are disabled only for lower level users.

So like i said in the previous comment, The constant should override the super admin account, just the same as it over rides a normal administrator account.

comment:31 follow-up: @janeforshort5 years ago

We're not taking out file editing, full stop. Matt has said that editing your templates should as pleasant an experience in WordPress as editing your text. Removing the file editor and forcing everyone back to using FTP would be a major step backwards. If someone is that much at risk of having passwords stolen and site overtaken, then they probably shouldn't be an admin. What's to stop someone from stealing their FTP password too, anyway? The whole point of being an admin is that you can do everything.

comment:32 in reply to: ↑ 31 @kchrist5 years ago

Replying to janeforshort:

If someone is that much at risk of having passwords stolen and site overtaken, then they probably shouldn't be an admin.

I respectfully disagree. By this logic, anyone running WP without SSL shouldn't be doing so. Anyone not using SSL is taking this risk. And even more so if they ever use public wifi. You and I know better, but how many WP users do you think do this every day?

Anyway, let's not lose sight of the fact that we're talking about an option. No one is suggesting that the code editor be removed entirely or even disabled by default. But it would be a nice security enhancement for those that want it.

comment:33 @dd325 years ago

Ok, So, To finalise this ticket we have 2 options:

Add this to the editor files:

if ( defined('DISALLOW_FILE_EDIT') && DISALLOW_FILE_EDIT )
die(); //or similar

Or, Fix #12109 somehow.

comment:34 @nacin5 years ago

I think we should handle this in #12109. I like the patch that is progressing there as well.

comment:35 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.