From 1677470af1eb47c606ce07edd34d20ed3d9ebca2 Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Wed, 7 Oct 2026 08:09:33 -0400 Subject: [PATCH] fix: require System edit to change a monitor's event commands refs GHSA-vcrj-35c6-g97c zmc runs a monitor's EventStartCommand and EventEndCommand at the start and end of each event. Both the classic save and bulk edit actions and the API monitor add and edit authorized only on the monitor, so a user with edit on one monitor and no System permission could set a command for zmc to run as the ZoneMinder service user. Changing either field now needs System=Edit, as Filter::canEdit() already requires for AutoExecuteCmd. Monitor::commandFieldChangesNotAllowed() reports the command fields a change would alter; submitting the current value is not a change, so other users can still save the rest of the monitor. The monitor form shows the fields read-only to them. Also close the EventEndCommand list item. Co-Authored-By: Claude Opus 5.5 --- web/api/app/Controller/MonitorsController.php | 20 +++++++++++++++++++ web/includes/Monitor.php | 19 ++++++++++++++++++ web/includes/actions/monitor.php | 7 +++++++ web/includes/actions/monitors.php | 6 ++++++ web/skins/classic/views/monitor.php | 10 +++++++--- 5 files changed, 59 insertions(+), 3 deletions(-) diff --git a/web/api/app/Controller/MonitorsController.php b/web/api/app/Controller/MonitorsController.php index 42a5ac3df..45c3ea774 100644 --- a/web/api/app/Controller/MonitorsController.php +++ b/web/api/app/Controller/MonitorsController.php @@ -126,6 +126,17 @@ class MonitorsController extends AppController { * * @return void */ + // The EventStartCommand / EventEndCommand values the request supplies, if any. + private function requestCommandFields() { + require_once __DIR__ .'/../../../includes/Monitor.php'; + $fields = array(); + foreach (ZM\Monitor::$commandFields as $field) { + $value = $this->requestField('Monitor', $field); + if ($value !== null) $fields[$field] = $value; + } + return $fields; + } + public function add() { if ( $this->request->is('post') ) { @@ -135,6 +146,12 @@ class MonitorsController extends AppController { throw new UnauthorizedException(__('Insufficient privileges')); return; } + // A new monitor's command fields start empty, so any value is a change. + require_once __DIR__ .'/../../../includes/Monitor.php'; + $new_monitor = new ZM\Monitor(); + if (count($new_monitor->commandFieldChangesNotAllowed($this->requestCommandFields()))) { + throw new UnauthorizedException(__('Setting the event commands requires System edit permission')); + } $this->pinRequestId($this->Monitor, null); $this->Monitor->create(); if ($this->Monitor->save($this->request->data) ) { @@ -178,6 +195,9 @@ class MonitorsController extends AppController { throw new UnauthorizedException(__('Insufficient Privileges')); return; } + if (count($zm_monitor->commandFieldChangesNotAllowed($this->requestCommandFields()))) { + throw new UnauthorizedException(__('Changing the event commands requires System edit permission')); + } $this->pinRequestId($this->Monitor, $id); $message = ''; diff --git a/web/includes/Monitor.php b/web/includes/Monitor.php index 01c4a3945..cc12b9cb0 100644 --- a/web/includes/Monitor.php +++ b/web/includes/Monitor.php @@ -953,6 +953,25 @@ class Monitor extends ZM_Object { return $this->connKey; } + // Fields whose value zmc runs as a command. Setting one is equivalent to shell access + // on the server, so it needs System=Edit, not just edit on this monitor, the same rule + // Filter::canEdit() applies to AutoExecuteCmd. refs GHSA-vcrj-35c6-g97c + public static $commandFields = array('EventStartCommand', 'EventEndCommand'); + + // The command fields $data would change on this monitor that the user may not set. + // Submitting the current value is not a change, since the monitor form always posts it. + public function commandFieldChangesNotAllowed($data, $u=null) { + global $user; + if (!$u) $u = $user; + if (!$u or $u->System() == 'Edit') return array(); + $refused = array(); + foreach (self::$commandFields as $field) { + if (!array_key_exists($field, $data)) continue; + if ((string)$data[$field] !== (string)$this->{$field}()) $refused[] = $field; + } + return $refused; + } + function canEdit($u=null) { global $user; if ($u===null or $u->Id() == $user->Id()) diff --git a/web/includes/actions/monitor.php b/web/includes/actions/monitor.php index 93ac0dadd..c82720bee 100644 --- a/web/includes/actions/monitor.php +++ b/web/includes/actions/monitor.php @@ -153,6 +153,13 @@ if ($action == 'save') { if (!empty($newMonitor['ModelId']) and empty($newMonitor['Model'])) unset($newMonitor['Model']); + $refused = $monitor->commandFieldChangesNotAllowed($newMonitor); + if (count($refused)) { + $error_message .= 'Changing '.implode(', ', $refused).' requires System edit permission.
'; + ZM\Warning('Refused to save monitor '.$mid.': '.implode(', ', $refused).' requires System edit permission'); + return; + } + $changes = $monitor->changes($newMonitor); ZM\Debug('Changes: '. print_r($changes, true)); $restart = false; diff --git a/web/includes/actions/monitors.php b/web/includes/actions/monitors.php index b059c404d..b3c45df4c 100644 --- a/web/includes/actions/monitors.php +++ b/web/includes/actions/monitors.php @@ -33,6 +33,12 @@ if ($action == 'save') { continue; } $Monitor = new ZM\Monitor($mid); + // Before stopping it, so a refused save leaves the monitor running. + $refused = $Monitor->commandFieldChangesNotAllowed($_REQUEST['newMonitor']); + if (count($refused)) { + $error_message .= 'Changing '.implode(', ', $refused).' requires System edit permission.
'; + continue; + } if ($Monitor->Type() != 'WebSite') { $Monitor->zmcControl('stop'); } diff --git a/web/skins/classic/views/monitor.php b/web/skins/classic/views/monitor.php index 75728c99c..4fcdb45bb 100644 --- a/web/skins/classic/views/monitor.php +++ b/web/skins/classic/views/monitor.php @@ -1328,14 +1328,18 @@ echo htmlSelect('newMonitor[OutputContainer]', $videowriter_containers, $monitor +
  • - + />
  • - -
  • + /> +