From 4aabc95234e76838ab5014436cd4882cc5334113 Mon Sep 17 00:00:00 2001 From: Chong Zhang Date: Wed, 21 Mar 2018 15:52:21 -0700 Subject: [PATCH] cas: fix UAF in descrambler Change the plugin holder in both CasImpl and DescramblerImpl to shared_ptr, and use atomic store/load for read/write. bug: 73172817 Test:CTS MediaCasTest, VTS VtsHalCasV1_0Target, poc in bug Change-Id: I3f1472d3b9d8d3dc74168c07325c5c319a96807d --- cas/1.0/default/CasImpl.cpp | 82 +++++++++++++---------------- cas/1.0/default/CasImpl.h | 2 +- cas/1.0/default/DescramblerImpl.cpp | 46 ++++++++++++---- cas/1.0/default/DescramblerImpl.h | 2 +- 4 files changed, 73 insertions(+), 59 deletions(-) diff --git a/cas/1.0/default/CasImpl.cpp b/cas/1.0/default/CasImpl.cpp index 2ac1c4f6dd..178020e477 100644 --- a/cas/1.0/default/CasImpl.cpp +++ b/cas/1.0/default/CasImpl.cpp @@ -31,19 +31,8 @@ namespace cas { namespace V1_0 { namespace implementation { -struct CasImpl::PluginHolder : public RefBase { -public: - explicit PluginHolder(CasPlugin *plugin) : mPlugin(plugin) {} - ~PluginHolder() { if (mPlugin != NULL) delete mPlugin; } - CasPlugin* get() { return mPlugin; } - -private: - CasPlugin *mPlugin; - DISALLOW_EVIL_CONSTRUCTORS(PluginHolder); -}; - CasImpl::CasImpl(const sp &listener) - : mPluginHolder(NULL), mListener(listener) { + : mListener(listener) { ALOGV("CTOR"); } @@ -69,7 +58,8 @@ void CasImpl::OnEvent( void CasImpl::init(const sp& library, CasPlugin *plugin) { mLibrary = library; - mPluginHolder = new PluginHolder(plugin); + std::shared_ptr holder(plugin); + std::atomic_store(&mPluginHolder, holder); } void CasImpl::onEvent( @@ -88,22 +78,22 @@ void CasImpl::onEvent( Return CasImpl::setPrivateData(const HidlCasData& pvtData) { ALOGV("%s", __FUNCTION__); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus(holder->get()->setPrivateData(pvtData)); + return toStatus(holder->setPrivateData(pvtData)); } Return CasImpl::openSession(openSession_cb _hidl_cb) { ALOGV("%s", __FUNCTION__); CasSessionId sessionId; - sp holder = mPluginHolder; + std::shared_ptr holder = std::atomic_load(&mPluginHolder); status_t err = INVALID_OPERATION; - if (holder != NULL) { - err = holder->get()->openSession(&sessionId); - holder.clear(); + if (holder.get() != nullptr) { + err = holder->openSession(&sessionId); + holder.reset(); } _hidl_cb(toStatus(err), sessionId); @@ -115,87 +105,87 @@ Return CasImpl::setSessionPrivateData( const HidlCasSessionId &sessionId, const HidlCasData& pvtData) { ALOGV("%s: sessionId=%s", __FUNCTION__, sessionIdToString(sessionId).string()); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus( - holder->get()->setSessionPrivateData( - sessionId, pvtData)); + return toStatus(holder->setSessionPrivateData(sessionId, pvtData)); } Return CasImpl::closeSession(const HidlCasSessionId &sessionId) { ALOGV("%s: sessionId=%s", __FUNCTION__, sessionIdToString(sessionId).string()); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus(holder->get()->closeSession(sessionId)); + return toStatus(holder->closeSession(sessionId)); } Return CasImpl::processEcm( const HidlCasSessionId &sessionId, const HidlCasData& ecm) { ALOGV("%s: sessionId=%s", __FUNCTION__, sessionIdToString(sessionId).string()); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus(holder->get()->processEcm(sessionId, ecm)); + return toStatus(holder->processEcm(sessionId, ecm)); } Return CasImpl::processEmm(const HidlCasData& emm) { ALOGV("%s", __FUNCTION__); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus(holder->get()->processEmm(emm)); + return toStatus(holder->processEmm(emm)); } Return CasImpl::sendEvent( int32_t event, int32_t arg, const HidlCasData& eventData) { ALOGV("%s", __FUNCTION__); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - status_t err = holder->get()->sendEvent(event, arg, eventData); + status_t err = holder->sendEvent(event, arg, eventData); return toStatus(err); } Return CasImpl::provision(const hidl_string& provisionString) { ALOGV("%s: provisionString=%s", __FUNCTION__, provisionString.c_str()); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - return toStatus(holder->get()->provision(String8(provisionString.c_str()))); + return toStatus(holder->provision(String8(provisionString.c_str()))); } Return CasImpl::refreshEntitlements( int32_t refreshType, const HidlCasData& refreshData) { ALOGV("%s", __FUNCTION__); - sp holder = mPluginHolder; - if (holder == NULL) { + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { return toStatus(INVALID_OPERATION); } - status_t err = holder->get()->refreshEntitlements(refreshType, refreshData); + status_t err = holder->refreshEntitlements(refreshType, refreshData); return toStatus(err); } Return CasImpl::release() { - ALOGV("%s: plugin=%p", __FUNCTION__, - mPluginHolder != NULL ? mPluginHolder->get() : NULL); - mPluginHolder.clear(); + ALOGV("%s: plugin=%p", __FUNCTION__, mPluginHolder.get()); + + std::shared_ptr holder(nullptr); + std::atomic_store(&mPluginHolder, holder); + return Status::OK; } diff --git a/cas/1.0/default/CasImpl.h b/cas/1.0/default/CasImpl.h index 841d64e038..d7928381cc 100644 --- a/cas/1.0/default/CasImpl.h +++ b/cas/1.0/default/CasImpl.h @@ -88,7 +88,7 @@ public: private: struct PluginHolder; sp mLibrary; - sp mPluginHolder; + std::shared_ptr mPluginHolder; sp mListener; DISALLOW_EVIL_CONSTRUCTORS(CasImpl); diff --git a/cas/1.0/default/DescramblerImpl.cpp b/cas/1.0/default/DescramblerImpl.cpp index 36699baf28..1f8993379f 100644 --- a/cas/1.0/default/DescramblerImpl.cpp +++ b/cas/1.0/default/DescramblerImpl.cpp @@ -50,12 +50,12 @@ CHECK_SUBSAMPLE_DEF(CryptoPlugin); DescramblerImpl::DescramblerImpl( const sp& library, DescramblerPlugin *plugin) : - mLibrary(library), mPlugin(plugin) { - ALOGV("CTOR: mPlugin=%p", mPlugin); + mLibrary(library), mPluginHolder(plugin) { + ALOGV("CTOR: plugin=%p", mPluginHolder.get()); } DescramblerImpl::~DescramblerImpl() { - ALOGV("DTOR: mPlugin=%p", mPlugin); + ALOGV("DTOR: plugin=%p", mPluginHolder.get()); release(); } @@ -63,12 +63,22 @@ Return DescramblerImpl::setMediaCasSession(const HidlCasSessionId& sessi ALOGV("%s: sessionId=%s", __FUNCTION__, sessionIdToString(sessionId).string()); - return toStatus(mPlugin->setMediaCasSession(sessionId)); + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { + return toStatus(INVALID_OPERATION); + } + + return toStatus(holder->setMediaCasSession(sessionId)); } Return DescramblerImpl::requiresSecureDecoderComponent( const hidl_string& mime) { - return mPlugin->requiresSecureDecoderComponent(String8(mime.c_str())); + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { + return false; + } + + return holder->requiresSecureDecoderComponent(String8(mime.c_str())); } static inline bool validateRangeForSize( @@ -86,12 +96,23 @@ Return DescramblerImpl::descramble( descramble_cb _hidl_cb) { ALOGV("%s", __FUNCTION__); + // Get a local copy of the shared_ptr for the plugin. Note that before + // calling the HIDL callback, this shared_ptr must be manually reset, + // since the client side could proceed as soon as the callback is called + // without waiting for this method to go out of scope. + std::shared_ptr holder = std::atomic_load(&mPluginHolder); + if (holder.get() == nullptr) { + _hidl_cb(toStatus(INVALID_OPERATION), 0, NULL); + return Void(); + } + sp srcMem = mapMemory(srcBuffer.heapBase); // Validate if the offset and size in the SharedBuffer is consistent with the // mapped ashmem, since the offset and size is controlled by client. if (srcMem == NULL) { ALOGE("Failed to map src buffer."); + holder.reset(); _hidl_cb(toStatus(BAD_VALUE), 0, NULL); return Void(); } @@ -100,6 +121,7 @@ Return DescramblerImpl::descramble( ALOGE("Invalid src buffer range: offset %llu, size %llu, srcMem size %llu", srcBuffer.offset, srcBuffer.size, (uint64_t)srcMem->getSize()); android_errorWriteLog(0x534e4554, "67962232"); + holder.reset(); _hidl_cb(toStatus(BAD_VALUE), 0, NULL); return Void(); } @@ -117,6 +139,7 @@ Return DescramblerImpl::descramble( "srcOffset %llu, totalBytesInSubSamples %llu, srcBuffer size %llu", srcOffset, totalBytesInSubSamples, srcBuffer.size); android_errorWriteLog(0x534e4554, "67962232"); + holder.reset(); _hidl_cb(toStatus(BAD_VALUE), 0, NULL); return Void(); } @@ -135,6 +158,7 @@ Return DescramblerImpl::descramble( "dstOffset %llu, totalBytesInSubSamples %llu, srcBuffer size %llu", dstOffset, totalBytesInSubSamples, srcBuffer.size); android_errorWriteLog(0x534e4554, "67962232"); + holder.reset(); _hidl_cb(toStatus(BAD_VALUE), 0, NULL); return Void(); } @@ -146,7 +170,7 @@ Return DescramblerImpl::descramble( // Casting hidl SubSample to DescramblerPlugin::SubSample, but need // to ensure structs are actually idential - int32_t result = mPlugin->descramble( + int32_t result = holder->descramble( dstBuffer.type != BufferType::SHARED_MEMORY, (DescramblerPlugin::ScramblingControl)scramblingControl, subSamples.size(), @@ -157,17 +181,17 @@ Return DescramblerImpl::descramble( dstOffset, NULL); + holder.reset(); _hidl_cb(toStatus(result >= 0 ? OK : result), result, NULL); return Void(); } Return DescramblerImpl::release() { - ALOGV("%s: mPlugin=%p", __FUNCTION__, mPlugin); + ALOGV("%s: plugin=%p", __FUNCTION__, mPluginHolder.get()); + + std::shared_ptr holder(nullptr); + std::atomic_store(&mPluginHolder, holder); - if (mPlugin != NULL) { - delete mPlugin; - mPlugin = NULL; - } return Status::OK; } diff --git a/cas/1.0/default/DescramblerImpl.h b/cas/1.0/default/DescramblerImpl.h index d3b146ecc7..305f115473 100644 --- a/cas/1.0/default/DescramblerImpl.h +++ b/cas/1.0/default/DescramblerImpl.h @@ -55,7 +55,7 @@ public: private: sp mLibrary; - DescramblerPlugin *mPlugin; + std::shared_ptr mPluginHolder; DISALLOW_EVIL_CONSTRUCTORS(DescramblerImpl); };