EyeZm Code in Trunk security issues.

[mitch]

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:

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);

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:

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);

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.

[jdhar]

Mitch, sorry that I missed this post. We are the maintainers of the code so it is most definitely our responsibility to fix the issues you have pointed out. If you have patches, it would definitely help in speeding things up, but we are going to commit some resources to improving security before the 1.24.3 release. Thanks again for pointing these out!

[zoneminder]

1.24.3 should be considered to be for evaluation only at this point. I would not suggest it's use in live systems until the various issues have been ironed out.

[jdhar]

Security updates have been applied to trunk, please go ahead and check/test them out. Feel free to point out any other vulnerabilities you see.

[mitch]

For some reason I did not get the notifications from the forum or somehow missed the replies. I did a quick check today over it and its still playing with fire (but such is most of the ZM code and I am not sure that will change any time soon). Going the escape route i think you are getting pretty close to being decently safe against normal attacks. The downside to escaping is one slight mistake and you are completely screwed. The issues I saw on quick review:

Code: Select all

Line 28 of actions.php has call for: spawn264 which has an unsafe use of the "br" param:

	It has: $br = validString(getset('br', ZM_EYEZM_H264_DEFAULT_BR)); #Valid string only strips out html(strip_tags) from the string, so quotes, new lines, backticks, etc can go through.
	It then calls: $streamUrl = stream264fn($monitor, $width, $height, $br);
		stream264fn calls:
			$ffstr = getFfmpeg264Str($width, $height, $br, "-", "-");
				getFfmpeg264Str calls:
					$ffparms = getFfmpeg264FoutParms($br, $fout);
						getFfmpeg264FoutParms finally just appends BR onto the command line string: 	$ffparms .= " -vcodec libx264 -b ".$br;
	Finally: $pid = shell_exec($streamUrl);

	Br is not even quoted when past to the console, allowing for direct command execution


kill264 is unauthenicated (anyone can kill off monitors)

Similar issue with the feed function at line 102 with the Br param

The good news is that atleast one must have access to the stream for the shell execution. Sorry I did not actually test the exploit to make sure it would work, but after seeing your reply figured id post what could be a hole.