Repository navigation
VPAAMP-1542: Heap overflow (read and write) in ClearKey decrypt via untrusted CENC subsample lengths - #321
Open
pstroffolino wants to merge 27 commits into
Open
VPAAMP-1542: Heap overflow (read and write) in ClearKey decrypt via untrusted CENC subsample lengths#321pstroffolino wants to merge 27 commits into
pstroffolino wants to merge 27 commits into
Conversation
Reason for Change: Changes required for DirectRialto Test Procedure: Build and run AAMP with DirectRialto enabled Priority: P1 Risks: Medium
Reason for Change: Fix AAMP compilation failure after rebase Summary of Changes: - Restore the file DrmSession.cpp - Revert some of the changes made to DrmSession.h - Restore fakes, CMake files and references to DrmSession.cpp Test Procedure: Build AAMP-CLI and AAMP L1 tests Priority: P1 Risks: Low
IsKeyIdProcessed() only held cachedKeyMutex, so it could observe a slot another thread had just claimed (before createDrmSession() finished acquiring the license) and report it as already succeeded. It also only checked the per-entry isFailedKeyId flag, missing whole-slot failures tracked in isFailedKeyEntries. Now also takes mDrmSessionLock (same order createDrmSession() already uses) so it blocks until any in-flight session creation for that key has resolved, and checks isFailedKeyEntries too. Add regression tests to DrmSessionManagerTestCases.cpp.
…sion status" This reverts commit 14edcaa.
True when NAL units are length-prefixed (AVCC/HVCC, e.g. avcC/hvcC sample entries per ISO/IEC 14496-15); false for Annex-B (start-code delimited) bitstreams such as HLS-TS ES output. Meaningless for non-NAL-unit codecs (audio/subtitle), where it stays false.
PlayerCCManagerBase::GetId()/Release() implements a usage-counter pattern that is correct for PlayerSubtecCCManager's shared, out-of-process connection, but was reused as-is for the Rialto and direct-Rialto backends, where the "handle" is actually a per-pipeline object owned by a single playback session. In multi-pipeline mode this can leave a dangling handle in the singleton after its owning session is destroyed, since the refcount only tracks session count, not the validity of a specific handle. - Add PlayerCCManagerBase::InvalidateHandle(void*), a no-op by default, that a handle owner can call to clear its handle from the singleton ahead of Release(), independent of the GetId()/Release() count. - Add PlayerCCManager::HasInstance() so callers can check for an existing singleton without creating one. - Override InvalidateHandle() in PlayerDirectRialtoCCManager and PlayerRialtoCCManager to clear m_control/mSubtitleControlHandle only when it matches the given handle. This is a minimal mitigation (Option A); wiring InvalidateHandle() into PlayerRialtoCCManager's GStreamer-side handle owner, and a larger structural fix (shared preferences singleton + per-session state), are tracked separately as follow-up work.
InvalidateHandle() relied on an atomic compare_exchange to clear the control/handle pointer, but the atomic only protects the pointer value, not the lifetime of the object it points to. A concurrent SetTrack() / StartRendering() / StopRendering() call could load the pointer before the CAS and dereference it after the owner destroyed the object. - PlayerDirectRialtoCCManager: replace std::atomic<IDirectRialtoCC*> with a mutex (m_controlMutex) shared by InvalidateHandle() and all accessors, so invalidation blocks until any in-flight call on the old handle completes. - PlayerRialtoCCManager: apply the same mutex-guard fix for mSubtitleControlHandle. - PlayerDirectRialtoCCManager::SetTrack(): return the actual result of setTextTrackIdentifier() (0/-1) instead of always returning 0, per the base class contract. - Add SetTrack_ControlRejectsIdentifier_ReturnsFailure L1 test covering the failure path. Testing: PlayerDirectRialtoCCManagerTests (10/10) pass.
…nCDM sessions DrmSessionManager::getDrmSession() called setKeyId() only inside already made unconditional. Non-OpenCDM backends (e.g. Direct-Rialto) never received the key ID. Move the call inside the existing drmSession != NULL check and drop the guard; setKeyId() defaults to a no-op, so calling it unconditionally is safe for every backend. Remove the matching stale #ifdef around the setKeyId mock in MockDrmSession.h so it stays in sync with the base class. PlayerDirectRialtoCCManager::Initialize() discarded the return value of SetTrack() for both the default "CC1" track and the cached-track re-apply path, always returning 0 even when the control rejected the identifier. Propagate the failure as -1 in both cases so PlayerCCManagerBase::Init() correctly reports failure. Add Initialize_ControlRejectsDefaultTrack_ReturnsFailure and Initialize_ControlRejectsCachedTrack_ReturnsFailure to cover the new failure paths.
drm/DrmSessionManager.h and closedcaptions/PlayerCCManager.h added a
default argument to an existing constructor/static method
(DrmSessionManager's 4th param, SetRialto's 2nd param). Since both are
installed public headers, this silently changed the mangled symbol,
breaking binaries linked against the previous release. Restore the
old symbols by keeping a distinct overload that delegates to the new
one:
- DrmSessionManager(int, void*, callback) delegates to
DrmSessionManager(int, void*, callback, creator).
- PlayerCCManager::SetRialto(bool) delegates to
SetRialto(bool, bool).
PlayerDirectRialtoCCManager::Initialize() left m_control pointing at
the new handle even when SetTrack() failed to apply the track. A
later Initialize() retry with the same handle then saw
changedHandle == false and returned success without ever configuring
the control. Clear the control via a new clearControlOnFailure()
helper whenever SetTrack() fails during Initialize(), so a retry is
treated as a handle change and reattempts SetTrack().
mTrack/mTrackFormat were written in SetTrack() outside m_controlMutex
and read/cleared in Initialize()/ResetState() without any lock. Move
all three accesses under m_controlMutex; Initialize() takes a local
snapshot before releasing the lock to avoid recursive-lock deadlock
when it subsequently calls SetTrack().
Add Initialize_RetryAfterRejectedDefaultTrack_ReappliesTrack to cover
the retry fix.
PlayerDirectRialtoCCManager: - Fix deadlock in ResetState(): PlayerCCManagerBase::ResetState() calls Stop() -> StopRendering(), which re-enters m_controlMutex. Run the base call before acquiring the lock instead of wrapping the whole function, matching the original (pre-mutex) call order. - Update m_controlMutex's doc comment to reflect that ResetState() clears mTrack/mTrackFormat via the base class before taking the lock. - Strengthen the test double: PlayerCCManagerBase::ResetState() in PlayerDirectRialtoCCManagerBaseStubs.cpp now calls Stop(), mirroring the real base class, so ResetState_ClearsControlHandle actually exercises the mutex re-entrancy this fix avoids instead of masking it. DrmSessionManager: - Revert the 3-arg constructor overload added to preserve the old mangled symbol. m_sessionCreator already changed this class's size, so a prebuilt binary compiled before that member existed would still link successfully against the restored 3-arg symbol, then heap- corrupt: operator new sizes the allocation from the caller's old (smaller) layout, while the constructor initializes an object sized for the new (larger) one. Collapsing back to a single constructor with creator defaulted to nullptr forces such callers to fail at compile time instead of silently corrupting memory at runtime. Verified: PlayerDirectRialtoCCManagerTests (13/13), DrmSessionManagerTests (29/29), DrmAes (33/34, 1 pre-existing skip), DrmHelperTests (160/160), DrmOcdmTests (96/96).
- PlayerCCManager.h: move InvalidateHandle() to the end of PlayerCCManagerBase's virtual interface. It was previously declared between Release() and SetStatus(), which shifts every subsequent vtable slot and silently mis-dispatches calls from binaries built against an older header. - PlayerCCManager.h/.cpp: split ResetState() into Stop() + a new ResetTrackState() helper that only resets the cached track/rendering fields, without acquiring any lock itself. PlayerDirectRialtoCCManager and PlayerRialtoCCManager's ResetState() now call Stop() unlocked (still required to avoid deadlocking through StopRendering()'s re-entrant lock), then call ResetTrackState() and clear their handle while holding their own control mutex, instead of invoking PlayerCCManagerBase::ResetState() directly - removing a race between ResetState() and concurrent SetTrack()/Initialize() calls on those fields. Updated PlayerDirectRialtoCCManagerBaseStubs.cpp with a matching ResetTrackState() stub. - PlayerRialtoCCManager.cpp/.h: move mTrack/mTrackFormat writes inside mControlMutex in SetTrack(), and snapshot them under the same lock as mSubtitleControlHandle in Initialize(), removing a data race with concurrent SetTrack()/Initialize() calls. Mirrors the fix already applied to PlayerDirectRialtoCCManager. Updated the mControlMutex doc comment to reflect the additional state it now guards. - PlayerDirectRialtoCCManager.cpp: only update the cached mTrack/ mTrackFormat once setTextTrackIdentifier() confirms success. A rejected track previously still updated the cache, so a later Initialize() with the same handle would see changedHandle == false and skip reapplying the last-accepted track. Added SetTrack_ControlRejectsIdentifier_CacheKeepsPreviousTrack to cover it. - DrmSessionManagerTestCases.cpp: add coverage for the DrmSessionCreator injection path in getDrmSession(), which previously had no test exercising it. Extend TestableDrmSessionManager with a constructor overload forwarding a DrmSessionCreator, and add tests verifying (a) the creator is invoked and its returned session's ownership is transferred into the selected slot, and (b) a null creator result is reported as MW_DRM_INIT_FAILED.
PlayerCCManager::SetRialto() had a two-argument version plus a one-argument overload kept only to preserve a "pre-existing mangled symbol for binaries linked against older releases." That reasoning doesn't hold: like DrmSessionManager's DrmSessionCreator parameter, this library is always rebuilt from source with its consumers, so there's no stable ABI symbol to preserve. Replace the two overloads with a single SetRialto(bool bIsRialto, bool bIsDirectRialto = false), matching the pattern already used for DrmSessionManager. Existing single-argument call sites continue to compile unchanged.
…sing AcquireLicenseCb PlayerRialtoCCManager and PlayerDirectRialtoCCManager released their control mutex between assigning the new handle and applying the cached track via SetTrack(), letting a concurrent Initialize()/SetTrack() call interleave and configure the wrong handle. Add a private SetTrackLocked() in each and hold the mutex for Initialize()'s entire body so handle assignment and configuration happen atomically. This lets PlayerDirectRialtoCCManager drop clearControlOnFailure() in favor of an inline reset, since the lock is now held continuously from handle assignment through configuration. Also set err = MW_DRM_INIT_FAILED in DrmSessionManager::createDrmSession() when AcquireLicenseCb isn't registered, matching sibling failure paths; previously err was left unset and callers' `if (err != -1)` check silently swallowed the failure.
RDKEMW-23999 Direct Rialto - Add changes required for Direct Rialto integration
…y caps setting in mp4demux Reason for change:Use codec info from the manifest rather than lazy caps setting in mp4demux Risks: p1 Signed-off-by: varshnie <varshniblue14@gmail.com>
…y caps setting in mp4demux Reason for change:Use codec info from the manifest rather than lazy caps setting in mp4demux without cfg Risks: p1 Signed-off-by: varshnie <varshniblue14@gmail.com>
… and minor cleanup
VPAAMP-959:[MP4DEMUX]Use codec info from the manifest rather than laz…
VPAAMP-1341: Fix MP4 demux unknown-format stream setup
Reason for change: EOS only fires when both audio and video have resumed data flow for a normal discontinuity — never for just one side Test Procedure: Refere ticket Risks: low Priority: P1 Co-authored-by: rekha kandhavelan <rekha_kandhavelan@comcast.com>
Reason for change: ClearKeySession::decrypt() trusted the subsample table from the media segment (senc). Clear/encrypted lengths were never checked against the sample size, so a crafted ClearKey stream could cause heap out-of-bounds reads and writes in the gather and scatter memcpy loops. Validate the whole subsample table before allocating or copying: reject truncated tables and tables whose summed ranges (64-bit, so no wraparound) exceed the sample buffer. Also handle malloc failure. Test Procedure: New ClearKeySubSampleDecryptTests in DrmOcdmTests cover valid, exact-fit, oversized, cumulative-overflow, wraparound and truncated subsample tables. The gst mock can now map real buffer data. Risks: Low. Valid content follows the existing decrypt path. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The change introduces a cross-track data race that can incorrectly suppress EOS and advertises encrypted elementary streams as clear when no DRM system is configured, preventing decryptor selection.
4 open issues
What changed in this PR
This PR hardens ClearKey decryption against malicious CENC subsample lengths. It also extends the player’s codec metadata, DRM session integration, and direct-Rialto closed-caption support.
Changes:
- Validate subsample ranges before copying encrypted data, with regression tests for oversized, overflowing, and truncated inputs.
- Carry encryption metadata through pipeline configuration and return thread-safe snapshots of usable DRM keys.
- Add injectable DRM session creation, direct-Rialto caption controls, and related lifecycle and test updates.
| File | Description |
|---|---|
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTests.cpp | Adds the caption test runner. |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp | Tests caption selection, failures, muting, and reset. |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerBaseStubs.cpp | Supplies caption base-class test stubs. |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/CMakeLists.txt | Builds the new caption test suite. |
| test/utests/tests/OCDMSessionAdapter/FunctionalTests.cpp | Replaces an expanded mock call with EXPECT_CALL. |
| test/utests/tests/OcdmBasicSessionAdapterTests/FunctionalTests.cpp | Updates usable-key mocks to return copies. |
| test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp | Tests encryption metadata, caps, and reconfiguration. |
| test/utests/tests/GstPlayer/PauseOnPlaybackTests.cpp | Migrates pipeline calls to codec metadata. |
| test/utests/tests/GstPlayer/FunctionalTests.cpp | Updates pipeline configuration arguments. |
| test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp | Tests injected session creation and updates key mocks. |
| test/utests/tests/DrmOcdmTests/CMakeLists.txt | Registers ClearKey subsample tests. |
| test/utests/tests/DrmOcdmTests/ClearKeySubSampleDecryptTests.cpp | Tests valid and malformed subsample ranges. |
| test/utests/tests/CMakeLists.txt | Registers the direct-Rialto caption suite. |
| test/utests/tests/ClosedCaptionsTests/CMakeLists.txt | Adds direct-Rialto header access. |
| test/utests/mocks/MockOpenCdmSessionAdapter.h | Returns usable keys by value. |
| test/utests/mocks/MockDrmSession.h | Makes key-ID mocking backend-independent. |
| test/utests/fakes/Fakeopencdmsessionadapter.cpp | Adopts snapshot return semantics. |
| test/utests/fakes/FakeDRMSessionManager.cpp | Matches the injectable constructor signature. |
| test/utests/fakes/CMakeLists.txt | Adds Rialto test include paths. |
| test/utests/drm/mocks/gstMocks.h | Declares buffer-mapping test helpers. |
| test/utests/drm/mocks/gstMocks.c | Adds mapped-buffer data and byte-reader positioning. |
| test/pi-cli/commandProcessing.cpp | Passes structured codec information. |
| InterfacePlayerRDK.h | Declares the codec-aware pipeline overload. |
| InterfacePlayerRDK.cpp | Updates encrypted caps, stream metadata, and discontinuity handling. |
| InterfacePlayerPriv.h | Stores codec metadata per stream. |
| drm/ocdm/opencdmsessionadapter.h | Declares usable-key snapshot returns. |
| drm/ocdm/opencdmsessionadapter.cpp | Copies usable keys while holding the mutex. |
| drm/DrmSessionManager.h | Defines and stores an injectable session creator. |
| drm/DrmSessionManager.cpp | Uses injected creators and guards missing license callbacks. |
| drm/DrmSession.h | Extends backend-neutral session APIs. |
| drm/DrmSession.cpp | Removes the reference-returning key implementation. |
| drm/ClearKeyDrmSession.cpp | Validates subsample bounds before allocation and copying. |
| DemuxDataTypes.h | Adds NAL framing metadata and grouped track codecs. |
| CMakeLists.txt | Builds direct-Rialto captions and installs their interface. |
| closedcaptions/rialto/PlayerRialtoCCManager.h | Declares handle locking and invalidation. |
| closedcaptions/rialto/PlayerRialtoCCManager.cpp | Synchronizes control access and cached track updates. |
| closedcaptions/PlayerCCManager.h | Adds backend selection and handle lifecycle APIs. |
| closedcaptions/PlayerCCManager.cpp | Creates direct-Rialto managers and separates state reset. |
| closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h | Declares the direct-Rialto caption manager. |
| closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp | Implements caption controls and handle lifecycle management. |
| closedcaptions/direct-rialto/IDirectRialtoCC.h | Defines the backend-independent caption control interface. |
Review effort: Balanced
Comment on lines
+106
to
+110
| if (drmSystem == NULL) | ||
| { | ||
| MW_LOG_WARN("Skipping encrypted caps conversion: DRM system ID is NULL"); | ||
| return; | ||
| } |
| if (!codecChange && siblingType != eGST_MEDIATYPE_DEFAULT) | ||
| { | ||
| gst_media_stream *siblingStream = &interfacePlayerPriv->gstPrivateContext->stream[siblingType]; | ||
| if (siblingStream->format != GST_FORMAT_INVALID && !siblingStream->firstBufferProcessed) |
Comment on lines
+3815
to
+3817
| if ((type == eGST_MEDIATYPE_AUDIO) && (interfacePlayerPriv->gstPrivateContext->stream[eGST_MEDIATYPE_SUBTITLE].source)) | ||
| { | ||
| GstPlayer_SignalEOS(interfacePlayerPriv->gstPrivateContext->stream[eGST_MEDIATYPE_SUBTITLE]); |
Comment on lines
+3267
to
+3268
| EXPECT_CALL(*g_mockGstUtils, GetCaps(GST_FORMAT_ISO_BMFF)).WillOnce(Return(&caps)); | ||
| EXPECT_CALL(*g_mockGStreamer, gst_caps_get_structure(_, _)).Times(0); |
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.
Security protection for a malicious ClearKey DASH stream