EyeZm Code in Trunk security issues.
Posted: Mon Jan 31, 2011 7:21 pm
I wanted to first thanks the eyeZm guys for contributing back the code to trunk. It moves another step towards a better XML interface for ZM and that only means more apps and making ZM even better. With that said it may be a good idea to disable the code by default or perform a code review of it. The code nicely adds the xml skin type but unfortunately the code uses a lot of shell calls for do things. While to exploit most of the issues would require a user to be authenticated (if auth is turned on) it could certainly cause problems for zome ZM deployments. Params taken from the web request are not always checked (look at actions.php an example is:
The problem here is it takes the monitor param, passes it to kill264proc, which immediately calls:
$pid = trim(shell_exec("pgrep -f -x \"zmstreamer -m ".$monitor."\""));
obviously easy to escape to the shell.
In addition to shell execute there are SQL injection issues like in:
which takes a get param with no checking and immediately puts it into an SQL query.
Again its great to see code being contributed to trunk and thanks again to eyeZM for doing so, but having it enabled by default like this certainly opens a lot of security holes.
I am not all talk however and it is certainly not eyeZM's job to fix it, so I will try to get a patch together to fix most of the major issues I see. Overall shell execute calls from php should only be used when absolutely necessary, and for mysql prepared statements would be best but given most of the doesn't use them making sure to use the proper validators and dbEscape would take a second place.
Code: Select all
} else if (strcmp($action, "kill264") == 0) {
/* ACTION: Kill existing H264 stream process and cleanup files.
* Parms: <monitor>.
* NOTE: This will be called directly by path, so include files
* may not be available */
session_start();
require_once(dirname(__FILE__)."/../includes/functions.php");
if (!isset($_GET['monitor'])) {
logXmlErr("Not all parameters specified for kill264");
exit;
}
$monitor = $_GET['monitor'];
kill264proc($monitor);
$pid = trim(shell_exec("pgrep -f -x \"zmstreamer -m ".$monitor."\""));
obviously easy to escape to the shell.
In addition to shell execute there are SQL injection issues like in:
Code: Select all
} else if (strcmp($action, "vevent") == 0) {
...
$eventsSql = "select E.Id, E.MonitorId, E.Name, E.StartTime, E.Length, E.Frames from Events as E where (E.Id = ".$_GET['eid'].")";
$event = dbFetchOne($eventsSql);
Again its great to see code being contributed to trunk and thanks again to eyeZM for doing so, but having it enabled by default like this certainly opens a lot of security holes.
I am not all talk however and it is certainly not eyeZM's job to fix it, so I will try to get a patch together to fix most of the major issues I see. Overall shell execute calls from php should only be used when absolutely necessary, and for mysql prepared statements would be best but given most of the doesn't use them making sure to use the proper validators and dbEscape would take a second place.