Snapshots index/view/associations returned each snapshot's events without
a monitor check, and Snapshots and Tags add/edit attached any event ids,
so a user could add a denied monitor's event to a snapshot and then read
it back. Attaching now needs view on each event, listed events are
filtered to viewable monitors, and ids are pinned. Tags' existing
filters also match nothing, rather than everything, for a user denied
every monitor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Group membership feeds per-monitor access through Groups_Permissions, but
the classic group actions and the Groups API checked only the global
Groups permission. A Groups editor could add a monitor they are denied to
a group they have access through, or remove it from, or delete, the group
that denies it.
Add Group::canEditMembership() and require it, in both the classic UI and
the API, for every monitor added to or removed from a group, for all of a
group's monitors when it is re-parented, and for all of them when it is
deleted. The API also pins the record id and omits monitors the user may
not view from the groups it lists.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LogsController::delete() never declared global $user, so its System=Edit
check always passed and anyone with System view could delete log entries.
Logs add, which ZM_LOG_INJECT opens to non-admins, could overwrite an
existing entry by Id; pin it.
ZonePresetsController had no permission checks. Reading presets stays
open to signed-in users; changing them now needs System=Edit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The API alarm action and ajax/alarm.php checked no per-monitor
permission and relied on zmu, which only requires that the user can see
the monitor. A user with view on a monitor could force, cancel or disable
its alarms. Changing alarm state now needs Monitor::canEdit(), and the
API status query needs canView().
Monitors API edit and add also pin the record id, so an Id in the body
cannot redirect the save to another monitor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- index/view, Frames index and EventData index/view treated an empty
viewable-monitor list as no restriction, so a user denied every monitor
saw all of them. They now match nothing in that case.
- search and consoleEvents had no monitor filter at all and returned
events and per-monitor counts for denied monitors.
- add saved any MonitorId; it now needs view on that monitor, as editing
an event does, and cannot update an existing event named in the body.
- edit checked the event in the URL but saved the body, which could name
another event's Id or move the event to a denied monitor. Pin the id and
check a new MonitorId.
- createThumbnail and getMaxScoreAlarmFrameId were public, so routable,
and checked nothing. They are internal helpers; make them private.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AppController mapped index/add/edit/view/keyvalue/category to Crud
actions, and CrudControllerTrait answers any action a controller does not
define with them. Those generic handlers apply none of the controller's
permission or per-monitor checks: zones/view/<id> and zones/<id> returned
any zone, including those of monitors the user is denied, and Controls
add/edit and Configs add were reachable the same way. Map no Crud actions,
so an undefined action is a 404, and give ZonesController a view() that
checks the zone's monitor.
ZonesController::index() passed its monitor filter as a find() option key
rather than a condition, so it was ignored and every zone was listed. Use
a real condition.
Add AppController helpers the following fixes share: a viewable-monitor
find() condition that matches nothing when the user may view no monitor
(callers treated an empty list as unrestricted), reading a field or
associated ids from request data, and requiring view or edit on a monitor
or view on events.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_AUTH_HASH_SECRET signs the JWT access and refresh tokens, and its
default is a fixed string in the public source. Nothing generated a
per-install value and tokens signed with the default verified normally,
so on an install with auth on and the secret untouched anyone could sign
an admin token and be accepted by the web UI, the API and zms.
- ZoneMinder::Config::saveConfigToDB() now replaces an empty or default
secret with 32 random bytes from /dev/urandom, hex encoded. Package
installs and upgrades run zmupdate.pl -f, which saves the config, so
existing installs get a secret on upgrade. A secret the admin set is
left alone.
- validateToken() in PHP and zmLoadTokenUser() in C++ refuse to verify
tokens while the secret is empty or the default, and the API refuses to
issue them, as it already did for an empty secret.
- zmLoadTokenUser() no longer writes the signing key to the debug log.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CakePHP's Model::set() takes the record id from a primary key in the
data passed to save(). edit() authorized the id in the URL and then saved
the request body, so Zone[Id]=<other> in the body wrote to that other
zone, past the per-monitor check just added. add() could likewise update
an existing row instead of creating one.
Add AppController::pinRequestId(), which drops the primary key from the
request data and sets the model id, and use it in these edits (pinned to
the URL id) and adds (cleared). Frames and EventData edit() never set the
model id at all, so a body without an Id inserted a new row rather than
updating; pinning fixes that too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZonesController add/edit/delete checked only the global Monitors
permission, so a Monitors=Edit user denied a monitor could still add,
rename, reshape or delete that monitor's zones by id.
Each now requires Monitor::canEdit() on the zone's current monitor, and on
the MonitorId in the request data for add and for an edit that moves the
zone. add() rejects a request without a MonitorId.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FramesController::add() and EventDataController::add() saved request data
behind only the controllers' Events != None gate, so an Events=View user
could create rows for any event, including one on a monitor they are
denied. edit() checked the existing row but not the event or monitor the
request moved it to.
add() now requires Events=Edit and edit on the event named by EventId,
which covers that event's monitor. For EventData a supplied MonitorId must
also be viewable. edit() applies the same check to any EventId/MonitorId
in the request. Load includes/Event.php explicitly rather than relying on
the model association to have pulled it in.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
712994b19 stopped writing sessions for requests that arrive without a
ZMSESSID cookie unless zm_session_persist() marks them as being issued.
Web login goes through zm_session_regenerate_id_login(), which marks the
session, but the legacy API login with stateful=1 authenticates via
validateUser() and only calls zm_session_start(). A client logging in that
way without a cookie was handed a ZMSESSID cookie for a session that was
never stored, so its following cookie-only requests were unauthenticated.
Mark the session persistent once the stateful login has a user.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fileSize() joined the stored DefaultVideo onto the event directory as is,
so an older row, or one written before the model validation was added,
holding ../ could stat a file outside it. fileExists() above already
applies basename(); do the same here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DefaultVideo can be set by any user with Events=Edit through the API
(EventsController add/edit), and readers join it onto the event path:
view_video.php streams it, image.php extracts frames from it,
findVideoEventFile, Event::getStreamSrc/FileSize and the API fileExists
check all use Path().'/'.DefaultVideo. A value such as ../../x pointed
those reads at arbitrary files readable by the web account.
Reject DefaultVideo values on save in the API model unless they are a
bare filename (no / or \, no NUL, not . or ..). Make the PHP Event
DefaultVideo() accessor return basename() so every web reader only ever
looks inside the event directory, even for rows written before this
check, and apply basename() to the raw array read in the API model's
fileExists.
Event::GenerateVideo built its output filename from Name with only
whitespace replaced; Name is editable through the event rename and
eventdetail actions. Replace anything outside [-A-Za-z0-9_.] and a
leading dot so the output stays inside the event directory.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit f3f679b644db28b4b1bff79d5dd98eca24395fc4)
With ZM_AUTH_HASH_IPS on, the client address bound into the auth hash was
taken from the left-most X-Forwarded-For value whenever the header was
present, both when PHP generated the hash (getRemoteAddr()) and when PHP or
zms validated it (getAuthUser(), zmLoadAuthUser()). The header is client
controlled, so anyone holding a leaked hash could replay it from anywhere by
sending the address it was bound to.
Add ZM_AUTH_TRUSTED_PROXIES, a list of exact reverse proxy addresses.
X-Forwarded-For is now used only when REMOTE_ADDR is one of them, and is read
from the right, skipping hops that are themselves listed proxies, so values a
client prepends are never chosen. With the option empty, the default, the
header is ignored and REMOTE_ADDR is used.
PHP (web/includes/Network.php getRemoteAddr()) and C++ (ClientAddress() in
zm_utils, used by zmLoadAuthUser()) implement the same rule so generation and
validation continue to agree. Every PHP caller already routes through
getRemoteAddr(), so session.php and auth.php need no change.
Reverse proxy users who enable ZM_AUTH_HASH_IPS must list their proxy in the
new option; until they do, hashes bind to the proxy address, which still
validates but no longer distinguishes clients. This is the behaviour change
that the #4921 work avoided by trusting the header.
The option is added through ConfigData only, like other recent options;
zmupdate.pl --freshen inserts it, zms falls back to the compiled-in default and
PHP treats an undefined constant as empty, so no schema migration is needed.
Tests: ClientAddress Catch2 case; tests/php/test_remote_addr.php updated for
the trusted-proxy rule.
refs GHSA-72rf-54rm-798c
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit acea5889dec0826f595cb736147a5fdc9c94a2a9)
/api/host/login took the token's subject from the `user` request parameter and
authenticated the request from a different one. beforeFilter() validates
user=/pass=, and zm_authenticate_request() logs in from username=/password=,
but nothing ever checked that the two named the same account. A caller could
authenticate with their own credentials and ask for a token issued to someone
else.
Reproduced end to end against a live instance. A System=None, Events=View
account posting
username=lowpriv&password=testpass123&user=admin
received an access and a refresh token whose claims read {"user":"admin"}, and
that token was accepted by a System-gated endpoint. Any enabled API account
escalated to administrator without knowing the administrator's password or
ZM_AUTH_HASH_SECRET.
The subject now comes from the authenticated user rather than from the request,
and a request that reached this point without authenticating is refused instead
of being handed a token for whoever it named.
Verified after the change on the same instance: the request above mints a token
for lowpriv, an ordinary user=/pass= login still returns tokens for the caller,
and the refresh-token path still issues a new access token.
See GHSA-m77q-66v7-j3fq.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WBHBB95RBX7D9p8ge2WDZb
(cherry picked from commit 72cb485655c53babfcfecaed572e0ef24e96db06)
The cache prefix was still the CakePHP skeleton default 'myapp_'. After an
upgrade that alters the schema, CakePHP could serve model and schema caches
written by the previous version. cmake substitutes @VERSION@ when generating
app/Config/core.php, so each version gets its own cache namespace.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FilterComponent::buildFilter() checked the operator against an allow list
but never the field, so an unrecognised name went straight into the SQL:
GET /api/events/index/MonitorName =:Monitor-1.json
PDOException SQLSTATE[42S22]: Column not found: 1054
Unknown column 'MonitorName' in 'where clause' -> HTTP 500
A caller filtering on a monitor-scoped attribute (MonitorName, Monitor,
Rack, Experiment, Server, Storage) or simply mistyping a column got an
opaque 500 with a stack trace and nothing naming the offending field.
Take an optional list of permitted fields and reject anything else with
BadRequestException. EventsController::index() passes the Events columns
from the model schema plus the two names it resolves itself: DateTime, the
pseudo-attribute it turns into an overlap test, and GroupId, served by the
Groups_Monitors join. A field written Model.Field is checked on the column.
The two other callers of buildFilter pass no list and are unchanged.
The invalid-argument and invalid-operator paths threw a plain Exception,
which was also a 500 for what is bad input; they throw BadRequestException
now too.
Verified against a live install:
MonitorName / Rack / NotAColumn 400, "Unknown filter field: <name>"
DateTime >=/<= over a window 200, 7 rows, overlap intact
Cause, Event.Cause, GroupId 200
?limit=2&sort=...&direction=desc 200, 2 rows, pagination unaffected
Plus an assert script over the validation itself, including that a caller
passing no list keeps the previous pass-through behaviour.
Filters sent as a query string rather than named parameters go through a
different branch, on raw $_REQUEST, and are deliberately left unvalidated:
that array also carries auth and cache-busting parameters, so rejecting
unknown names there would break existing callers. ?MonitorName=x is still
a 500 and wants its own change.
auth.php ended in a 130-line block at file scope, so merely including the file
authenticated the current request: it read $_REQUEST, opened a session, queried
the database, and on a login could rewrite the user's stored password hash and
populate $_SESSION. Any caller that wanted one of the functions in the file got
all of that as a side effect, and the order of includes decided when it ran.
HostController requires auth.php twice purely to reach generateAuthHash() and
validateToken().
Move the block into zm_authenticate_request() and call it explicitly from the
two places that want it, web/index.php and AppController::beforeFilter(). The
function returns the ZM\User or null and still sets the global $user, so the
views, ajax handlers and API controllers that read that global are unaffected.
HostController now gets only the function definitions from its requires, which
is all it ever wanted.
Inside a function the five `unset($user)` calls would drop the local binding
and leave the global set, so they become `$user = null` - the idiom the rest of
the file already uses for this, and one that keeps isset($user) false for the
gate at index.php:255. The block's other locals ($ret, $username, $password,
$sql) no longer leak into the caller's scope, which in beforeFilter() means they
can no longer collide with the variables of the same name it assigns just after.
The body is otherwise unchanged; `git diff -w` shows only the wrapper, those
five assignments and the return.
Tests: tests/php/test_auth_no_include_side_effects.php tokenises auth.php and
asserts nothing executes at file scope, with a fixture check so a broken
detector cannot pass vacuously. 4 assertions, all pass. Verified it reports the
pre-refactor file's file-scope block, so it would have caught this.
Not covered by tests: the login, logout, auth-hash and API token flows this
touches. auth.php cannot be included without a database (User.php pulls in
database.php, which connects at include time), so the check is structural.
Needs manual testing on an installed tree before merging.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01477mR97vfnK6zczbHgzq6T
A preference belongs to the user who set it, so ownership is the permission.
The controller instead required System, which got it wrong in both directions:
any System account could read and overwrite another user's preferences, and an
ordinary user could not save their own at all, which is what the montage layout
in montage_common.js does through this controller.
Reads and writes are now scoped to the caller. index lists only the caller's
rows, view, edit and delete refuse a row belonging to someone else, and add pins
UserId to the authenticated user: the client sends its own id in the body, so
taking that on trust let anyone write a preference onto another account. A
System Edit account may still manage anyone's, so an administrator can clear a
broken one.
The find conditions named the table, User_Preferences, where CakePHP wanted the
model alias, UserPreference, so view and edit returned a 500 for everyone. That
is fixed here because it otherwise hides whether the ownership check works.
Neither checked anything, so any enabled, API-enabled account could read them.
Control definitions describe how to drive a camera and are edited under Options,
which requires System. Manufacturers, CameraModels and EncoderTemplates already
gate their reference data that way; Controls was left open.
getLoad returns the system load average, which getSysLoadHTML() in the web ui
declines to render without canView('System'), so the API no longer hands the
same figure to an account without it.
getDiskPercent reported usage per monitor, keyed by monitor name, telling an
account which monitors exist and how much disk each uses even when it was not
permitted to view any of them. It now reports only on monitors the caller can
view, and asking about one specific monitor requires view on that monitor.
daemonCheck is deliberately left alone: it reports only whether ZoneMinder is
running, which the web ui's navbar shows to every logged-in user, and gating it
here would make the API stricter than the interface it serves.
daemonStatus() and daemonControl() checked nothing at all. Any enabled,
API-enabled account could read the state of, and start or stop, the capture and
analysis daemons of any monitor, including monitors it was not permitted to see.
index() and view() have always filtered on ZM\Monitor::canView(), so these two
were the odd ones out.
Confirmed on a live instance with a user whose every permission was None:
monitors.json returned an empty list, while
monitors/daemonControl/1/status.json ran zmdc against that same monitor. On a
running system the equivalent stop call would have halted recording.
Reporting state now needs view on the monitor, and starting or stopping it needs
edit, matching the rest of the controller.
zmdc.pl takes the daemon and the command as separate arguments and they were
interpolated into the command line, escaped only by escapeshellcmd() over the
whole string. That stops metacharacters but not extra arguments, so both are now
checked against the set of names zmdc accepts.
add() and delete() call this internally, having already made their own
permission decision, and a System Edit account need not hold Monitors Edit. They
call an ungated private runner so that re-checking here cannot stop them.
Any enabled, API-enabled account could read the effective configuration,
including secrets, whatever its permissions. Confirmed on a live instance: a
user with every area set to None retrieved ZM_DB_PASS from
/api/configs/viewByName/ZM_DB_PASS.json.
The controller already required System Edit to change a config but checked
nothing at all to read one, so index, view, viewByName and categories were open
to anyone the API let in. They now require the same System permission the web
ui's Options page requires.
Permission alone is not enough, because nothing outside the server has any use
for these values. Two kinds are now withheld from every caller, administrators
included:
- Rows flagged Private in the Config table. That flag already existed and was
loaded into $zm_config, but nothing ever acted on it. It covers
ZM_AUTH_HASH_SECRET, which is enough to forge an authentication hash for any
user, and the reCaptcha secret.
- The database credentials. These are read from zm.conf and conf.d rather than
the table, so they have no row to flag and are listed by name. This is also
why filtering the table alone would not have been enough: index appends the
file-backed values to its response.
Requesting an unknown name logged print_r($zm_config, true), copying the whole
effective configuration into the log and anything collecting it. It now logs the
name at Debug.
Verified against a live instance, before and after, with a temporary System=None
API user: reading ZM_DB_PASS returned the password and now returns 401; an
administrator gets an empty Value for ZM_DB_PASS and ZM_AUTH_HASH_SECRET, the
293-entry index carries no secret values, and ordinary settings such as
ZM_WEB_TITLE still read normally.
DateTime is a pseudo-attribute meaning "the event was running then", so a
window over it should select events that overlap the window. The code applied
the term's own operator to both StartDateTime and EndDateTime, which turns the
upper bound into StartDateTime <= max AND EndDateTime <= max -- a containment
test. Any event spanning the end of the window was dropped, which under
continuous recording is most of them, and with events longer than the window,
all of them. Montage review showed an empty timeline as a result.
The lower bound now tests the event's end and the upper bound its start.
The EndDateTime IS NULL allowance is also bounded. It was there so an event
still being written stays visible, but as written it made every crash-orphaned
event ever recorded match every window: montage review was returning events
from two weeks earlier and nothing from the requested hour. An event with no
EndDateTime still has Length, flushed every few seconds by zmc, so
StartDateTime + Length is its effective end; only an event with neither falls
back to NOW(). This is the same expression the Event model already uses for
its EndTimeSecs virtual field.
Verified against a live instance: for a 09:43-10:43 window that had one
overlapping event per monitor, the API returned 8 events for monitor 1, all
crash orphans from 2026-07-26 and none from the window. It now returns the
overlapping event, and montage review draws it and renders its frames.
EventDataController and TagsController both authorized only with the
account-wide Events permission in beforeFilter() (Events != None) and
never applied the per-monitor ACL that EventsController resolves via
unviewableMonitorIds()/viewableMonitorIds(). A monitor-restricted account
could therefore read data belonging to cameras it is explicitly denied.
EventDataController::index() and ::view() now filter on Event_Data.MonitorId
(the table carries its own MonitorId, so the restriction applies directly,
mirroring EventsController's Event.MonitorId filter). ::view() returns 403
when the row exists but is outside the caller's allowed monitors, matching
EventsController. ::edit() and ::delete() now go through requireEventDataEdit(),
which requires Events=Edit plus the per-monitor ACL on the target row, so a
restricted or view-only account cannot mutate or delete another monitor's
event data by Id. The pre-existing view/edit reads also stop hardcoding a
"Event_Data." alias prefix that does not match the model alias, using
$this->EventData->alias instead.
TagsController exposes per-monitor data only where a tag is joined to its
events: index() filtered by Events.Id (returns Events_Tags.EventId) and
associations() (contains each tag's Events). Both now restrict the joined
Events to viewable monitors when the caller is monitor-restricted. The plain
tag list is a global label vocabulary shared across monitors and is left
unrestricted so restricted users can still tag their own events.
This is the same defect class as GHSA-mg2g-jmfc-3w8g (FramesController),
reported here as GHSA-rgwp-wvjw-6925 for EventDataController; the Tags
controller was found to share the shape during the audit the report
suggested.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FramesController::index() computed $mon_options (the caller's per-monitor
restriction from unviewableMonitorIds()/viewableMonitorIds()) but never
applied it to the find() conditions, so any authenticated user with
Events=View could enumerate Frame rows (Id, EventId, MonitorId, TimeStamp,
Delta, Score, Type) for monitors they are explicitly denied via
GET /api/frames.json. Every sibling controller (EventsController,
ZonesController) and every other action in this same controller
(view/edit/delete, via eventForFrame()/requireFrameEdit()) already enforce
this restriction; index() was the one path left over from before the
per-monitor ACL helpers were added.
The naive fix of merging Event.MonitorId into $conditions the way
EventsController does does not work here: Frame belongsTo Event via
EventId, index() sets $this->Frame->recursive = -1, and Frame's own table
has no MonitorId column, so the condition can't resolve without a join.
Add an explicit inner join to Events (aliased Event) on
Event.Id = Frame.EventId whenever the caller has a monitor restriction,
and filter on Event.MonitorId, mirroring the explicit-join pattern
EventsController already uses for Tags.
Verified against the production database via a temporary CLI script
exercising the exact query-building logic: unrestricted find() returns
all ~110.5M frames (matching the pre-fix behaviour), while restricting to
a single monitor returns only that monitor's frames (~2.9M, cross-checked
row by row against the owning Event's MonitorId), and restricting to a
monitor with no events correctly returns zero. Reported as
GHSA-mg2g-jmfc-3w8g.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HoNiNxwgyaHV29CbiueCUf
Several API endpoints checked only the coarse Events/Monitors permission
and not the per-monitor object ACL, so a user explicitly denied a monitor
could still reach that monitor's objects by addressing them directly:
- EventsController::edit() and ::delete() checked Events=Edit but never
called canEdit() on the event, so any event could be mutated or deleted
by Id.
- FramesController only guaranteed Events != None in beforeFilter().
view() returned any frame by Id, and edit()/delete() mutated frames
without requiring Events=Edit or checking the parent event at all.
- ZonesController::forMonitor() listed zones for any monitor Id.
Resolve the owning object and apply the same canView()/canEdit() checks
the normal read paths already use. Frames are addressed by their own Id,
so their parent event is looked up to reach the monitor ACL.
Refs GHSA-hw39-qpjw-p7cg.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A capture crash can leave events with no frames, no Length and no
EndDateTime. The API's EndTimeSecs virtual field reported these with an
end of NOW(), the fallback meant for a genuinely in-progress recording,
so every orphan appeared to span from its start until now.
On a monitor with hundreds of these, the events all overlapped: at a
typical timestamp 313 events covered the same instant. findEventByTime is
a binary search that assumes non-overlapping ordered events, so it
returned an essentially arbitrary one, and zms then streamed a different
event than the JS believed was playing. That is the erratic playback and
event/stream mismatch seen in montage review.
Two changes:
- Event model EndTimeSecs: for an event with no EndDateTime and Length 0,
report end = StartDateTime (zero duration) instead of NOW(). An event
with no length has no span to occupy, so it no longer overlaps others.
- montagereview receive_events: skip events with no frames. There is
nothing to review in an empty event, and dropping them keeps the
timeline and event selection to real recordings.
The same NOW() fallback exists in web/ajax/events.php and
web/skins/classic/views/montagereview.php; those paths are not used by
montage review's event load and are left unchanged.
Verified on a monitor with 521 orphaned events: overlapping events at a
given instant dropped from 313 to 2, and the streamed event now matches
the selected event with sub-second drift.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The config-api RCE fix (b036408a5) guards edit() and delete() with
($user['System'] == 'Edit'), but $user is a ZM\User object that does
not implement ArrayAccess and whose System property is protected. Array
access on it raises "Error: Cannot use object of type ZM\User as array",
so the endpoint fatals with HTTP 500 for every authenticated user. This
blocks the RCE only by accident and also breaks config editing for
legitimate System=Edit admins.
Use $user->System(), matching the idiom in every other API controller
(States, Servers, Monitors, Users, etc.).
Refs GHSA-mvj8-mqqq-2w5f.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The REST API archive action toggled an event's Archived
(retention-protection) flag with no authorization beyond the
controller's coarse "Events permission is not None" gate. Any
authenticated read-only user, including one restricted to a subset of
monitors, could flip the retention state of any event by enumerating
event ids, and the action was reachable over GET (CSRF-able).
Gate the write by direction: archiving (protects from purge) requires
view access via Event::canView(); un-archiving (re-exposes to purge)
requires edit access via Event::canEdit(). Both enforce the per-monitor
object-level ACL. Restrict the action to POST/PUT to block CSRF.
Addresses GHSA-5v9h-ww7p-hxgv.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When an event closes the recording file is renamed from incomplete.* to
<Id>-video.* and the DB row is updated. A stale Event model still reports
DefaultVideo=incomplete.mp4, producing spurious 'File does not exist'
warnings. When the incomplete file is missing, clear the object cache and
re-read the event from the database to check the finished file before
warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CakePHP applies the datasource 'encoding' as SET NAMES, and it was 'utf8',
MySQL's 3-byte utf8mb3 alias. Like the C++ daemon connection, this mangles
4-byte UTF-8 characters in utf8mb4 columns such as Monitors.Name to '?' on
read and truncates them on write, so the API returned and stored corrupted
names. Set it to utf8mb4 to match the schema.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- API (database.php.default): only set the PDO verify flag when SSL is
actually configured (ZM_DB_SSL_CA_CERT set), matching the web/Perl/C++
layers. Previously a fresh install's default (1) would set the flag on a
non-SSL connection, since the CakePHP datasource merges 'flags' uncondi-
tionally.
- Both PHP layers: cast to string and trim before parsing the value, and use
strict in_array, to avoid type-juggling and stray-whitespace edge cases.
- zm_db.cpp: use my_bool (not char) for the MYSQL_OPT_SSL_VERIFY_SERVER_CERT
fallback argument, the type libmysqlclient expects. That branch only
compiles on older clients without MYSQL_OPT_SSL_MODE, where my_bool exists.
refs #3816
Add a ZM_DB_SSL_VERIFY_SERVER_CERT setting so a database connection that uses
ZM_DB_SSL_CA_CERT can talk to a server with a self-signed or otherwise
non-matching certificate. When enabled, verification is by identity (the cert
must chain to the CA and its CN/SAN must match ZM_DB_HOST), consistent across
the C++ daemons, the PHP web interface, the CakePHP API and the Perl scripts.
This re-does the reverted #3817. That PR broke the build because it called
mysql_options(MYSQL_OPT_SSL_VERIFY_SERVER_CERT, ...), and that enum was removed
from the MySQL 8.0 C client in favour of MYSQL_OPT_SSL_MODE; it also passed a
c_str() where a my_bool* was expected, and referenced the PHP constant
unconditionally (fatal on PHP 8 for an upgraded install whose zm.conf predates
the option).
The option that controls server-cert verification differs by client library and
the symbols are enum values, not macros, so CMake feature-detects them by
compiling:
- HAVE_MYSQL_OPT_SSL_MODE (MySQL 5.7.11+/8.0, MariaDB Connector/C 3.1+)
- HAVE_MYSQL_OPT_SSL_VERIFY_SERVER_CERT (older MariaDB/MySQL)
zm_db.cpp uses SSL_MODE_VERIFY_IDENTITY / SSL_MODE_REQUIRED when the former is
available, else falls back to the latter with a proper my_bool.
Value handling is three-way in every layer: a truthy value verifies, a false-y
value (0/false/no/off) skips verification, and an empty/unset value leaves the
client default in place so existing installs are unchanged on upgrade. PHP, the
API datasource (via PDO flags) and the Perl DSN are all guarded with defined()
checks. Fresh installs default to 1.
Documents the full ZM_DB_* connection and SSL settings, including the hostname
verification gotcha when connecting by IP, in docs/userguide/configfiles.rst.
refs #3816
The monitors index unconditionally LEFT JOINed Groups_Monitors and collapsed
the duplicate rows (monitors in multiple groups) with GROUP BY `Monitor`.`Id`.
That GROUP BY fails under ONLY_FULL_GROUP_BY on engines without functional
dependency detection (MariaDB), raising 1055 'Monitor.Name isn't in GROUP BY',
so /api/monitors.json returned a 500 while /api/monitors/<id>.json worked.
Only join Groups_Monitors when the request filters by group (matching the
existing EventsController pattern and the original commented-out intent), drop
the GROUP BY, and dedupe by monitor Id in the existing result loop to cover
multi-value GroupId filters. Portable across MySQL and MariaDB.
refs #3633
The view() action sets recursive=1 on the Event model, which the
subsequent find('neighbors') calls inherited. That made each of the four
neighbor lookups (prev/next, prevOfMonitor/nextOfMonitor) SELECT every
column from Events plus LEFT JOIN Monitor and Storage, then fire a
separate Frames hasMany query per neighbor row. Only Event.Id is used
downstream.
Pass fields=Event.Id and recursive=-1 on each neighbor call so the
generated SQL is just:
SELECT Event.Id FROM Events AS Event WHERE Event.Id < ?
ORDER BY Event.Id DESC LIMIT 1
The per-monitor variant uses Events_MonitorId_idx which already covers
(MonitorId, Id) via InnoDB's implicit PK suffix, so no schema change is
needed.
When zmc is killed or crashes without writing EndDateTime, three code
paths invent a fake end of NOW(), so an event from hours ago appears to
extend across the entire down-time. Montage review then paints a bar
that makes it look like recorded video exists where it doesn't.
Length is flushed to the DB every few seconds during recording, so even
crashed events have an accurate last-known duration. Fall back to
StartDateTime + Length when EndDateTime IS NULL, and only fall back to
NOW() when Length is also 0 (event has no recorded data yet).
- web/api/app/Model/Event.php: EndTimeSecs and EndTime virtual fields,
which is what the montagereview JS actually reads via the API.
- web/ajax/events.php: same fix in the AJAX events list SQL.
- web/skins/classic/views/montagereview.php: \$eventsSql kept in sync
even though it is no longer executed directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- AppController.php: stop overwriting $_SESSION['remoteAddr'] with bare
REMOTE_ADDR right after zm_session_start() already populated it from
HTTP_X_FORWARDED_FOR. The clobber bound generated hashes to the proxy
IP, but getAuthUser() validates against XFF, so any hash produced
inside the legacy stateful API path was DOA behind a reverse proxy.
- getAuthUser(): prefer the URL user= parameter over
\$_SESSION['username'] for filtering, matching what zms's
zmLoadAuthUser does, and honor ZM_CASE_INSENSITIVE_USERNAMES on the
primary filter. Warn when the URL user= disagrees with the session
username (stale hash, cross-tab contamination, or tampered request).
- Add a Debug input dump on entry and an Info-level failure line that
reports filterUser, XFF, REMOTE_ADDR, rowsTried and the TTL window so
the next 401 surfaces which input is wrong.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a curated, per-encoder parameter-template library to ZoneMinder:
- Monitor edit page: a new Template row above the EncoderParameters
textarea offers per-encoder templates (Balanced / Archival / Low
Power / Low CPU). Apply merges the template's params into the
textarea, preserving user-only keys. Advisory lint flags option
keys that aren't recognised for the selected encoder. Switching
encoders offers a same-name template on the new encoder via a
native confirm.
- Options page: a new Encoder Templates tab with full CRUD —
list / edit / copy / delete — backed by a new CakePHP REST API
at /api/encoder_templates.
- Storage: a new EncoderTemplates DB table seeded with 14 shipped
defaults across libx264 / libx265 / h264_nvenc / hevc_nvenc /
h264_vaapi / hevc_vaapi. The table is mutable; ZM upgrades do not
re-seed user-edited rows.
- valid_keys (the lint allow-list) stays in PHP code as ffmpeg
vocabulary, not user data.
- Default params explicitly include pix_fmt to avoid the yuvj420p
HEVC HW-decode rejection issue we hit earlier.
No C++ change. The textarea content is parsed by the existing
av_dict_parse_string call in src/zm_videostore.cpp.
version.txt -> 1.39.6.
Specs: docs/superpowers/specs/2026-05-0{1,2}-*.md
Plans: docs/superpowers/plans/2026-05-0{1,2}-*.md
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to 419846c87 (GHSA-g66m-77fq-79v9). The Device path check was
applied to all monitor Types in three places, but the Device column is
only passed to a shell for Type='Local'. Non-Local monitors (Ffmpeg,
Remote, Libvlc, cURL, VNC) may legitimately hold legacy values such as
an RTSP URL in that column and should not be rejected or warned about.
- scripts/ZoneMinder/lib/ZoneMinder/Monitor.pm: control() dropped the
spurious Warning for non-Local monitors that was flooding zmwatch
logs. The Error/early-return path is preserved for Local.
- web/includes/actions/monitor.php: save action only runs
validDevicePath() when Type=='Local'.
- web/api/app/Model/Monitor.php: replaced the unconditional regex rule
with a validDevicePath() method that checks Type before enforcing
the /dev/ pattern.
Also add client-side validation matching the server rule, so Local
monitors get immediate feedback instead of a round-trip error:
- web/skins/classic/views/monitor.php: HTML5 pattern attribute on the
Device input. Escaped for the v-flag regex engine used by pattern=.
- web/skins/classic/views/js/monitor.js.php: validateForm() now also
rejects Device values that don't match the /dev/ pattern.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FilterTerm.php:
- Replace eval() with safe compare() method for SystemLoad, DiskPercent,
and DiskBlocks filter conditions (RCE via crafted op/val)
- Validate operator against allowlist in constructor
- Sanitize collate field to alphanumeric/underscore only (SQLi)
onvifprobe.php:
- Use escapeshellarg() on interface, device_ep, soapversion, username,
and password arguments passed to execONVIF() (command injection)
Event.php:
- Use escapeshellarg() on all arguments to zmvideo.pl instead of
escapeshellcmd() on the whole command (command injection via format)
- Anchor scale regex with ^ and $ to prevent partial matches
image.php:
- Restrict proxy URL scheme to http/https only (SSRF via file:// etc)
filterdebug.php:
- Use already-sanitized $fid instead of raw $_REQUEST['fid'] (XSS)
MonitorsController.php:
- Use escapeshellarg() on token, username, password, and monitor id
in zmu shell command instead of escapeshellcmd() on whole command
HostController.php:
- Use escapeshellarg() on path in du command (command injection via mid)
- Remove space from daemon name allowlist (argument injection)
EventsController.php:
- Remove single quotes from interval expression regex (SQLi)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Device field from the Monitors table was interpolated directly into
shell commands (qx(), backticks, exec()) without sanitization, allowing
authenticated users with monitor-edit permissions to execute arbitrary
commands as www-data via the Device Path field.
Defense in depth:
- Input validation: reject Device values not matching /^\/dev\/[\w\/.\-]+$/
at save time in both web UI and REST API
- Output sanitization: use escapeshellarg() in PHP and quote validated
values in Perl at every shell execution point
Affected locations:
- scripts/ZoneMinder/lib/ZoneMinder/Monitor.pm (control, zmcControl)
- scripts/zmpkg.pl.in (system startup)
- web/includes/Monitor.php (zmcControl)
- web/includes/functions.php (zmcStatus, zmcCheck, validDevicePath)
- web/includes/actions/monitor.php (save action)
- web/api/app/Model/Monitor.php (daemonControl, validation rules)
- web/api/app/Controller/MonitorsController.php (daemonStatus)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Revert accidental Users.RoleId FK change from CASCADE back to SET NULL
- Remove System != 'None' gate in beforeFilter; any authenticated user
can manage their own notifications, per-row ownership checks suffice
- Add allowMethod('post', 'put') guard to edit() for consistent REST behavior
- Change PushState validation from allowEmpty to required=false
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- UserId is now DEFAULT NULL instead of NOT NULL
- FK changed to ON DELETE SET NULL (keep token if user deleted)
- Removed auth guard from add() — no-auth mode stores NULL UserId
- No-auth mode already treated as admin by _isAdmin(), so scoping
works correctly (sees all tokens)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add FOREIGN KEY on UserId -> Users.Id with ON DELETE CASCADE
(both in fresh schema and migration)
- Reject push token registration when auth is disabled
(UserId would be null, violating NOT NULL constraint)
- Add $belongsTo association to User in Notification model
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add RBAC checks to ConfigsController edit() and delete() requiring
System=Edit permission, matching the pattern used by other controllers.
Harden System/Readonly column checks with !empty() to handle missing
columns gracefully. Fix command injection in Event.php by using
ZM_PATH_FFMPEG constant with escapeshellarg() instead of hardcoded
unsanitized ffmpeg call. Add is_executable() validation at all exec()
sites using ZM_PATH_FFMPEG as defense-in-depth against poisoned config
values.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
