ZM_Object's constructor treats an array as the object's row, and validInt()
and validCardinal() returned an array as they got it (preg_replace() does).
So wherever a request id reached `new ZM\X(...)`, raw or through those
validators, a crafted array built an object whose Id, UserId or MonitorId
the sender chose. The permission check then looked at those, and the delete
or update that followed used the Id. A user with Events=View deleted another
user's filter by posting Id[Id]=<filter>&Id[UserId]=<self> to the filter
delete action; the same shape reached events (MonitorId), users, reports,
snapshots and storage.
validInt() and validCardinal() now give '' for anything that isn't a single
value. The places that built an object from a raw $_REQUEST value (the
control and event ajax handlers, the event, filter, report, snapshot and
storage actions, view_hls, and the download and event detail modals) take
the id through validCardinal().
tests/php/test_request_ids_scalar.php covers the validators with arrays and
fails if web code builds an object straight from $_REQUEST, $_GET or $_POST.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Fix: JPEG snapshot generation when requested frame is unavailable in event video
* Improve frame extraction fallback for unavailable timestamps
- Limit ffprobe search to a small time range around the requested position
- Validate the generated JPEG before considering extraction successful
- Fix timestamp formatting to be locale-independent
- Remove obsolete command/output handling from callers
-Handle non-monotonic packet PTS order
-- Do not assume ffprobe returns packets sorted by PTS
-- Scan all packets in the selected time range
-- Select the closest packet before and the closest packet after the requested timestamp
-- Avoid selecting an incorrect fallback frame when PTS values are reordered
* Use temporary files for frame extraction
- Write FFmpeg output to a unique temporary file for each extraction attempt
- Atomically rename the validated frame to the target path on success
- Prevent failed concurrent requests from deleting a frame created by another request
* Fix video frame extraction near event end
* Remove frame_time condition from lookback loop
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Isaac Connor <isaac@zoneminder.com>
Co-authored-by: Isaac Connor <iconnor@connortechnology.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
view_video.php took the end of the range straight from the request and never
checked it against the file:
if (!empty($matches[2])) $end = intval($matches[2]);
$length = $end - $begin + 1;
so on a 1000 byte file:
bytes=0-99999 206, Content-Length 100000, body 1000 bytes
bytes=5000-6000 206, Content-Range naming bytes that do not exist
bytes=500-100 206, Content-Length -399
bytes=1000- 206, Content-Length 0
bytes=-100 200 with the whole file, not the last 100 bytes
A client that is told to expect 100000 bytes and gets 1000 does not see a bad
request, it sees a truncated file, and reports the video as broken. The suffix
form was not recognised at all because the pattern required a digit before the
dash.
This is reached once per fragment by the byte-range HLS manifest VideoStore
writes, every fragment being a Range against the one mp4, so a player that
asks for anything the file cannot supply gets a body that does not match its
own Content-Length rather than an answer it can act on.
Parse the header properly: clamp a range that runs past the end, because a
client may ask for more than is there and is entitled to what is there;
answer 416 with "Content-Range: bytes */size" when the range cannot be
satisfied at all, so the client learns the real length; and read "-N" as the
last N bytes. Length is now derived from the range being served rather than
the one requested, and the send loop counts down by the bytes it actually
read, so Content-Length and the body cannot disagree.
Only the first range of a multi-range request is served, as before. A
multipart/byteranges body is not worth building for this, and falling back to
sending the whole representation is not an option when these are event videos
of hundreds of megabytes; Content-Range names exactly what was sent.
The parsing is its own dependency-free include so it can be tested without a
database, and tests/php/test_http_range.php covers each case above plus a
sweep asserting that every range it ever returns lies inside the file.
Co-Authored-By: Claude Opus 5 (1M context) <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>
The CSRF token check on the image proxy only runs when ZM_ENABLE_CSRF_MAGIC
is on. Browsers that send Sec-Fetch-Site report when another site started a
request, so refuse proxy requests whose value is anything but same-origin or
none, whatever the CSRF setting. Browsers that do not send the header are
unaffected.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
index.php skips csrf_check() for view=image because images are loaded
through <img src>, and csrf_check() only validates POSTs in any case. The
proxy= handler makes the server fetch a caller-supplied URL, so a page
on another site could embed an <img> pointing at it and have a logged-in
monitor editor's browser drive server-side requests to the LAN.
When ZM_ENABLE_CSRF_MAGIC is on, the proxy branch now checks
__csrf_magic from the request with csrf_check_tokens() and answers 403
without it. Plain image views are unaffected. The only caller, the
camera discovery thumbnail on add_monitors, appends csrfMagicName and
csrfMagicToken to its <img> URL, and now URL-encodes the camera stream
URL so a stream URL containing & or " no longer truncates the proxy
parameter or breaks out of the src attribute.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 2958f89c5af63040483ac8a389d1fd7080b777a7)
The proxy resolved the camera host, refused reserved addresses, then
handed the original URL to fopen(), which resolved the name a second
time. A hostname whose DNS answer changes between the two lookups (DNS
rebinding) passed the guard with a LAN address and was then fetched from
127.0.0.1 or another reserved address.
Build the fetch URL with the first checked address in place of the host
and keep the original name in the Host header (including a non-default
port) and as the TLS peer_name so SNI is unchanged. Credentials, port,
path and query are carried over from the parsed URL, and the digest
retry sends the same Host header alongside Authorization. Also strip the
brackets parse_url leaves on IPv6 literal hosts so they validate as
addresses instead of failing the lookup.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit e3b7e35d275a0e92001a30df446f06c9237cef87)
The proxy= handler in web/views/image.php validates the resolved address
of the URL host against FILTER_FLAG_NO_RES_RANGE, then fetches with
fopen() using PHP's default follow_location=1. A host that passes the
check could answer 302 to 127.0.0.1 (or ::1, 169.254.169.254) and the
stream wrapper would request it, returning the loopback content.
Set follow_location to 0 in the http stream context so a 3xx is not
followed. The same $opts is reused for the digest auth retry, so both
fetches are covered. Camera discovery has no need for redirects.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit ec71edad630ff614eaae499146d33fd780449715)
Without imagecreatefromjpeg the scaling path fatals, so fall through to
passing the file through as is.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merged mp4 export is named '<Monitor> <start> to <end>.mp4', so it contains
spaces and colons, and download.php emitted it as a bare unquoted filename=
parameter. That is not a valid RFC 6266 token, so browsers that parse
Content-Disposition strictly find no filename and fall back to naming the
download after the last path segment of the URL - index.php. Firefox is lenient
and accepted it, which is why the report was Chrome-on-Windows only.
Add contentDispositionAttachment(), which emits a quoted ASCII filename with the
Windows-illegal characters folded to '_', plus the untouched name as RFC 5987
filename* whenever that folding changed anything, so unicode monitor names still
arrive intact.
Also in that path:
- urlencode the file and export_root query parameters; a monitor name containing
'&' or '+' would otherwise split or mis-decode the download URL. Read them back
in export.js with URLSearchParams so the link text shows the decoded name.
- drop the stray ';' from Content-Length, which made the value unparseable.
- silence the shutdown unlink()s, whose warnings would be appended to the body
of a download that had already started.
- log $this->filenamePath, not an undefined local, on the unreadable-file path.
Tests: tests/php/test_download_content_disposition.php, 12 assertions, all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nr76CednxtDt2nPuq6WrbL
web/views/image.php exposes an outbound fetch via ?view=image&proxy=.
It validated only the URL scheme, so any user with canView('Events')
could make the server issue HTTP requests to arbitrary hosts and, on a
401 Digest challenge, replay credentials taken from the URL's user-info.
view=image is also exempt from CSRF handling in index.php, so the fetch
could be triggered from an attacker page via a plain <img> tag.
Camera discovery legitimately proxies cameras on the local LAN, so
private ranges stay reachable. Reject only loopback, link-local and
other reserved addresses via FILTER_FLAG_NO_RES_RANGE, which covers
127.0.0.0/8, ::1, fe80::/10 and 169.254.0.0/16 (cloud metadata) without
excluding 10/8, 172.16/12, 192.168/16 or fc00::/7. Resolve the host
first so a hostname cannot point at those ranges.
Also require canEdit('Monitors') — the only consumer is the discovery
thumbnail in add_monitors, which already demands that right — and stop
reading $url_parts['user'] without an isset() guard.
Refs GHSA-g28p-q36w-h3c5, GHSA-rq9f-p634-rrpg.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
image.php, view_video.php and view_hls.php previously checked only the
coarse canView('Events') / canView('Snapshots') role before streaming
media for a user-supplied event id. An authenticated user denied access
to a monitor could still fetch event snapshots, captured frames,
recorded MP4s and HLS manifests for events belonging to that monitor by
calling the direct endpoints with the event id.
Call Event->canView() after loading the event and return 404 on denial
so the event id cannot be enumerated. view_video also validates
Event->Id() so unknown ids return 404 instead of an empty 200 body.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- When executing Length() and Duration() , do not write the result to the database.
- When assigning a value to the "data-video-duration-secs" attribute, first check the Length in $row['Length'] and, if it is 0, only then check the file's Duration.
- If the M3u8 file is missing or invalid, do not cache the server response.
When DefaultVideo is 'index.m3u8' but the view_video.php fallback
resolves $path to the actual mp4 (so byte-range playback works), the
Content-Disposition header still advertised filename='index.m3u8'. The
download button then saved a playlist instead of video.
Derive $filename from $path unconditionally — after the fallback runs,
$path is always the file we're streaming, regardless of mode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The segment-URL rewrite captured "index.php?…" in $1 and then prepended
$base_url . '?', producing a double-prefixed URL that drops the view=
routing parameter. Players fetched the manifest itself instead of the
mp4 byte range, so Firefox showed a spinner and Chrome silently failed.
Capture only the query string (everything after "index.php?") for both
the bare segment URLs and the EXT-X-MAP URI rewrite.
Same fix that landed (unmerged) in PR #4803 and PR #4806; pulled in here
so the full HLS path can be reviewed and merged together.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OK, let's simplify this.
Let's hope that either there won't be any national characters in the file name, or that basename() will handle them correctly.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Fragment tracking:
- Rewrite detection to use avio_flush()+avio_tell() before each video
keyframe write, giving exact fragment boundaries. Fixes duplicate
entries and missing first fragment in the m3u8 manifest.
- Remove unused pending_fragment_dts_ and last_flush_pos_ members.
Live event playback:
- Set DefaultVideo to index.m3u8 at event INSERT time (no DB update
needed after videoStore opens)
- Rename incomplete file to include codec (incomplete.h264.mp4) so
canPlayCodec() works during recording
- Rewrite final m3u8 after video file rename to reference final name
- Add live retry handler in event.php: retries all error codes with
backoff (3s then 5s), max 30 retries, resets on successful playback
- Update progress bar duration from video element for live events
Fallback handling:
- view_video.php: when DefaultVideo is m3u8 and no file param given,
search event dir for actual mp4 (final name then incomplete)
- view_hls.php: reject m3u8 with no EXTINF entries (event just started)
- event.php: require m3u8 file to exist on disk before offering HLS,
fall back to direct MP4 otherwise
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Write a single continuous fragmented MP4 per event and generate an HLS
m3u8 manifest with byte-range references into that file. This enables
seamless browser playback via video.js's built-in http-streaming (VHS)
without needing separate segment files.
C++ changes:
- VideoStore tracks fragment boundaries (moof+mdat byte offsets and
durations) as packets are written, by monitoring avio_tell() around
keyframe writes in write_packet()
- Add writeM3U8() method that generates EXT-X-VERSION:7 byte-range
manifests with EXT-X-MAP for the init segment
- Event writes a live m3u8 (no EXT-X-ENDLIST) on each new fragment
for in-progress viewing, and a final VOD manifest at event close
- Change movflags to frag_keyframe+empty_moov+default_base_moof
(default_base_moof required by HLS fMP4 spec, faststart removed
as it's meaningless with empty_moov)
PHP/web changes:
- New view_hls.php endpoint serves pre-built m3u8 with auth tokens
- event.php detects index.m3u8 and uses HLS as primary source with
direct MP4 as fallback
- CSRF exemption for view_hls in index.php
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>
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>
Add input validation and shell argument escaping to prevent OS command
injection via the 'show' parameter in web/views/image.php. The parameter
is now validated against an allowlist and all values passed to exec()
are wrapped with escapeshellarg().
Also fix PHP operator precedence bug in shutdown.php where 'and' was
used instead of '&&', causing the 'when' parameter validation to not
work as intended.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
