nextcloud / serverinfo

📊 A monitoring app which creates a server info dashboard for admins
GNU Affero General Public License v3.0
95 stars 61 forks source link

shell_exec #169

Open MeiRos opened 4 years ago

MeiRos commented 4 years ago

Is it possible to not use shell_exec because it's unsafe? There are people who have disabled it and that's causing errors. Of course that also means something isn't working.

_error shellexec() has been disabled for security reasons

Steps to reproduce

1.Install NC18.0.0 RC1 with PHP 7.4.1 2.Open the NC and go to the settings/system information 3.See log

Expected behaviour

No errors

Actual behaviour

I got errors and for example I can't see network information

Server configuration

Operating system: Centos 7.8

Web server: Nginx 1.17.7

Database: MariaDB 10.3.21 PHP version: 7.4.1

Nextcloud version: Nextcloud 18.0.0 RC1

Updated from an older Nextcloud/ownCloud or fresh install: fresh

Where did you install Nextcloud from: download.nextcloud.com

Signing status:

Signing status ``` No errors have been found. ```

List of activated apps:

Only default which are tested

Are you using external storage, if yes which one: no

Are you using encryption: no

Are you using an external user-backend, if yes which one: no

Logs

Nextcloud log (data/nextcloud.log)

Nextcloud log ``` Insert your Nextcloud log here {"reqId":"rgMe4fuvcWCRhoGuelTv","level":3,"time":"2020-01-04T21:14:33+00:00","remoteAddr":"x","user":"admin","app":"PHP","method":"GET","url":"/ocs/v2.php/apps/serverinfo/api/v1//basicdata?format=json","message":"shell_exec() has been disabled for security reasons at /home/nginx/home.net/public/apps/serverinfo/lib/OperatingSystems/DefaultOs.php#96","userAgent":"Mozilla/5.0 (X11; CrOS x86_64 12607.58.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.86 Safari/537.36","version":"18.0.0.8","id":"5e1101566eed7"} {"reqId":"rgMe4fuvcWCRhoGuelTv","level":3,"time":"2020-01-04T21:14:33+00:00","remoteAddr":"x","user":"admin","app":"PHP","method":"GET","url":"/ocs/v2.php/apps/serverinfo/api/v1//basicdata?format=json","message":"shell_exec() has been disabled for security reasons at /home/nginx/home.net/public/apps/serverinfo/lib/OperatingSystems/DefaultOs.php#95","userAgent":"Mozilla/5.0 (X11; CrOS x86_64 12607.58.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.86 Safari/537.36","version":"18.0.0.8","id":"5e1101566eee8"} {"reqId":"rgMe4fuvcWCRhoGuelTv","level":3,"time":"2020-01-04T21:14:33+00:00","remoteAddr":"x","user":"admin","app":"PHP","method":"GET","url":"/ocs/v2.php/apps/serverinfo/api/v1//basicdata?format=json","message":"shell_exec() has been disabled for security reasons at /home/nginx/home.net/public/apps/serverinfo/lib/OperatingSystems/DefaultOs.php#87","userAgent":"Mozilla/5.0 (X11; CrOS x86_64 12607.58.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.86 Safari/537.36","version":"18.0.0.8","id":"5e1101566eef5"} {"reqId":"rgMe4fuvcWCRhoGuelTv","level":3,"time":"2020-01-04T21:14:33+00:00","remoteAddr":"x","user":"admin","app":"PHP","method":"GET","url":"/ocs/v2.php/apps/serverinfo/api/v1//basicdata?format=json","message":"shell_exec() has been disabled for security reasons at /home/nginx/home.net/public/apps/serverinfo/lib/OperatingSystems/DefaultOs.php#79","userAgent":"Mozilla/5.0 (X11; CrOS x86_64 12607.58.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.86 Safari/537.36","version":"18.0.0.8","id":"5e1101566ef01"} ```
tflidd commented 4 years ago

You mean a different way to execute shell commands? Because without shell, it's probably hard to get system information, not sure perhaps there are modules or other ways to get this.

MeiRos commented 4 years ago

Errors on lines 79 and 87 are about uptime. I have read that uptime can be read with file_get_contents. I can't write code so I don't know if it's suitable here. Or is it any safer... It doesn't matter how we'll get the same information if it's done more securely it's better. (with or without shell)

If there are no other ways to do this. Is it possible to get rid of the errors in the log? Like testing if shell_exec is available and giving notification to serverinfo if not and skipping shell_exec parts in the code. Just no errors to log.

Mikrz commented 4 years ago

Regarding Uptime calculation, for Linux systems at least you can read /proc/uptime to get the seconds since last boot as the first number in the file. There's also /proc/net/dev that can be used to determine network interface usage, at least accumulated usage. This would be perfectly secure as they're publicly readable "files" that any user on the system can read, with no sensitive information.

makuser commented 4 years ago

as @kesselb already said in the original issue https://github.com/nextcloud/server/issues/18659#issuecomment-570921605, you will be able to get most if not all of the information without shell_exec.

So instead of doing cat /proc/cpuinfo, you can just do file_get_contents("/proc/cpuinfo") and do the grepping and awk in native php.

kesselb commented 4 years ago

I already replaced the shell_exec call for /proc/meminfo here: https://github.com/nextcloud/serverinfo/pull/183.

I will look into:

J0WI commented 4 years ago

What about disk_free_space and disk_free_space to replace df? https://github.com/nextcloud/serverinfo/blob/9dfa5ddb2aabb1772b37ced95f8ca991e20f6c7f/lib/OperatingSystems/DefaultOs.php#L209

kesselb commented 4 years ago

Could work together with /proc/mounts.

HigH-HawK commented 4 years ago

It looks like this has already been solved with the protected functions executeCommand() and readContent()

@kesselb I guess this issue can be closed :)

J0WI commented 4 years ago

executeCommand() and readContent() are just wrappers and still require shell_exec():

https://github.com/nextcloud/serverinfo/blob/3230b6e58f6fe46e208e1ed54f26e5beec060f0d/lib/OperatingSystems/DefaultOs.php#L217-L231

HigH-HawK commented 4 years ago

readContent() doesn't as far as I can see, since it's only the equivalent to file_get_contents().

The shell_exec in the function is escaped, hence as secure as possible. Some UNIX systems still require the shell_exec because there's no file equivalent, to use readContent() on.

J0WI commented 4 years ago

The issue is to avoid shell_exec completely to avoid issues on systems where the function is disabled. https://github.com/nextcloud/serverinfo/issues/169#issuecomment-631762215 is the only remaining use of executeCommand().

kesselb commented 4 years ago

shell_exec is also used for the network section.

J0WI commented 4 years ago

shell_exec('cat ...') can also be replaced by readContent('...')

HigH-HawK commented 4 years ago

@J0WI I see what you mean, thanks for the insight on this and I kind of agree, to keep functions as open as possible, in case someone can't use certain PHP functionality, due to restrictions.

Unless someone else is quicker changing it and creating a pull request, I'm happy to have a look at it.

kesselb commented 4 years ago

I'm already working on a new version of the network information parser: https://gist.github.com/kesselb/1d40c6f3c3491c8ecd59dcbb4ae5ea8d

It still needs shell_exec but for most information no read access to /sys/class/net is required anymore. Also it uses less shell_exec calls then before.

HigH-HawK commented 4 years ago

That looks good 👍

solracsf commented 3 years ago

Shouldn't we also check if we can shell_exec() before even call it anywhere?

https://stackoverflow.com/questions/21581560/php-how-to-know-if-server-allows-shell-exec#answer-21581873

HighPriest commented 3 years ago

I would like to dig out this issue, as I've come to find out the "System" Settings tab is inaccessible without access to root level applications like ifconfig. This makes the mentioned "System" tab throw a 500 error no matter if the shell_exec is allowed or not.

derekslenk commented 2 years ago

Still hope people are thinking about this

qchn commented 2 years ago

This Arbitrary code execution vector needs to be fixed, urgently. Not only because it is unnecessary to retreive the system information via shell_exec as explained in the comments before. This finding deserves a little more attention, it has been opened 2 years ago and the implementation of a fix is very easy. Please act, Nextcloud. :-)

Thanks Robert

tflidd commented 2 years ago

a fix is very easy. Please act, Nextcloud. :-)

Thanks to open source, you don't have to wait for Nextcloud, you can act yourself, pull requests are welcome.

qchn commented 2 years ago

Thanks to open source, you don't have to wait for Nextcloud, you can act yourself, pull requests are welcome.

True that! Unfortunately, I'm not really into PHP but I'm sure someone around here is... :-)

kesselb commented 1 year ago
<?php

$route = file_get_contents('/proc/net/route');

$matches = [];
if (preg_match('#\t00000000\t(?<Gateway>\w+)\t#', $route, $matches) === 1) {
    $a = hexdec($matches['Gateway']);
    $b = long2ip($a);
    $c = explode('.', $b);
    $d = array_reverse($c);
    echo 'gateway: ' . implode('.', $d);
} else {
    echo 'gateway not found';
}

Hi, could you please give the above script a test and let me know if it detects your gateway properly?