Changing a user's password rewrote only the password hash and left the
user's JWT API tokens valid: a pre-change access token kept working and a
pre-change refresh token kept minting new access tokens for up to its 24
hour lifetime, so a leaked token survived the password change meant to
cut it off.
Set TokenMinExpiry to the change time in the same step that rewrites the
hash, so validateToken() rejects every token issued before the change,
matching the options page's existing revoke action. Covered paths: the
REST API User model (beforeSave), with the self-edit controller whitelist
widened to persist the field, and the classic web UI user action for both
the administrator edit and the self edit. The login-time upgrade of a
legacy hash to bcrypt rewrites the same password and is left untouched so
it does not revoke tokens.
refs GHSA-5w6w-qp25-2qqh
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CORSHeaders() (web UI) and AppController::beforeFilter() (API) send
Access-Control-Allow-Origin with Access-Control-Allow-Credentials: true.
With no Servers rows and ZM_MIN_STREAMING_PORT set they reflected any
Origin, and with Servers rows they matched the Origin against each
server's Hostname and Name with a regex anchored only at the start, so
https://zm.example.com.evil.net was accepted for zm.example.com.
Both now call corsAllowedOrigin() in Network.php, which accepts an
Origin of the form scheme://host[:port] whose host equals a server's
Hostname or Name, or, without Servers rows and with streaming ports,
the host this request was made to. Any port is accepted, so multi-port
streaming keeps working. Same-origin and unlisted origins get no CORS
headers, as before; the web UI still warns about unlisted origins only
when Servers rows exist.
Tests: tests/php/test_cors_allowed_origin.php
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qualifyFields() required a bare field to be a Monitors column, so names
only a joined table has, such as Status, CaptureFPS and AnalysisFPS from
Monitor_Status, answered 400 where they used to filter:
GET /api/monitors/index/Status:Connected.json -> 400 Unknown filter field
A bare field that Monitors has is still written Monitor.Field. Otherwise
it must be a column of exactly one joined table and is left bare, as
before, for MySQL to resolve; buildFilter() does not accept an alias with
an underscore such as Monitor_Status, so it cannot be qualified. A bare
field no table has stays a 400, and one two joined tables have and
Monitors does not (MonitorId, in Monitor_Status and Event_Summaries) is a
400 "Ambiguous filter field" instead of a 500 from MySQL.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventsController::search() and consoleEvents() threw a plain Exception
for an interval or field name they could not parse, which is a 500.
With the generic message in 5xx bodies the client no longer learned
what was wrong. Throw BadRequestException, as FilterComponent does
since 028ccecb5: a 400 that names the bad value.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MonitorsController::index() passed its named parameters to
FilterComponent::buildFilter() without a field list, so any name made
of letters, digits and dots reached MySQL as a column:
GET /api/monitors/index/Nope:1.json -> 500, PDOException 42S22
and the bare names Id and Name, which the joined Manufacturers and
Models tables have too, failed as ambiguous columns (also 500).
FilterComponent::qualifyFields() checks each field against the columns
of the queried model and of the models find() joins, and writes it
Model.Field: a bare field is a Monitors column. GroupId stays bare for
the Groups_Monitors join. Anything else is a 400 "Unknown filter field",
as events/index answers since 028ccecb5. Column names match without
regard to case and are written as the schema spells them.
The web UI's map (Deleted:0) keeps working;
Name:x, Id >:0, Name LIKE:x and Id IN:1,2 now return monitors instead
of a 500.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With debug 0, Cake replaces a 500's message with a generic one, but
Crud's exception renderer adds data.exception.message from the raw
exception. A PDOException (e.g. monitors/index/<unknown column>:1.json)
returned the SQL error and its fragment of the query to the client.
data.url also echoed the request URL with its token or password.
ZmExceptionRenderer extends Crud's renderer: with debug 0 a 5xx body
carries the generic message in data.exception.message (class and code
are kept), and data.url has credential values redacted.
AppController::beforeFilter() swaps it in for the renderer Crud's
ApiListener registers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventsController::view() looks the event up with the per-monitor
condition and then asks ZM\Event::canView(). For an event on a denied
monitor the restricted find() is empty, the empty ZM\Event has no
monitor, and canView() falls back to the user's global Monitors
permission, so it passes. The action then answered 200 with a stub
built from the id alone: the hidden event's Next/Prev ids, its max
score and alarm frame ids, fileSize and a FileSystemPath.
The event is known to exist at that point, so an empty result means
the monitor condition removed it: answer 401 as edit, delete and
archive do.
Test: tests/php/test_api_event_view_acl.php (2 of 6 checks fail
without the fix).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit cf62c055763afa8fbaaa5925abe1659fda1544ec)
An edit replaces all of a snapshot's or tag's event links, but only the
new list was checked, so {"Event": []} from a user denied a monitor
unlinked that monitor's events, and any list without them dropped them.
The API's view of a snapshot or tag leaves those events out, so a client
that reads the list, changes it and saves it dropped them too.
The new list still needs view on every event it links. The links to
events the user may not view are now added back to the list before the
save (eventLinksToSave()), so the user can change the links they see and
the others stay. A link to an event that no longer exists is dropped.
Test: tests/php/test_api_associated_ids.php
(cherry picked from commit 6b1d0eaf8d)
Groups, Snapshots and Tags add/edit checked the ids from MonitorIds or
EventIds when the request had them, but Model::save() links the HABTM
entry (Monitor, Event) and ignores those fields on edit. A Groups=Edit
user could send {"Group":{"MonitorIds":"<current members>"},"Monitor":[]}:
the membership check saw no monitor moved, the save emptied the group and
lifted the Groups_Permissions deny on its monitors. On add, an empty
MonitorIds or EventIds next to a Monitor/Event list let monitors the user
may not edit into a new group, and events the user may not view into a
snapshot.
requestAssociatedIds() now reads the list the way save() does and
rewrites the request's entry to that plain list of ids, so a row given as
a join record links only its MonitorId/EventId. add() copies MonitorIds
or EventIds into the entry before the check, as it did before the save.
Test: tests/php/test_api_associated_ids.php
(cherry picked from commit 6bf015530e)
beforeFilter() authenticates user=/pass= (or username=/password=) and
then a token=, keeping the token's user. login() mints a refresh token
for that user whenever a password was sent, so a caller's own password
plus another user's refresh token gave a new 24 h refresh token for the
other user, renewing a token that is otherwise never renewed. A login now
takes one or the other. refs GHSA-m77q-66v7-j3fq
Test: tests/php/test_api_login_token.php
(cherry picked from commit 4223c12dad28b1ff1ff8738a6709bae6dc74f1bd)
The stateful branch regenerated the session id only on a password login.
A stateful=1 request with an access token (any action) or a refresh
token (host/login) wrote the user into the session id the client
brought. Any anonymous request stores a session id, so use_strict_mode
did not stop a planted one (CWE-384). Regenerate whenever the session
does not already hold this user, as the password login does.
(cherry picked from commit 657742ec5d)
An auth= login (ZM_AUTH_HASH_LOGINS) and the API's stateful login
(stateful=1 with user/pass, legacy API auth) wrote the user into the
session the client arrived with, keeping its id. With an id planted in
the client beforehand, the attacker's cookie became the victim's
session. The login form already issues a new id.
Both now call zm_session_regenerate_id_login() first: auth= when a
session the client carries a cookie for changes to another user, the
API whenever credentials log in. The API's cookie path no longer logs
the whole session.
The API's stateful login also drops $_SESSION['password'] =
$_REQUEST['password']. This is not a behaviour change: API logins send
pass=, not password=, so under ZM_AUTH_RELAY=plain the line stored
nothing for them. A stateful request that logs in with username= and
password= goes through zm_authenticate_request()'s form-login path,
which stores the password itself, as before. zmu gets its credentials
from zmuCredentials() (the zmu token change, which this one follows).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit bb3bd92b92)
When ZM_AUTH_HASH_SECRET is unusable zmu refuses tokens, so
zmuCredentials() falls back to what zmu got before. With
ZM_AUTH_RELAY=plain the API's alarm action used to pass the password
from the request (pass= or password=); the fallback read only the
session, which an API call without a stateful login doesn't have, so
zmu refused the call (rc 255). zmuCredentials() takes the caller's
password first again. With a usable secret it is never used.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 5ca19c2703ea9211fe040991305ce9b0a873faff)
getZmuCommand() and the API's alarm action put the user's credentials on
zmu's command line, where every local user can read them (ps,
/proc/<pid>/cmdline): an auth hash with the default hashed relay, the
plaintext password with ZM_AUTH_RELAY=plain. The API also wrote the
whole command to the debug log. With the default ZM_AUTH_HASH_IPS=0 that
hash is accepted as an auth= login from any address for ZM_AUTH_HASH_TTL
hours.
zmuCredentials() builds them in one place: with a usable
ZM_AUTH_HASH_SECRET, an access token for the user that expires after a
minute (zmu -T, accepted with every relay). Without the secret zmu
refuses tokens, so it gets what it did before. The API logs the command
without the credentials.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 32bc2b3fee87b752590ba6f210403afd9222d9ff)
Two ways a System=View account could read secrets through the Config API.
The oracle: ConfigsController::index() passed the caller's named filter to
buildFilter() and returned the matched rows. redactSecret() blanked a secret
Value in the response but not in the WHERE clause, so a filter such as
Name:ZM_AUTH_HASH_SECRET/Value LIKE:a% returned the row for a correct prefix
and nothing otherwise - the row count leaked the secret a character at a time,
and that secret signs auth tokens. index() now detects a filter on the Value
column and drops the secret rows from the result, so their presence reveals
nothing; a filter on any other column is unaffected.
The flags: ZM_JANUS_SECRET, ZM_UPLOAD_PASS and ZM_MQTT_PASSWORD held secrets
but were not marked Private, so the API returned them to System=View and
skin.js.php wrote them into every page as JavaScript constants. Mark them
private in ConfigData.pm.in (fresh installs) and add a Config UPDATE to the
untagged 1.39.41 migration (existing installs). No web page reads the three;
zmfilter, zmc and the Janus integration read them server side.
Tested: tests/php/test_config_value_filter.php (11 cases) covers the Value
detection, including Model.Value, case, and the DefaultValue non-match.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The monitors/alarm command:on cause was escaped with escapeshellarg(),
which drops bytes that are invalid in the server's locale and so mangles
a UTF-8 cause (for example an accented word) under a C/POSIX locale,
even though alarmCause() had kept those characters. Wrap the cause in
single quotes with POSIX quote escaping instead, which preserves every
byte and stays safe independently of what alarmCause() lets through.
Read the cause by comparing the query parameter to null before falling
back to POST data, so a cause of literally "0" is passed through rather
than discarded as a falsy value.
refs #5348
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
monitors/alarm/command:on now takes an optional cause (query or POST data)
and passes it to zmu --cause, so the event records it in place of
"Forced Web". Without a cause nothing changes.
The cause is stored as the event's Cause and in its Notes, so alarmCause()
keeps it to plain text: letters, digits, spaces and plain punctuation, cut
to the 31 bytes zmu stores on a character boundary, falling back to the
default when nothing is left. The value is quoted with escapeshellarg()
for the zmu command line.
Test: tests/php/test_monitor_alarm_cause.php
fixes#5348
packageControl() runs zmpkg.pl (which starts, stops and switches the run state
of ZoneMinder) with the state name from the request. It was public, so CakePHP
routed GET /api/states/packageControl/<command> straight to it, skipping the
System=Edit check in change() -- the method's only intended caller. With
beforeFilter() admitting any user whose System permission is not None, a
System=View user could stop, restart or change the run state.
Make it protected. change() still calls it after its System=Edit check; the
direct route now raises PrivateActionException (404).
Addresses GHSA-9gc6-9xg2-5636.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The events, frames, event_data and tags controllers compared the user's own
Events field, so a user granted Events=View or Edit only through their role
was refused every call, while the web UI's canView('Events') and
canEdit('Events') accept the role. Use ZM\User::canView() and canEdit().
Per-monitor limits are applied as before.
refs #5189
ControlsController only reads control definitions (index/view) — editing
them happens under Options, which requires System — but its beforeFilter
required System for the reads too. A client building a PTZ command fetches
the monitor's control definition from this API, and the web ui lets anyone
who can control a monitor (canView('Control')) do that, so a user with
Control permission but System=None could drive PTZ in the web ui yet got
401 from the API.
Allow the read for canView('System') or canView('Control'), matching
web/ajax/control.php. A user with neither is still refused.
Tested with the API on a scratch database: a Control=Edit, System=None user
got 401 on /api/controls and /api/controls/view/1 before and 200 after; a
Stream-only user with neither permission still gets 401.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
UsersController::view() and edit() assigned their permission check with
`$canX = A or B;`. `=` binds tighter than `or`, so only A was stored and the
self check was dropped: with ZM_USER_SELF_EDIT on, a user without System=Edit
got 401 on PUT users/<own id>.json. beforeFilter() also refused every user
with System=None before view() or edit() ran.
Let beforeFilter() through view and edit of the user's own record while
ZM_USER_SELF_EDIT is on, use || in both checks, and limit what such a user
saves to the fields the web UI allows for self edit
(includes/actions/user.php): Password, Language and HomeView. The request
data is rebuilt from those three fields, so Id, System, permissions and the
rest are never saved; a Language without a lang file is refused as in the UI.
Users with System=Edit save as before.
Self view follows the web UI too: a user without System permission sees their
own record only while ZM_USER_SELF_EDIT is on.
fixes#5242
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MonitorsController::edit() restarted zmc through the CakePHP model but
left the monitor's control server running. zmcontrol.pl reads
ControlAddress, ControlDevice and the Control when it starts, so after
an API edit PTZ kept using the old values: a camera password changed
through the API kept failing PTZ with 401 until zmcontrol.pl was
restarted by hand.
Restart the daemons the way the console's save does
(includes/actions/monitor.php), through ZM\Monitor: stop zmc and, for a
controllable monitor, zmcontrol.pl; start them again from the saved row
unless the monitor does not capture, is a WebSite or is deleted.
ZM\Monitor also sends the commands to the monitor's own server in a
multi-server setup, where the model's daemonControl() logged an error and
did nothing.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
requestField() read Model[field] whenever the request's Model entry was an
array, empty or not. CakePHP's Model::set() treats the bare top-level fields
as the model's data when that entry is empty (_setAliasData), so
{"Monitor":[],"EventEndCommand":"x"} passed the System-edit check on the
monitor command fields, which found no value, while save() stored x. A
Monitors=Edit user without System=Edit could set EventStartCommand or
EventEndCommand, which zmc runs through /bin/sh.
Every other check built on requestField() had the same gap: the target
MonitorId when adding or editing an event, the EventId when adding a frame,
and the ParentId when editing a group.
Read Model[field] only when the Model entry is non-empty, as set() does, and
otherwise the bare field; a non-empty entry that is not an array yields
nothing, since set() skips it.
tests/php/test_api_request_field.php runs the real method against each
request shape.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
DELETE /monitors/:id hard-deleted the Monitors row through CakePHP.
Zones went with it, but the monitor's Events, Monitor_Status and
Event_Summaries rows were left pointing at a monitor that no longer
exists. The console deletes with ZM\Monitor::delete(): it stops zmc
and zmcontrol and marks the monitor Deleted, keeping its events
("Events will age out") and letting the monitor be undeleted.
Use the same call in the API.
refs #5175
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The State model still declared primaryKey = 'Name' from before
zm_update-1.28.99 gave States an Id primary key. The REST routes only
match numeric ids, so states/<id>.json view/edit/delete looked up a
state *named* <id> and always failed, and names don't match the routes.
Through states/edit/<name>.json, edit never set the record id and
inserted a new nameless state instead of updating; view had no
_serialize and returned 500.
Use the default Id key. view serializes the state; edit sets the id,
accepts POST/PUT only and, like add, answers {message: Saved} or the
validation errors (as Monitors and Zones do) instead of an empty flash.
Names must be non-empty and unique, since zmpkg.pl and
states/change/<name> select states by name. index, change and delete
are unchanged.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* fix: give each migrated password its own random bcrypt salt
migratePasswords() stored the Bytes::Random::Secure object rather than
bytes from it, so en_base64() encoded the string
"Bytes::Random::Secure=HASH(0x...)". bcrypt only reads the first 22
characters of the salt, all from the constant class name, so every user
on every install got the same salt. With neither Bytes::Random::Secure
nor Data::Entropy installed the salt was empty and bcrypt() died with
"bad bcrypt settings", aborting zmupdate.pl. Even the Data::Entropy path
reused a single salt for every user in the run.
Read 16 bytes per user from /dev/urandom, as generateAuthHashSecret()
in ZoneMinder::Config already does. If it can't be read, leave that
user's legacy hash in place, which auth.php still verifies, instead of
writing a weak one. This drops the need for Bytes::Random::Secure and
the deprecated Data::Entropy. refs #4333
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* chore: drop the Data::Entropy dependency from packaging
zmupdate.pl no longer uses Data::Entropy, which upstream has deprecated
(CVE-2025-1860). refs #4333
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: rehash legacy passwords on every password login
Only the login form called migrateHash(), so accounts that sign in with
user=/pass= or through the API kept their mysql or zmupdate.pl
(mysql+bcrypt) hash, including those with the fixed salt. Call it from
those paths too, and have it check the password type itself.
migrateHash() also regenerated the auth hash from the in-memory user,
which still held the old password, so getAuthUser(), which checks
against the new one in the database, rejected it. Update the in-memory
password first, and update the row by Id rather than by the username
as typed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: note passwords still on a zmupdate.pl-migrated hash
Every salt migratePasswords() used before this branch was shared by all
users in a run: from Data::Entropy, which before 0.008 keys its generator
from rand() (CVE-2025-1860), or, since 38c0f743 with Bytes::Random::Secure
installed, a constant. The original hash input is gone, so zmupdate.pl
can't rehash these, and nothing distinguishes them from properly salted
ones. Log a warning listing every user still on a -ZM- hash, noting that
it is upgraded at their next login or password reset, and that unused
accounts can be disabled or deleted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Every query on Frames filters on EventId and orders or ranges by FrameId,
and nothing needs a frame by Id. The Id primary key was a clustered index
nothing read, and EventId_FrameId_idx was a secondary index every query
used. On a copy of a local table that secondary index was half the
table's size. With (EventId, FrameId) as the primary key the index is
gone, and an event's rows are stored together, so deleting an event is a
range delete.
zm_update-1.39.36.sql:
- converts AI_Detections.FrameId from Frames.Id to the per-event frame
number, drops its foreign key to Frames and indexes (EventId, FrameId).
A composite foreign key cannot replace it: ON DELETE SET NULL would
have to null the NOT NULL EventId, and Frames rows are written in
batches, so a detection can be recorded before its frame row.
- removes duplicate (EventId, FrameId) rows, keeping the earliest.
- rebuilds Frames with the new primary key.
- removes ON UPDATE CURRENT_TIMESTAMP from Frames.TimeStamp. Any UPDATE
of a frame row was overwriting its capture time.
Each step checks the current schema first, so the migration can be
re-run.
REST API: view, edit and delete take /frames/<action>/<EventId>/<FrameId>.json.
The old single-Id URLs return 404. CakePHP 2 has no composite keys, so
the model's primaryKey is EventId. That keeps Event's dependent cascade
delete limited to the event's own frames. The controller writes with
explicit (EventId, FrameId) conditions instead of save(), which would
match rows on EventId alone. Edit no longer changes EventId or FrameId.
view=image with fid but no eid used to look up Frames.Id. It now returns
404.
The Perl Frame class is identified by (EventId, FrameId).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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)
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.
