Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow wp-cli as root without --allow-root #973

Merged
merged 6 commits into from Jan 26, 2014
Merged

Don't allow wp-cli as root without --allow-root #973

merged 6 commits into from Jan 26, 2014

Conversation

SpikesDivZero
Copy link
Contributor

I work for a hosting company, and we deploy wp-cli on our hosting machines. On some of our machines, we've observed customers accidentally(?) running wp-cli as root. It goes without saying, this is never ideal, and given what I've seen in the support histories, I'm not sure customers always understand just what root is capable of, and how dangerous it can be to run it as root.

We have been discussing doing this as a plug-in in our internal deployments, but this seems like something that would be nearly universally useful as a safety precaution for all wp-cli users.

The key fear we're hoping to address with this is just what will happen if wp-cli is run against a hacked site.

Seeing as this is a public forum, I'm hesitant to provide further details on a hypothetical attack vector here. If you'd like specifics, scribu, send me a message on freenode (wspikes or wspikes1) and I'll be happy to elaborate more there.

NOTE: My offer for an exploit vector applies only to scribu. I will not send potential exploit details to anyone else at this time.

@scribu
Copy link
Member

scribu commented Jan 21, 2014

Similar: https://github.com/wp-cli/wp-cli/pull/966/files#diff-17626789dd81897bc9f712aa8a7938faR10

I couldn't find you on Freenode. You could send me an email instead, to scribu AT gmail.

@danielbachhuber
Copy link
Member

On some of our machines, we've observed customers accidentally(?) running wp-cli as root.

If this is a part of the security disclose, I totally understand, but how are your customers accidentally running WP-CLI as root? Should they have root access in the first place? Could whatever problem you're seeing be solved at the host-level, instead of a one-off in WP-CLI?

@jmslbam
Copy link
Contributor

jmslbam commented Jan 21, 2014

Meanwhile, is it an option to wrap wp command from within the root .bashrc?
@scribu suggests this method for quite some other issues / for solving other problems.

    if [ `id -u` -eq 0 ]; then
    fi

Hope Scribu can help you.

@SpikesDivZero
Copy link
Contributor Author

@danielbachhuber

[...] how are your customers accidentally running WP-CLI as root? Should they have root access in the first place?

Customers may get full root access on VPS/Dedicated offerings.

It's human error to sometimes forget to "su -s /bin/bash - USER" (or whatever your preferred shell is) before running commands like wp-cli.

Even beyond that, in the sections where customers can get root access to their boxes, we have posted warnings reminding/informing users of the dangers they may encounter. While I'd like to assume that users proceeding past this point would be knowledgeable about what they're doing, the reality is that while most are, this is not always the case.

Could whatever problem you're seeing be solved at the host-level, instead of a one-off in WP-CLI?

We could do this in a system-wide wp-cli plugin, as we've done for some of our other needs. However, this issue may also affect anyone else who may unintentionally run it as root. (Other hosting companies' customers.)

Along this vein, I tried to make this patch as flexible as possible. If a user would like to pull a Leeroy Jenkins, it should be quite easy to add a --allow_root flag on the CLI. :)

if ( $this->config['allow_root'] )
return; # they're aware of the risks!
if ( !function_exists( 'posix_geteuid') )
return; # posix functions not available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to try calling /usr/bin/id -u directly, instead of just giving up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to use posix_geteuid primarily for cross-platform support.

I see that wp-cli has a bin/wp.bat, which suggests that wp-cli hopes to be cross-platform. Executing /usr/bin/id directly is not cross-platform, and may leak warnings/errors through STDERR, in addition to having possibly undefined behavior. (I say "may" since I don't use Windows enough to speak to this definitively.)

I am unfortunately fully aware that PHP's POSIX extension is hit and miss. While they're enabled by default for PHP builds in POSIX-like environments, they are sometimes disabled by distributions.

@scribu
Copy link
Member

scribu commented Jan 21, 2014

Got the details from @SpikesDivZero. The scenario involves an install already compromised via the web somehow. When WP-CLI is run as root, the attacker can get even more privileges - ALL OF THEM, in fact.

@scribu
Copy link
Member

scribu commented Jan 22, 2014

Couple more thoughts:

  • the check doesn't seem to be needed when running commands that have the @when before_wp_load tag
  • the config option should be called --allow-root, instead of --allow_root

@SpikesDivZero
Copy link
Contributor Author

Rename to --allow-root is done. I'll look into the before_wp_load bit later today.

I know file=false is the default, but were that to change at any point,
having this be true could potentially compromise the system's security
if wp-cli was run as root.
@SpikesDivZero
Copy link
Contributor Author

the check doesn't seem to be needed when running commands that have the @when before_wp_load tag

I've replied to your email off-GitHub with my thoughts on this. Suffice it to say, it might tie the same already compromised install scenario we've been talking about.

I don't really like forever-secrecy, so if/when this is merged, I'll share the details more publicly on this bug for other watchers to know exactly what I was bringing up.

It'll still be there, it just won't show up in the man pages anymore,
since the "NOT RECCOMENDED" is pretty ugly. :)
@SpikesDivZero
Copy link
Contributor Author

Re: @when before_wp_load, per email, @scribu agrees that moving it down there isn't something that should happen. (Logging on the bug for reference.)

return; # not root

WP_CLI::error(
"YIKES! It looks like you're running this as root. You probably meant to" .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a space at the end. It prints out "torun".

@scribu
Copy link
Member

scribu commented Jan 24, 2014

Thinking more about this, it's sometimes useful to run a WP-CLI command as the same user that runs the web server.

For example, say you have Apache running under the www-data user. If you run sudo -u www-data wp media regenerate, then all the newly created thumbnails will still be editable from /wp-admin/, without setting extra permissions on them.

@SpikesDivZero
Copy link
Contributor Author

For example, say you have Apache running under the www-data user. If you run sudo -u www-data wp media regenerate, then all the newly created thumbnails will still be editable from /wp-admin/, without setting extra permissions on them.

Yeah, I can certainly see this as being something that'd make sense.

However, Apache should never be set to run as root -- most distributions include either a user specific to Apache for it to run under (httpd, www-data, apache, etc), and all of these users have a non-zero UID so wouldn't really be related to this PR as I understand it?

@scribu
Copy link
Member

scribu commented Jan 25, 2014

so wouldn't really be related to this PR as I understand it?

No, not really. Just a general consideration about file permissions.

scribu pushed a commit that referenced this pull request Jan 26, 2014
Don't allow wp-cli as root without --allow_root
@scribu scribu merged commit 881ecfc into wp-cli:master Jan 26, 2014
TheLastCicada added a commit to Varying-Vagrant-Vagrants/VVV that referenced this pull request Jan 26, 2014
wp-cli now requires --allow-root in order to run as the root user due to security concerns (see wp-cli/wp-cli#982 and wp-cli/wp-cli#973).  I've added this flag to all uses of wp-cli in provision.sh.  The auto-site setup stuff will need adjustment as well, but I'm less familiar with those and will have to take a closer look to see where that is needed.
heyfletch added a commit to heyfletch/Vagrant-Autosetup that referenced this pull request Jan 26, 2014
wp-cli now requires --allow-root in order to run as the root user due to security concerns (see [wp-cli/wp-cli#982[(wp-cli/wp-cli#982) and [wp-cli/wp-cli#973](wp-cli/wp-cli#973)).  I've added this flag to all uses of wp-cli in vvv-init.sh.
@SpikesDivZero SpikesDivZero deleted the no-root branch January 27, 2014 00:51
@nikolay
Copy link

nikolay commented Feb 23, 2014

Can this be configured in wp-cli.yml?

@SpikesDivZero
Copy link
Contributor Author

Unfortunately no, @nikolay. At the time this patch was written, WP-CLI didn't really have a flexible enough configuration system to say "only allow setting this in a config file at the root-level".

I forgot to post a public description of the exact problem that this is meant to solve, so I'm writing that up now and will update this again after a bit.

@SpikesDivZero
Copy link
Contributor Author

I hope this helps clarify the situation, for anyone stumbling across this down the road.

Scenario:

A WP site is compromised, most commonly by either an insecure plugin or theme. The attacker gets filesystem access, and can upload/update arbitrary files. The attacker decides to update wp-config.php to contain some malicious code. I'll leave it to your imagination what you can accomplish from there.

Next, a site administrator sees that their site is acting a bit strange, so they go to use wp-cli to troubleshoot/fix it. So they log into their VPS as root (because they don't know better), and start running commands.

Now you've got malicious code able to do whatever it so pleases on the server, since it's being ran as root.

Why not allow a config file to specify allow-root?

WP-CLI loads all config files at once, and composites them into a single config array. There's no option available, as best I could find, to say "this setting should only be defined in a system-wide configuration, only writable by root".

With this in mind, if we allowed setting allow-root=true in a config file, then an attacker could merely upload their own config file in the WordPress install's DocRoot, and they'd have effectively bypassed this sanity check.

Why not put the check_root after @before_wp_load happens?

Similar to the config file problem above, before_wp_load requires plugins as specified in a config file. This means that all an attacker would have to do to bypass this check is to tell WP-CLI to load an arbitrary plugin that the attacker uploaded.

@nikolay
Copy link

nikolay commented Feb 23, 2014

@SpikesDivZero I understand the security concerns, but my issue is that a minor version broke backward-compatibility for me, and I'm sure I'm not gonna be the only one having this issue out of the blue. Well, I am happy with this feature at the end though as I haven't considered the scenario where an attacker will patiently wait for me to run manually WP-CLI commands as root.

@SpikesDivZero
Copy link
Contributor Author

You're right, it can and did break things for some legitimate use cases (see above where some consumers of WP-CLI had to adjust their tool chains).

Knowing that there were some legitimate use cases, I tried to make the best compromise reasonably possible here -- adding an allow-root flag while having the error message as informative as possible (without being overly verbose).

I wish there was a way not to break things in closing this hole, but it was inevitable in addressing a problem of this nature. =S

@Twanneman
Copy link

Which user should be used when using wp cli? Should I create one ? Or use an already created user?

I'm just getting started using WP CLI and Ubuntu so I'm not sure which user I should use. I'm running ubuntu 14.04 LTS and I've got the following users:
daemon
bin
sys
sync
games
man
lp
mail
news
uucp
proxy
www-data
backup
list
irc
gnats
nobody
libuuid
syslog
messagebus
sshd
avahi
mysql

Could anyone advise or point me in the right direction?

Kind regards :)

@jmslbam
Copy link
Contributor

jmslbam commented Sep 16, 2014

If I'm correct it runs as your user www-data automaticly so you don't have to do anything specific to run it.

Just run an command without you webroot, that's it.

On 16 sep. 2014, at 20:12, Twan van Landschoot notifications@github.com wrote:

Which user should be used when using wp cli? Should I create one ? Or use an already created user?

I'm just getting started using WP CLI and Ubuntu so I'm not sure which user I should use. I'm running ubuntu 14.04 LTS and I've got the following users:
daemon
bin
sys
sync
games
man
lp
mail
news
uucp
proxy
www-data
backup
list
irc
gnats
nobody
libuuid
syslog
messagebus
sshd
avahi
mysql

Could anyone advise or point me in the right direction?

Kind regards :)


Reply to this email directly or view it on GitHub.

@bellwood
Copy link

bellwood commented Oct 6, 2015

Hate to reopen, but, if PHP has the following:

disable_functions => exec,passthru,shell_exec,system,proc_open,popen,show_source,eval

How would any malicious code inside the PHP files cause any issues if ran from root?

@danielbachhuber
Copy link
Member

Hate to reopen

This conversation isn't for re-opening. Using WP-CLI as root requires the --allow-root flag.

@bellwood
Copy link

bellwood commented Oct 6, 2015

No debate there, just asking if the scenario I provide would be mitigated in terms of the risk.

@danielbachhuber
Copy link
Member

No debate there, just asking if the scenario I provide would be mitigated in terms of the risk.

It would be unsafe to assume the situation you provide fully mitigates the risk. If you don't fully understand the impact of running WP-CLI as root, you probably shouldn't run it as root.

@bellwood
Copy link

bellwood commented Oct 6, 2015

I understand it to the extent that it's been explained in this issue.

In a scenario where:

  1. users have no shell

  2. php is prohibited from making system level calls/commands

  3. php is ran as safe cgi where no custom php.ini can be loaded

...and from your explanation, everything is loaded up as the user running the commands; I am trying to come up with a scenario where in any malicious could harm the system.

Perhaps I'm naive (very may well be) but even if something in that wordpress was malicious:

exec("rm -rf /")

...exec is explicitly disabled and would not run.

Perhaps what would be a better approach to "allow-root", would be, if so desired, the ability to enable a shell (jail shell for cpanel, or otherwise) on-the-fly under which wp-cli runs for the duration of the command.

This to me would:

-remove the need for root
-only allow php to run within the users permitted directories (open_basedir or otherwise)

It would be a rather trival use of usermod to make happen and would require the current user to be root.

@SpikesDivZero
Copy link
Contributor Author

@bellwood

Please don't use the --allow-root flag without very good cause and a full understanding of the security implications it carries -- running untrusted code as root, no matter how much you try to lock it down, is an inherently dangerous proposition given the insanely brilliant creativity of attackers.

To expand upon that, however, let's offer a fairly small example...

## UNTESTED
ob_start();
# This file contains all the hashed user passwords. The security
# level of the hashing uses varies based on several factors, but can
# be rather weak on some systems, meaning easy to crack hashes.
# Given the importance of this file, it is *only* readable by root.
include('/etc/shadow');
$shadow = ob_get_clean();
ob_end_clean();

You likely noticed that the functions I've mentioned above -- include and Output Buffering -- are required for WP to operate without issue.

From there, an attacker has a few different options on how to ship the contents of the shadow file to a remote server, but really, any function that provides a way to make an HTTP request is more than enough. WP Core also makes simple HTTP requests as part of it's usual operations.

Now, this is just one small but real example. There's so many more things one can do with just a little bit of creativity.

In choosing what to write here as an example, I had thought of a handful of far more dangerous potential examples that would work without issue in your sample scenario. (I hesitated to put any example here at all, but the use of OB functions with include is prior/known art.)

@bellwood
Copy link

bellwood commented Oct 6, 2015

While I do agree that running anything as root has it's risks and one can't simply account for all instances the above example renders nothing on my system.

It is denied access to the file as it is outside its allowed open_basedir pathing.

I do completely agree with your stance to enforce allow-root, 100%, no questions asked =)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants