fix(mediaplayer): native C/C++ security hardening (parser/protocol memory safety + fuzzing)#968
Merged
dooly123 merged 17 commits intoJul 17, 2026
Conversation
towneh
force-pushed
the
fix/mediaplayer-native-security-hardening
branch
2 times, most recently
from
July 17, 2026 12:41
0d4fcfe to
946b6d3
Compare
towneh
force-pushed
the
fix/mediaplayer-native-security-hardening
branch
from
July 17, 2026 14:03
946b6d3 to
7755304
Compare
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
force-pushed
the
fix/mediaplayer-native-security-hardening
branch
from
July 17, 2026 14:18
7755304 to
49d408d
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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://user:pass@hostURL could encode a 255-byteuser:passinto 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.EXT-X-MAPURIs from the playlist body were fetched with no host check, so a hostile playlist could steer a fetch at169.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.parse_box_treerecursed with no depth limit; a small craftedmoovoverflowed the demux-thread stack. Capped at 32.Medium
OnRenderEventcan't use-after-free an engine freed bybasis_media_close._resultAMF parsing bounds every read (was reading up to 8 bytes past the buffer).AMediaCodec_getInputBuffernull-checked, sizes compared assize_t, and the codec queue status propagated so a dropped frame isn't reported as accepted.Fuzzing — new
fuzz_url,fuzz_hls,fuzz_rtspandfuzz_rtmptargets under ASan/UBSan, wired intobuild.sh; the URL/HLS/RTSP/RTMP parsers were all previously unfuzzed. HLS uses an in-memory HTTP provider; RTSP/RTMP#includethe real.cand provide abasis_iostub (byte-serving the socket-read paths), drivingparse_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] << 24on 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-streamsDocker 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.
TransformAccessArrayor are otherwise batched. I have not added per-frametransform.position/transform.rotation/transform.localPositioncalls inside loops. Whenever I need both position and rotation, I use the combined APIs —SetPositionAndRotation/SetLocalPositionAndRotationfor writes,GetPositionAndRotation/GetLocalPositionAndRotationfor reads — instead of two separate property accesses; the combined call does one local-to-world matrix traversal instead of two.Resources.Load, no direct asset references that pull large content into memory on scene load.GetComponent/AddComponentwhere avoidable — Where unavoidable, the result is cached on a field, and anyGetComponent<T>is replaced withTryGetComponent<T>(out var x)— bareGetComponentwill be denied.TryGetComponentis the modern API (Unity 2019.2+) and skips the Editor-only GC allocationGetComponentcauses when a component is missing: Unity wraps thenullreturn in a managed "fake null" object so its overloaded==operator can still detect destroyed C++ objects, and constructing that wrapper allocates;TryGetComponentreturns aboolplusoutparameter and never builds the wrapper. None of these calls run insideUpdate,LateUpdate,FixedUpdate, jobs, or other per-frame code paths.BasisEventDriver— Any new per-frame work hooks intoBasisEventDriverrather than adding standaloneUpdate/LateUpdate/FixedUpdatecallbacks on a MonoBehaviour.BasisEventDriveris bulletproof, or guarded bytry/catch—BasisEventDriverruns 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 atry/catchthat contains the failure and surfaces it throughBasisDebug— logged once / rate-limited, never every frame (see the existingHVRBasisBuiltInAddresses.Simulate()guard for the pattern). Expect this to be scrutinized closely in review.{ get; set; }properties or access lockdowns — Public fields are fine; Basis is meant to be read and modified freely, so don't wall things offprivate/internalwithout 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.Instancesingletons, callers reassigningType.Instanceis allowed; if that would break your code, log a warning or throw — don't block the assignment. Locking down access is not your call.BasisLocalCameraDriver— Code that needs the local camera (transform, projection, rig data, etc.) pulls it fromBasisLocalCameraDriverrather than looking one up itself. Don't roll a separate camera discovery path.BasisDebug— All new logging calls go throughBasisDebug.Log/BasisDebug.LogWarning/BasisDebug.LogError(with an appropriateLogTag) instead ofUnityEngine.Debug.Log/Debug.LogWarning/Debug.LogError.BasisDebugroutes through Basis's tagged, color-coded logger and respects the project-wideLoggingDisabledtoggle so logging can be killed at runtime; bareDebug.Logcalls bypass that and will be denied.FindObjectOfType/FindObjectsOfType/GameObject.Find/FindGameObjectsWithTagto 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.newon reference types, no LINQ, nostringconcatenation/interpolation, no boxing, noforeachover interface-typed collections. Allocate once at init and reuse the buffer.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_EDITORand remove (or leave gated) before merge..Count(lists) /.Length(arrays) into a localintbefore the loop instead of re-reading the property each iteration. PreferT[](with a separate length int when the array is over-sized) overList<T>where the data is hot — Unity's mono BCL doesn't exposeCollectionsMarshal.AsSpan(List<T>), so a list can't be fed intoSpan<T>/ unsafe paths cleanly. Where the perf justifies it, drop intoSpan<T>/reflocals /Unsafe.As/unsafepointer 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.
Input / control mode coverage:
Where applicable, confirm these flows still work after your changes:
Notes
This is almost entirely a native-plugin change under
Native~/. The one C# file it touches isBasisMediaPlayerSecurity.cs— the SSRF address classifier, extended (in lockstep with the nativebasis_io.cguard) 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), andtools/media-conformance(demux output still matches ffprobe on all synthetic fixtures, so the caps don't change valid-input playback). Portable C andbasis_win_decode.cppcompile 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
localhostURL on those transports is refused even in the Editor unlessBASIS_MEDIA_ALLOW_LOCALrelaxes the native re-check (it does not relax the top-level C# gate — a top-level RFC1918 URL stays refused regardless). Documented inTESTING.md.Known residuals (deliberate, tracked as follow-ups — not oversights):
RENDER_UPDATE-only, idempotent usage; fully closing it needs a generation-stamped handle in the event payload (a C# ABI change) — follow-up.