From 5872b8058aedcfca7fe658cf92302023dce902f2 Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Tue, 21 Jun 2022 01:36:43 +0000 Subject: [PATCH 1/2] [Fix vulnerability] setSecurityLevel in clearkey Potential race condition in clearkey setSecurityLevel. POC test in http://go/ag/19083795 Test: sts-tradefed run sts-dynamic-develop -m StsHostTestCases -t android.security.sts.CVE_2022_2209#testPocCVE_2022_2209 Bug: 235601882 Change-Id: I6447fb539ef0cb395772c61e6f3e1504ccde331b Merged-In: I2e2084e85fe45d7d7f958c59b0063a477c7d24bf (cherry picked from commit 9bfc2fbcc4be68bc8939a10dd7942845dc724f75) Merged-In: I6447fb539ef0cb395772c61e6f3e1504ccde331b --- drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp | 2 ++ drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp b/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp index 6a374f9150..24b12e59ba 100644 --- a/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp +++ b/drm/mediadrm/plugins/clearkey/hidl/DrmPlugin.cpp @@ -624,6 +624,7 @@ Return DrmPlugin::getSecurityLevel(const hidl_vec& sessionId, return Void(); } + Mutex::Autolock lock(mSecurityLevelLock); std::map, SecurityLevel>::iterator itr = mSecurityLevel.find(sid); if (itr == mSecurityLevel.end()) { @@ -696,6 +697,7 @@ Return DrmPlugin::setSecurityLevel(const hidl_vec& sessionId, return Status::ERROR_DRM_SESSION_NOT_OPENED; } + Mutex::Autolock lock(mSecurityLevelLock); std::map, SecurityLevel>::iterator itr = mSecurityLevel.find(sid); if (itr != mSecurityLevel.end()) { diff --git a/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h b/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h index 5d6e3daca1..7836f96a60 100644 --- a/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h +++ b/drm/mediadrm/plugins/clearkey/hidl/include/DrmPlugin.h @@ -414,7 +414,8 @@ struct DrmPlugin : public IDrmPlugin { std::map > mByteArrayProperties; std::map > mReleaseKeysMap; std::map, std::string> mPlaybackId; - std::map, SecurityLevel> mSecurityLevel; + std::map, SecurityLevel> mSecurityLevel + GUARDED_BY(mSecurityLevelLock); sp mListener; sp mListenerV1_2; SessionLibrary *mSessionLibrary; @@ -435,6 +436,7 @@ struct DrmPlugin : public IDrmPlugin { DeviceFiles mFileHandle GUARDED_BY(mFileHandleLock); Mutex mFileHandleLock; Mutex mSecureStopLock; + Mutex mSecurityLevelLock; CLEARKEY_DISALLOW_COPY_AND_ASSIGN_AND_NEW(DrmPlugin); }; From ba8c00348154d60e9e5dde7c35f4758a16f54e80 Mon Sep 17 00:00:00 2001 From: Girish Date: Wed, 21 Sep 2022 19:18:49 +0000 Subject: [PATCH 2/2] libstagefright: fix heap use after free issue Refactor NuMediaExtractor::setDataSource to move the common code under a function to reduce code duplication. And also, fixing a heap based string dereference after free in NuMediaExtractor::getName Bug: 245242273 Test: atest android.mediav2.cts.ExtractorTest atest MediaSampleReaderNDKTests:MediaSampleReaderNDKTests.MediaSampleReaderNDKTests Change-Id: Ie6c663d2df85bee5a2d28a9b709aadf4b6cdd47f (cherry picked from commit 9a3fb4bfd9c43edbeac59d8b8f7253140fb5c0c1) (cherry picked from commit 2bddcbdd0c25b434920c87d74a11d0d63fd7edea) Merged-In: Ie6c663d2df85bee5a2d28a9b709aadf4b6cdd47f --- media/libstagefright/NuMediaExtractor.cpp | 95 +++++++------------ .../media/stagefright/NuMediaExtractor.h | 2 + 2 files changed, 36 insertions(+), 61 deletions(-) diff --git a/media/libstagefright/NuMediaExtractor.cpp b/media/libstagefright/NuMediaExtractor.cpp index 68933242a8..2b45f2d16d 100644 --- a/media/libstagefright/NuMediaExtractor.cpp +++ b/media/libstagefright/NuMediaExtractor.cpp @@ -72,31 +72,17 @@ NuMediaExtractor::~NuMediaExtractor() { } } -status_t NuMediaExtractor::setDataSource( - const sp &httpService, - const char *path, - const KeyedVector *headers) { - Mutex::Autolock autoLock(mLock); - - if (mImpl != NULL || path == NULL) { - return -EINVAL; - } - - sp dataSource = - DataSourceFactory::getInstance()->CreateFromURI(httpService, path, headers); - - if (dataSource == NULL) { - return -ENOENT; - } +status_t NuMediaExtractor::initMediaExtractor(const sp& dataSource) { + status_t err = OK; mImpl = MediaExtractorFactory::Create(dataSource); - if (mImpl == NULL) { + ALOGE("%s: failed to create MediaExtractor", __FUNCTION__); return ERROR_UNSUPPORTED; } + setEntryPointToRemoteMediaExtractor(); - status_t err = OK; if (!mCasToken.empty()) { err = mImpl->setMediaCas(mCasToken); if (err != OK) { @@ -105,6 +91,10 @@ status_t NuMediaExtractor::setDataSource( } } + // Get the name of the implementation. + mName = mImpl->name(); + + // Update the duration and bitrate err = updateDurationAndBitrate(); if (err == OK) { mDataSource = dataSource; @@ -113,6 +103,27 @@ status_t NuMediaExtractor::setDataSource( return OK; } +status_t NuMediaExtractor::setDataSource( + const sp &httpService, + const char *path, + const KeyedVector *headers) { + Mutex::Autolock autoLock(mLock); + + if (mImpl != NULL || path == NULL) { + return -EINVAL; + } + + sp dataSource = + DataSourceFactory::getInstance()->CreateFromURI(httpService, path, headers); + + if (dataSource == NULL) { + return -ENOENT; + } + + // Initialize MediaExtractor using the data source + return initMediaExtractor(dataSource); +} + status_t NuMediaExtractor::setDataSource(int fd, off64_t offset, off64_t size) { ALOGV("setDataSource fd=%d (%s), offset=%lld, length=%lld", @@ -131,27 +142,8 @@ status_t NuMediaExtractor::setDataSource(int fd, off64_t offset, off64_t size) { return err; } - mImpl = MediaExtractorFactory::Create(fileSource); - - if (mImpl == NULL) { - return ERROR_UNSUPPORTED; - } - setEntryPointToRemoteMediaExtractor(); - - if (!mCasToken.empty()) { - err = mImpl->setMediaCas(mCasToken); - if (err != OK) { - ALOGE("%s: failed to setMediaCas (%d)", __FUNCTION__, err); - return err; - } - } - - err = updateDurationAndBitrate(); - if (err == OK) { - mDataSource = fileSource; - } - - return OK; + // Initialize MediaExtractor using the file source + return initMediaExtractor(fileSource); } status_t NuMediaExtractor::setDataSource(const sp &source) { @@ -166,32 +158,13 @@ status_t NuMediaExtractor::setDataSource(const sp &source) { return err; } - mImpl = MediaExtractorFactory::Create(source); - - if (mImpl == NULL) { - return ERROR_UNSUPPORTED; - } - setEntryPointToRemoteMediaExtractor(); - - if (!mCasToken.empty()) { - err = mImpl->setMediaCas(mCasToken); - if (err != OK) { - ALOGE("%s: failed to setMediaCas (%d)", __FUNCTION__, err); - return err; - } - } - - err = updateDurationAndBitrate(); - if (err == OK) { - mDataSource = source; - } - - return err; + // Initialize MediaExtractor using the given data source + return initMediaExtractor(source); } const char* NuMediaExtractor::getName() const { Mutex::Autolock autoLock(mLock); - return mImpl == nullptr ? nullptr : mImpl->name().string(); + return mImpl == nullptr ? nullptr : mName.string(); } static String8 arrayToString(const std::vector &array) { diff --git a/media/libstagefright/include/media/stagefright/NuMediaExtractor.h b/media/libstagefright/include/media/stagefright/NuMediaExtractor.h index d17a480bb8..52ea28b9ca 100644 --- a/media/libstagefright/include/media/stagefright/NuMediaExtractor.h +++ b/media/libstagefright/include/media/stagefright/NuMediaExtractor.h @@ -146,6 +146,7 @@ struct NuMediaExtractor : public RefBase { Vector mSelectedTracks; int64_t mTotalBitrate; // in bits/sec int64_t mDurationUs; + String8 mName; void setEntryPointToRemoteMediaExtractor(); @@ -165,6 +166,7 @@ struct NuMediaExtractor : public RefBase { bool getTotalBitrate(int64_t *bitRate) const; status_t updateDurationAndBitrate(); status_t appendVorbisNumPageSamples(MediaBufferBase *mbuf, const sp &buffer); + status_t initMediaExtractor(const sp& dataSource); DISALLOW_EVIL_CONSTRUCTORS(NuMediaExtractor); };