Skip to content

fix(mediaplayer): native C/C++ security hardening (parser/protocol memory safety + fuzzing)#968

Merged
dooly123 merged 17 commits into
BasisVR:developerfrom
towneh:fix/mediaplayer-native-security-hardening
Jul 17, 2026
Merged

fix(mediaplayer): native C/C++ security hardening (parser/protocol memory safety + fuzzing)#968
dooly123 merged 17 commits into
BasisVR:developerfrom
towneh:fix/mediaplayer-native-security-hardening

Conversation

@towneh

@towneh towneh commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Security hardening for the media player's native C/C++ (Native~/), from a memory-safety review of the parser/protocol layer. That code parses attacker-controlled bytes in-process — a peer-broadcast URL is parsed by every client in a room — so the findings here are remotely reachable. This closes five higher-severity and five medium issues, and extends the fuzz harness to two parsers that had no coverage. One commit per concern so each fix can be reviewed independently.

Out-of-bounds write / SSRF / DoS (higher severity)

  • RTSP Basic-auth base64 OOB stack write — a rtsp://user:pass@host URL could encode a 255-byte user:pass into a 256-byte field (340 bytes out), an 85-byte overwrite of adjacent stack fields. Size the field for the true maximum and bound the encode loop.
  • HLS sub-resource SSRF — the managed URL check only validates the top-level playlist; variant/segment/EXT-X-MAP URIs from the playlist body were fetched with no host check, so a hostile playlist could steer a fetch at 169.254.169.254/RFC1918/loopback. Every HLS fetch now goes through a resolved-address block (fail-closed, http(s)-only), with policy rejection distinguished from transient failure so a blocked reload/segment terminates deterministically instead of skipping or busy-looping. The non-global-unicast IPv4 classifier is also aligned to the full IANA special-use set in both the native and managed guards.
  • MP4 box-tree recursionparse_box_tree recursed with no depth limit; a small crafted moov overflowed the demux-thread stack. Capped at 32.
  • MPEG-TS PES accumulation — a stream that sets PUSI once and streams continuation packets forever grew the PES buffer unbounded, with a signed-overflow in the growth math. 8 MiB cap, 64-bit math.
  • Android Vulkan output-texture UAF — the framebuffer/image-view were destroyed on the main thread while the render thread could be mid-present, on a mid-stream resolution change. The destroy is deferred to the render thread under the lock, with one consistent target extent for the framebuffer and render area.

Medium

  • Render-event liveness registry so a late OnRenderEvent can't use-after-free an engine freed by basis_media_close.
  • RTMP _result AMF parsing bounds every read (was reading up to 8 bytes past the buffer).
  • RTSP RTP reassembly buffers capped (16 MiB) with the overflow fixed; a cap hit drops the whole AU/frame rather than a truncated one.
  • Windows Media Foundation allocations checked (a large declared sample turned an alloc failure into a null-deref); COM buffer unlock/HRESULT hygiene; software-fallback size bounded against overflow.
  • Android AMediaCodec_getInputBuffer null-checked, sizes compared as size_t, and the codec queue status propagated so a dropped frame isn't reported as accepted.

Fuzzing — new fuzz_url, fuzz_hls, fuzz_rtsp and fuzz_rtmp targets under ASan/UBSan, wired into build.sh; the URL/HLS/RTSP/RTMP parsers were all previously unfuzzed. HLS uses an in-memory HTTP provider; RTSP/RTMP #include the real .c and provide a basis_io stub (byte-serving the socket-read paths), driving parse_sdp/depkt_video/depkt_audio/amf_find_stream_id/FLV tag parsers directly. The RTSP target earned its keep — it found a signed left-shift UB in the RTP timestamp read (rtp[4] << 24 on a byte ≥ 128) on its first run, fixed here and pinned as a replay repro. Deeper full-session RTSP/RTMP coverage (a scripted handshake, or an injected transport vtable that would also enable RTSP/RTMP unit tests) is tracked as a backlog follow-up.

Docs / tooling — also retires the self-hosted tools/media-test-streams Docker stack and its TESTING.md guidance. The public endpoints already listed cover the common lanes, the CI conformance gate (tools/media-conformance) already proves the demuxers on every supported container/codec, and the remaining lanes (live RTSP/RTMP/RIST, split-stream, captions, LPCM 7.1, localhost) are cheaper to serve from your own files/server than to maintain a bundled stack. TESTING.md now lists the supported container/codec/transport inputs to bring your own of, framed around what CI can't touch (real decode + present, A/V sync, live transports).

Required checks

All boxes below must be ticked before this PR can merge. If a check is genuinely N/A, tick it anyway and explain under Notes.

  • Tested — I built and ran this locally. The change works in the editor and (where relevant) in a built player.
  • Transform access is combined and limited — In hot paths, transform reads/writes go through TransformAccessArray or are otherwise batched. I have not added per-frame transform.position / transform.rotation / transform.localPosition calls inside loops. Whenever I need both position and rotation, I use the combined APIs — SetPositionAndRotation / SetLocalPositionAndRotation for writes, GetPositionAndRotation / GetLocalPositionAndRotation for reads — instead of two separate property accesses; the combined call does one local-to-world matrix traversal instead of two.
  • Addressables used for asset/memory loading — Any new asset loads go through Addressables. No new Resources.Load, no direct asset references that pull large content into memory on scene load.
  • No new GetComponent / AddComponent where avoidable — Where unavoidable, the result is cached on a field, and any GetComponent<T> is replaced with TryGetComponent<T>(out var x) — bare GetComponent will be denied. TryGetComponent is the modern API (Unity 2019.2+) and skips the Editor-only GC allocation GetComponent causes when a component is missing: Unity wraps the null return in a managed "fake null" object so its overloaded == operator can still detect destroyed C++ objects, and constructing that wrapper allocates; TryGetComponent returns a bool plus out parameter and never builds the wrapper. None of these calls run inside Update, LateUpdate, FixedUpdate, jobs, or other per-frame code paths.
  • Per-frame work is scheduled through BasisEventDriver — Any new per-frame work hooks into BasisEventDriver rather than adding standalone Update / LateUpdate / FixedUpdate callbacks on a MonoBehaviour.
  • Anything added to BasisEventDriver is bulletproof, or guarded by try/catchBasisEventDriver runs the single per-frame tick that drives the whole framework (network apply, local player sim, blendshapes, JigglePhysics, nameplates, and more) as one sequential chain. An unhandled exception anywhere in that chain aborts the rest of the tick, so every step after the throwing one is silently skipped for that frame. New work added to the driver must either be guaranteed not to throw, or be wrapped in a try/catch that contains the failure and surfaces it through BasisDebug — logged once / rate-limited, never every frame (see the existing HVRBasisBuiltInAddresses.Simulate() guard for the pattern). Expect this to be scrutinized closely in review.
  • Considered jobification — I asked whether this work can be moved to a Unity Job (Burst-compiled where possible). If it can, it is. If it cannot, the reason is in Notes.
  • No needless { get; set; } properties or access lockdowns — Public fields are fine; Basis is meant to be read and modified freely, so don't wall things off private/internal without a real reason. Don't wrap a field in { get; set; } when the accessors do nothing — property accessors have a real performance cost vs direct field access, and the lead maintainer prefers plain fields (or a method / setter-only property when only the setter needs logic) over a noop-getter pair. For .Instance singletons, callers reassigning Type.Instance is allowed; if that would break your code, log a warning or throw — don't block the assignment. Locking down access is not your call.
  • Camera access goes through BasisLocalCameraDriver — Code that needs the local camera (transform, projection, rig data, etc.) pulls it from BasisLocalCameraDriver rather than looking one up itself. Don't roll a separate camera discovery path.
  • Logging uses BasisDebug — All new logging calls go through BasisDebug.Log / BasisDebug.LogWarning / BasisDebug.LogError (with an appropriate LogTag) instead of UnityEngine.Debug.Log / Debug.LogWarning / Debug.LogError. BasisDebug routes through Basis's tagged, color-coded logger and respects the project-wide LoggingDisabled toggle so logging can be killed at runtime; bare Debug.Log calls bypass that and will be denied.
  • No scene-wide discovery for dependencies — New code is architected so it does not need FindObjectOfType / FindObjectsOfType / GameObject.Find / FindGameObjectsWithTag to locate what it depends on. References are wired in — registered through an existing manager/driver, injected at init, or passed in by the caller — rather than discovered by scanning the scene at runtime. If a scene scan is genuinely unavoidable, justify it under Notes.
  • No allocations in hot paths — Per-frame code (Update / LateUpdate / FixedUpdate, simulation loops, jobs, anything called once per frame or more) does not allocate. No new on reference types, no LINQ, no string concatenation/interpolation, no boxing, no foreach over interface-typed collections. Allocate once at init and reuse the buffer.
  • No debugging in hot paths — No log calls of any kind on per-frame paths, including BasisDebug. Hot-path logging floods the console and incurs cost on every frame regardless of whether the message is filtered out downstream. If a hot-path log is needed while iterating, gate it behind #if UNITY_EDITOR and remove (or leave gated) before merge.
  • Hot-path collection access is optimized — Cache .Count (lists) / .Length (arrays) into a local int before the loop instead of re-reading the property each iteration. Prefer T[] (with a separate length int when the array is over-sized) over List<T> where the data is hot — Unity's mono BCL doesn't expose CollectionsMarshal.AsSpan(List<T>), so a list can't be fed into Span<T> / unsafe paths cleanly. Where the perf justifies it, drop into Span<T> / ref locals / Unsafe.As / unsafe pointer code to skip bounds checks and copies, and call out the invariants you're relying on under Notes so reviewers can sanity-check them.

Testing details

Tick the platforms you actually tested on. Leave the rest unticked — these are informational and do not block merge.

  • Windows
  • Linux
  • Android
  • iOS
  • macOS

Input / control mode coverage:

  • Tested in VR (note headset under Notes)
  • Tested in desktop / non-VR mode
  • Tested with phone controls (mobile touch input)
  • N/A — change does not touch player/XR/input code

Where applicable, confirm these flows still work after your changes:

  • Hot-switching (desktop ↔ VR mode swap at runtime)
  • Avatar swapping
  • Server swapping (joining / leaving / changing servers)
  • N/A — change does not touch any of the above

Notes

This is almost entirely a native-plugin change under Native~/. The one C# file it touches is BasisMediaPlayerSecurity.cs — the SSRF address classifier, extended (in lockstep with the native basis_io.c guard) to the full IANA "not globally reachable" special-use set. It has no transform/component/hot-path/per-frame code, so the Unity-specific required checks (transform access, Addressables, GetComponent, BasisEventDriver, per-frame allocation/logging, camera driver, scene discovery, jobification, collection access) still don't apply and are ticked as N/A per the template.

Verification is via the repo's own native gates, run locally against the real shipping source rather than a mock: tools/media-fuzz (all 10 targets build + run clean under AddressSanitizer + UndefinedBehaviorSanitizer, and the pinned crash-repro replay gate passes — including the two shift-UB repros the new RTSP/RTMP fuzzers surfaced), and tools/media-conformance (demux output still matches ffprobe on all synthetic fixtures, so the caps don't change valid-input playback). Portable C and basis_win_decode.cpp compile clean, and both native binaries are rebuilt from this source.

One behavioural change for testers: the native transports (HLS/RTSP/RTMP) now enforce the non-global-unicast address block themselves, so a localhost URL on those transports is refused even in the Editor unless BASIS_MEDIA_ALLOW_LOCAL relaxes the native re-check (it does not relax the top-level C# gate — a top-level RFC1918 URL stays refused regardless). Documented in TESTING.md.

Known residuals (deliberate, tracked as follow-ups — not oversights):

  • SSRF pre-check depth. The host check closes literal-internal and statically-private URLs (the common vector). It does not close active DNS rebinding or an allowed URL that redirects to an internal host, because the platform HTTP stacks (WinHTTP/JNI) re-resolve and follow redirects. Fully closing those is a connect-by-pinned-IP change with per-redirect re-validation and connected-peer verification at the provider boundary — it needs on-device testing (a naive "reject all 3xx" breaks legitimate CDN redirects), so it's scoped as its own follow-up.
  • Render-event ABA. The liveness registry closes the use-after-free of a closed engine. The narrow pointer-reuse edge (a delayed event matching a new engine at the same address) is benign for the C# binding's RENDER_UPDATE-only, idempotent usage; fully closing it needs a generation-stamped handle in the event payload (a C# ABI change) — follow-up.
  • RTSP/RTMP fuzz depth. The new targets drive the buffer-taking parsers directly (where the found bugs lived); full end-to-end session fuzzing (scripted handshake) is a follow-up.

@towneh
towneh force-pushed the fix/mediaplayer-native-security-hardening branch 2 times, most recently from 0d4fcfe to 946b6d3 Compare July 17, 2026 12:41
@towneh towneh added the bug Something isn't working label Jul 17, 2026
@towneh
towneh force-pushed the fix/mediaplayer-native-security-hardening branch from 946b6d3 to 7755304 Compare July 17, 2026 14:03
towneh added 12 commits July 17, 2026 15:18
A rtsp://user:pass@host URL could drive the Basic-auth base64 encoder past authb64[256]: a maximal 255-byte user:pass encodes to 340 bytes, an 85-byte out-of-bounds write into adjacent stack fields. Size authb64 for the true maximum and bound the encode loop.

The RTP reassembly buffers (access unit, FU, afrag) also grew without limit — a server that never sets the marker bit could exhaust memory — and grow()'s doubling overflowed to a negative int near INT_MAX. Add a 16 MiB ceiling with 64-bit math, and drop the whole access unit/frame when the cap is hit (latching the audio discard across the frame's packets) rather than flushing a truncated one to the decoder.

Read the RTP timestamp through uint32_t so rtp[4] << 24 can't shift a byte >= 128 into the int sign bit (signed-overflow UB in depkt_video/depkt_audio).
The managed URL check validates only the top-level playlist. Variant, segment and EXT-X-MAP URIs come from the attacker-controlled playlist body and were followed straight to the platform HTTP stack with no host check, so a hostile playlist could steer a fetch at 169.254.169.254, an RFC1918 host or loopback.

Add basis_io_host_is_blocked() (resolves the name, blocks any non-global-unicast result, fail-closed, honouring BASIS_MEDIA_ALLOW_LOCAL) and route every HLS fetch through it, restricted to http(s). Policy rejection (bad scheme/host) is distinguished from a transient open failure so a blocked playlist reload, init segment OR media segment terminates the producer deterministically while a transient failure backs off — neither skips init/segment data nor busy-loops. This pre-check does not close active DNS rebinding or a redirect to an internal host (the platform stack re-resolves and follows redirects at connect) — recorded in the code as a follow-up.

Also align the non-global-unicast IPv4 classifier in both the native (basis_io.c) and managed (BasisMediaPlayerSecurity.cs) guards, kept in lockstep, to the IANA not-globally-reachable special-use set: 192.0.0/24, 192.0.2/24, 192.88.99/24 (6to4 relay), 198.18/15, 198.51.100/24, 203.0.113/24.
parse_box_tree recursed on mdia/minf/edts/stbl and trak with no depth limit, so a small crafted moov nesting boxes thousands deep overflowed the demux-thread stack. Thread a depth counter through and bail past 32 levels.
A PES buffer only flushes on the next payload-unit-start, so a stream that sets PUSI once and streams continuation packets forever grew it without bound, and accum_reserve's doubling overflowed to a negative int near INT_MAX. Add an 8 MiB cap with 64-bit math; reset and resync on overflow.
basis_vk_set_output_texture ran on the main thread and destroyed the framebuffer and image-view while the render thread could be mid-present using them — a cross-thread use-after-free on a mid-stream resolution change. Record the handle change under the lock and let render_update drop and rebuild the framebuffer on its own thread.

Read all Unity-registration state as one locked snapshot (no unsynchronised peek at unityNativeTex); give the framebuffer cache its own render-thread-owned dimensions (fboW/fboH) instead of overloading unityW/unityH (written by the setter under the lock); derive one target extent for both framebuffer creation and the render area; and only detach the pending frame when a render target is registered, so its AHardwareBuffer reference isn't leaked when none is.
OnRenderEvent dereferences the engine pointer on Unity's render thread; basis_media_close frees it on the main thread, so a render event in flight during close is a use-after-free. Add a liveness registry: the render dispatch runs under a lock only while the engine is registered. Deregister at the very top of basis_media_close — before stopping and joining the demux threads — so no render callback can touch the decoder during any part of teardown; it blocks until any in-flight event returns. Open fails cleanly if the registry is full rather than returning an engine whose events would be silently dropped.
amf_find_stream_id walked a server-supplied AMF buffer checking only i < len before multi-byte reads, so a truncated _result read up to 8 bytes past the buffer. Bound every field read against len, and require the full name length before the _result strncmp.

Also cast the chunk stream-id bytes to uint32_t before shifting, so sid[3] << 24 can't shift a byte >= 128 into the int sign bit (signed-overflow UB).
…ecoder

make_input_sample fed an attacker-controlled sample length to MFCreateMemoryBuffer and used the result without checking it, so a stream declaring an oversized sample turned an allocation failure into a null-deref. Reject a wild sample length (> 64 MiB, far above any real compressed AU) up front — including before the AV1 configOBU concatenation, with an overflow guard — check every MFCreate*/Lock result, unlock a successfully-locked buffer before releasing it, and propagate the Unlock/SetCurrentLength/AddBuffer HRESULTs.

Bound the decoded-output buffer too: cap the MFT-declared cbSize and the software-fallback estimate (dimensions bounded before the multiply) so a malformed output size can't exhaust memory.
AMediaCodec_getInputBuffer can return NULL for a valid index under buffer pressure; submit_video/submit_audio/feed_extractor_sample used it without a check. Guard the pointer and compare sizes as size_t so a large cap can't sign-cast negative into memcpy. Queue only a complete AU that fits the buffer, releasing the slot empty otherwise, and propagate the codec queue status so a rejected or dropped frame isn't reported as accepted.
Add fuzz_url, fuzz_hls, fuzz_rtsp and fuzz_rtmp to the media-fuzz harness under ASan/UBSan, wired into build.sh. URL/HLS were previously unfuzzed; HLS uses an in-memory provider (SSRF resolver stubbed). RTSP/RTMP own their sockets, so their harness #includes the real .c and provides a basis_io stub (byte-serving for rtsp_recv / rtmp_read_message) and drives the buffer-taking parsers (parse_sdp, depkt_video/audio, amf_find_stream_id, handle_video/audio) directly.

The RTSP target found a signed-shift UB in the RTP timestamp read on its first run (fixed in the RTSP/RTMP commits); pinned as testcases/rtsp/rtp_ts_shift_ub.bin and testcases/rtmp/chunk_streamid_shift_ub.bin (the RTMP chunk stream-id path the RTP repro doesn't reach) for the replay gate.
Note that RTSP/RTMP/HLS enforce the non-global-unicast block natively — BASIS_MEDIA_ALLOW_LOCAL relaxes only that native re-check, not the top-level C# gate (a top-level RFC1918 URL stays refused regardless). Add an HLS sub-resource SSRF negative test, and record the URL/HLS/RTSP/RTMP fuzz targets (deeper full-session RTSP/RTMP coverage tracked as backlog B76).
Rebuild the Windows x64 DLL and Android arm64 .so (both RIST-enabled) from the hardened Native~ source so the security fixes ship in the plugins.
@towneh
towneh force-pushed the fix/mediaplayer-native-security-hardening branch from 7755304 to 49d408d Compare July 17, 2026 14:18
@towneh
towneh requested a review from dooly123 July 17, 2026 14:30
towneh added 5 commits July 17, 2026 15:46
Remove tools/media-test-streams and its TESTING.md guidance. The public endpoints cover the common lanes, the CI conformance gate already proves the demuxers on every supported container/codec, and the remaining lanes (live RTSP/RTMP/RIST, split-stream, captions, LPCM 7.1, localhost) are cheaper to serve from your own files/server than to maintain a bundled Docker stack. TESTING.md now lists the supported container/codec/transport inputs to bring your own of.
Drop a stray backlog-ID reference (a private planning ID that shouldn't be in a committed doc); note that rtmps is allowlisted but the player rejects it (RTMP-over-TLS unsupported) and rist needs the -DBASIS_WITH_RIST build — passing the URL gate isn't the same as playable; and widen the always-blocked address row to the full IANA non-global-unicast set the guards now enforce (TEST-NET / benchmarking / 6to4 relay), matching basis_io.c and BasisMediaPlayerSecurity.cs.
… fixtures

Drop the non-public fixture URLs from the codec matrix and the fMP4/WebM seek rows. The common lanes keep their license-clean public endpoints (VRCDN, Blender, Mux, Fraunhofer); the codec-specific fixtures now come with an ffmpeg recipe to generate them from a CC clip (Big Buck Bunny / any Blender open movie), which is more durable than a hosted file and matches the bring-your-own approach the rest of the guide takes. Demux is already covered by the CI conformance gate, so these rows are the real decode + present pass.
The A/V-sync row asks for footage with visible speech, but the baseline endpoint (Big Buck Bunny) has none. Point at the Blender Studio films page and name Sintel/Spring (both CC-BY, clear lip-sync) as good sources for that row.
media.xiph.org mirrors the Blender films as lossless/4K masters on a stable host. Name Sintel 4K (media.xiph.org/sintel/sintel-4k.y4m.xz + 4K frame sets) as the 4K source and note cutting a short segment, so the AV1-4K row has a concrete, license-clean 4K clip to encode from.
@dooly123
dooly123 merged commit bc8ba23 into BasisVR:developer Jul 17, 2026
14 checks passed
@towneh
towneh deleted the fix/mediaplayer-native-security-hardening branch July 17, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants