From 3ba0bbe12904d0dfa2245fa3abf2b92034b15db3 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 10:56:13 -0800 Subject: IOMX.cpp uninitialized pointer in BnOMX::onTransact This can lead to local code execution in media server. Fix initializes the pointer and checks the error conditions before returning Bug: 26403627 Change-Id: I7fa90682060148448dba01d6acbe3471d1ddb500 --- media/libmedia/IOMX.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 5423c2a..8f55eb9 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -446,7 +446,7 @@ public: remote()->transact(CONFIGURE_VIDEO_TUNNEL_MODE, data, &reply); status_t err = reply.readInt32(); - if (sidebandHandle) { + if (err == OK && sidebandHandle) { *sidebandHandle = (native_handle_t *)reply.readNativeHandle(); } return err; @@ -948,11 +948,13 @@ status_t BnOMX::onTransact( OMX_BOOL tunneled = (OMX_BOOL)data.readInt32(); OMX_U32 audio_hw_sync = data.readInt32(); - native_handle_t *sideband_handle; + native_handle_t *sideband_handle = NULL; status_t err = configureVideoTunnelMode( node, port_index, tunneled, audio_hw_sync, &sideband_handle); reply->writeInt32(err); - reply->writeNativeHandle(sideband_handle); + if(err == OK){ + reply->writeNativeHandle(sideband_handle); + } return NO_ERROR; } -- cgit v1.1 From 638e5bad20b9d05f0d10b5e216cf5860a18ffc70 Mon Sep 17 00:00:00 2001 From: Jeff Tinker Date: Mon, 8 Feb 2016 17:52:25 -0800 Subject: Fix info leak vulnerability of IDrm bug: 26323455 Change-Id: I25bb30d3666ab38d5150496375ed2f55ecb23ba8 --- media/libmedia/IDrm.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/media/libmedia/IDrm.cpp b/media/libmedia/IDrm.cpp index b1ad0c5..7c709cd 100644 --- a/media/libmedia/IDrm.cpp +++ b/media/libmedia/IDrm.cpp @@ -658,7 +658,7 @@ status_t BnDrm::onTransact( Vector request; String8 defaultUrl; - DrmPlugin::KeyRequestType keyRequestType; + DrmPlugin::KeyRequestType keyRequestType = DrmPlugin::kKeyRequestType_Unknown; status_t result = getKeyRequest(sessionId, initData, mimeType, keyType, optionalParameters, request, defaultUrl, -- cgit v1.1 From 57bf4973b57dc62e7171c1cb2e0854f9e21fd3e4 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Mon, 8 Feb 2016 17:07:06 -0800 Subject: 3 uninitialized variables in IOMX.cpp Uninitialized MetadataBufferType pointer in case: SET_INPUT_SURFACE STORE_META_DATA_IN_BUFFERS CREATE_INPUT_SURFACE Fix: initialize them to kMetadataBufferTypeInvalid Bug: 26324358 Change-Id: Ib3bcac9bedc98bc65efa39f67cdbffbd90b374c9 --- media/libmedia/IOMX.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 8f55eb9..07f3697 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -844,9 +844,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); sp bufferProducer; - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = createInputSurface(node, port_index, &bufferProducer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); @@ -886,9 +890,13 @@ status_t BnOMX::onTransact( sp bufferConsumer = interface_cast(data.readStrongBinder()); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = setInputSurface(node, port_index, bufferConsumer, &type); + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); return NO_ERROR; @@ -914,8 +922,13 @@ status_t BnOMX::onTransact( OMX_U32 port_index = data.readInt32(); OMX_BOOL enable = (OMX_BOOL)data.readInt32(); - MetadataBufferType type; + MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); + + if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { + android_errorWriteLog(0x534e4554, "26324358"); + } + reply->writeInt32(type); reply->writeInt32(err); -- cgit v1.1 From 1f76ce4e2c13d0347523e8c9a27077c820715f08 Mon Sep 17 00:00:00 2001 From: "mspector@google.com" Date: Fri, 19 Feb 2016 12:10:48 -0800 Subject: Fixing safteynet logging bug introduced in ag/862848 We moved the safteynet logging into storeMetaDataInBuffers_l Bug: 26324358 Change-Id: I2171742b53192ebb71d546bcb8970bb3c68f366f --- media/libmedia/IOMX.cpp | 4 ---- media/libstagefright/omx/OMXNodeInstance.cpp | 1 + 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 07f3697..97b7ff0 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -925,10 +925,6 @@ status_t BnOMX::onTransact( MetadataBufferType type = kMetadataBufferTypeInvalid; status_t err = storeMetaDataInBuffers(node, port_index, enable, &type); - if ((err != OK) && (type == kMetadataBufferTypeInvalid)) { - android_errorWriteLog(0x534e4554, "26324358"); - } - reply->writeInt32(type); reply->writeInt32(err); diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index 94a213a..8735eff 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -519,6 +519,7 @@ status_t OMXNodeInstance::storeMetaDataInBuffers( status_t OMXNodeInstance::storeMetaDataInBuffers_l( OMX_U32 portIndex, OMX_BOOL enable, MetadataBufferType *type) { if (portIndex != kPortIndexInput && portIndex != kPortIndexOutput) { + android_errorWriteLog(0x534e4554, "26324358"); return BAD_VALUE; } -- cgit v1.1 From 3a90a02bb6c4e8352112dbb9f2316935e4dd7315 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Mon, 22 Feb 2016 13:05:15 -0800 Subject: Clear allocation to avoid info leak Bug: 26914474 Change-Id: Ie1a86e86d78058d041149fe599a4996e7f8185cf --- media/libmedia/IOMX.cpp | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/media/libmedia/IOMX.cpp b/media/libmedia/IOMX.cpp index 97b7ff0..865d575 100644 --- a/media/libmedia/IOMX.cpp +++ b/media/libmedia/IOMX.cpp @@ -692,34 +692,39 @@ status_t BnOMX::onTransact( size_t size = data.readInt64(); - void *params = malloc(size); - data.read(params, size); - - status_t err; - switch (code) { - case GET_PARAMETER: - err = getParameter(node, index, params, size); - break; - case SET_PARAMETER: - err = setParameter(node, index, params, size); - break; - case GET_CONFIG: - err = getConfig(node, index, params, size); - break; - case SET_CONFIG: - err = setConfig(node, index, params, size); - break; - case SET_INTERNAL_OPTION: - { - InternalOptionType type = - (InternalOptionType)data.readInt32(); - - err = setInternalOption(node, index, type, params, size); - break; + status_t err = NO_MEMORY; + void *params = calloc(size, 1); + if (params) { + err = data.read(params, size); + if (err != OK) { + android_errorWriteLog(0x534e4554, "26914474"); + } else { + switch (code) { + case GET_PARAMETER: + err = getParameter(node, index, params, size); + break; + case SET_PARAMETER: + err = setParameter(node, index, params, size); + break; + case GET_CONFIG: + err = getConfig(node, index, params, size); + break; + case SET_CONFIG: + err = setConfig(node, index, params, size); + break; + case SET_INTERNAL_OPTION: + { + InternalOptionType type = + (InternalOptionType)data.readInt32(); + + err = setInternalOption(node, index, type, params, size); + break; + } + + default: + TRESPASS(); + } } - - default: - TRESPASS(); } reply->writeInt32(err); -- cgit v1.1 From 582c02ea5c9c8db5f993d784a0a85b275b2e59fd Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Tue, 23 Feb 2016 14:48:46 -0800 Subject: Also fix out of bounds access for normal read Previous fix accidentally only fixed the fragmented read case. Bug: 27208621 Change-Id: Ie16f1920b84c8aba613842659238fcd5925694ad --- media/libstagefright/MPEG4Extractor.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/media/libstagefright/MPEG4Extractor.cpp b/media/libstagefright/MPEG4Extractor.cpp index e4f8384..f8789da 100755 --- a/media/libstagefright/MPEG4Extractor.cpp +++ b/media/libstagefright/MPEG4Extractor.cpp @@ -4228,7 +4228,15 @@ status_t MPEG4Source::read( continue; } - CHECK(dstOffset + 4 <= mBuffer->size()); + if (dstOffset > SIZE_MAX - 4 || + dstOffset + 4 > SIZE_MAX - nalLength || + dstOffset + 4 + nalLength > mBuffer->size()) { + ALOGE("b/27208621 : %zu %zu", dstOffset, mBuffer->size()); + android_errorWriteLog(0x534e4554, "27208621"); + mBuffer->release(); + mBuffer = NULL; + return ERROR_MALFORMED; + } dstData[dstOffset++] = 0; dstData[dstOffset++] = 0; -- cgit v1.1 From de2430fd7855b7941250e1c3744756ec9e699bd1 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Thu, 18 Feb 2016 08:25:47 -0800 Subject: Get service by value instead of reference to prevent a cleared service binder from being used. Bug: 26040840 Change-Id: Ifb5483c55b172d3553deb80dbe27f2204b86ecdb --- include/media/mediametadataretriever.h | 2 +- media/libmedia/mediametadataretriever.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/media/mediametadataretriever.h b/include/media/mediametadataretriever.h index f655f35..8ed07ee 100644 --- a/include/media/mediametadataretriever.h +++ b/include/media/mediametadataretriever.h @@ -82,7 +82,7 @@ public: const char* extractMetadata(int keyCode); private: - static const sp& getService(); + static const sp getService(); class DeathNotifier: public IBinder::DeathRecipient { diff --git a/media/libmedia/mediametadataretriever.cpp b/media/libmedia/mediametadataretriever.cpp index 9a76f58..08a9e6a 100644 --- a/media/libmedia/mediametadataretriever.cpp +++ b/media/libmedia/mediametadataretriever.cpp @@ -35,7 +35,7 @@ Mutex MediaMetadataRetriever::sServiceLock; sp MediaMetadataRetriever::sService; sp MediaMetadataRetriever::sDeathNotifier; -const sp& MediaMetadataRetriever::getService() +const sp MediaMetadataRetriever::getService() { Mutex::Autolock lock(sServiceLock); if (sService == 0) { @@ -62,7 +62,7 @@ const sp& MediaMetadataRetriever::getService() MediaMetadataRetriever::MediaMetadataRetriever() { ALOGV("constructor"); - const sp& service(getService()); + const sp service(getService()); if (service == 0) { ALOGE("failed to obtain MediaMetadataRetrieverService"); return; -- cgit v1.1