From fb511fc3d0149394434ff6ad9f1539ff2946e5e5 Mon Sep 17 00:00:00 2001 From: Wonsik Kim Date: Fri, 10 Feb 2017 14:29:40 +0900 Subject: [PATCH 1/6] codecs: handle onReset() for a few encoders Test: Run PoC binaries Bug: 34749392 Bug: 34705519 AOSP-Change-Id: I3356eb615b0e79272d71d72578d363671038c6dd CVE-2017-0595, CVE-2017-0596 Change-Id: I431e810876891f29bbef5b8b84c25bce8c5eeae0 --- .../codecs/aacenc/SoftAACEncoder.cpp | 14 ++++++-- .../codecs/aacenc/SoftAACEncoder.h | 2 ++ .../codecs/aacenc/SoftAACEncoder2.cpp | 14 ++++++-- .../codecs/aacenc/SoftAACEncoder2.h | 2 ++ .../codecs/m4v_h263/enc/SoftMPEG4Encoder.cpp | 35 +++++++++++-------- .../codecs/m4v_h263/enc/SoftMPEG4Encoder.h | 2 ++ .../codecs/on2/enc/SoftVPXEncoder.cpp | 9 ++++- .../codecs/on2/enc/SoftVPXEncoder.h | 2 ++ 8 files changed, 61 insertions(+), 19 deletions(-) diff --git a/media/libstagefright/codecs/aacenc/SoftAACEncoder.cpp b/media/libstagefright/codecs/aacenc/SoftAACEncoder.cpp index ab0a2285fc..96bbb850fb 100644 --- a/media/libstagefright/codecs/aacenc/SoftAACEncoder.cpp +++ b/media/libstagefright/codecs/aacenc/SoftAACEncoder.cpp @@ -62,8 +62,7 @@ SoftAACEncoder::SoftAACEncoder( } SoftAACEncoder::~SoftAACEncoder() { - delete[] mInputFrame; - mInputFrame = NULL; + onReset(); if (mEncoderHandle) { CHECK_EQ(VO_ERR_NONE, mApiHandle->Uninit(mEncoderHandle)); @@ -579,6 +578,17 @@ void SoftAACEncoder::onQueueFilled(OMX_U32 portIndex) { } } +void SoftAACEncoder::onReset() { + delete[] mInputFrame; + mInputFrame = NULL; + mInputSize = 0; + + mSentCodecSpecificData = false; + mInputTimeUs = -1ll; + mSawInputEOS = false; + mSignalledError = false; +} + } // namespace android android::SoftOMXComponent *createSoftOMXComponent( diff --git a/media/libstagefright/codecs/aacenc/SoftAACEncoder.h b/media/libstagefright/codecs/aacenc/SoftAACEncoder.h index d148eb7630..981cbbb18e 100644 --- a/media/libstagefright/codecs/aacenc/SoftAACEncoder.h +++ b/media/libstagefright/codecs/aacenc/SoftAACEncoder.h @@ -43,6 +43,8 @@ struct SoftAACEncoder : public SimpleSoftOMXComponent { virtual void onQueueFilled(OMX_U32 portIndex); + virtual void onReset(); + private: enum { kNumBuffers = 4, diff --git a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp index e8dabed69c..a89b43aa06 100644 --- a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp +++ b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp @@ -62,8 +62,7 @@ SoftAACEncoder2::SoftAACEncoder2( SoftAACEncoder2::~SoftAACEncoder2() { aacEncClose(&mAACEncoder); - delete[] mInputFrame; - mInputFrame = NULL; + onReset(); } void SoftAACEncoder2::initPorts() { @@ -670,6 +669,17 @@ void SoftAACEncoder2::onQueueFilled(OMX_U32 /* portIndex */) { } } +void SoftAACEncoder2::onReset() { + delete[] mInputFrame; + mInputFrame = NULL; + mInputSize = 0; + + mSentCodecSpecificData = false; + mInputTimeUs = -1ll; + mSawInputEOS = false; + mSignalledError = false; +} + } // namespace android android::SoftOMXComponent *createSoftOMXComponent( diff --git a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.h b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.h index bce9c244e4..f1b81e18f6 100644 --- a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.h +++ b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.h @@ -42,6 +42,8 @@ struct SoftAACEncoder2 : public SimpleSoftOMXComponent { virtual void onQueueFilled(OMX_U32 portIndex); + virtual void onReset(); + private: enum { kNumBuffers = 4, diff --git a/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.cpp b/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.cpp index 2eb51c998b..800238685e 100644 --- a/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.cpp +++ b/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.cpp @@ -99,6 +99,7 @@ SoftMPEG4Encoder::SoftMPEG4Encoder( SoftMPEG4Encoder::~SoftMPEG4Encoder() { ALOGV("Destruct SoftMPEG4Encoder"); + onReset(); releaseEncoder(); List &outQueue = getPortQueue(1); List &inQueue = getPortQueue(0); @@ -208,22 +209,15 @@ OMX_ERRORTYPE SoftMPEG4Encoder::initEncoder() { } OMX_ERRORTYPE SoftMPEG4Encoder::releaseEncoder() { - if (!mStarted) { - return OMX_ErrorNone; + if (mEncParams) { + delete mEncParams; + mEncParams = NULL; } - PVCleanUpVideoEncoder(mHandle); - - free(mInputFrameData); - mInputFrameData = NULL; - - delete mEncParams; - mEncParams = NULL; - - delete mHandle; - mHandle = NULL; - - mStarted = false; + if (mHandle) { + delete mHandle; + mHandle = NULL; + } return OMX_ErrorNone; } @@ -519,6 +513,19 @@ void SoftMPEG4Encoder::onQueueFilled(OMX_U32 /* portIndex */) { } } +void SoftMPEG4Encoder::onReset() { + if (!mStarted) { + return; + } + + PVCleanUpVideoEncoder(mHandle); + + free(mInputFrameData); + mInputFrameData = NULL; + + mStarted = false; +} + } // namespace android android::SoftOMXComponent *createSoftOMXComponent( diff --git a/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.h b/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.h index 3389c373ca..422fe221b0 100644 --- a/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.h +++ b/media/libstagefright/codecs/m4v_h263/enc/SoftMPEG4Encoder.h @@ -48,6 +48,8 @@ struct SoftMPEG4Encoder : public SoftVideoEncoderOMXComponent { virtual void onQueueFilled(OMX_U32 portIndex); + virtual void onReset(); + protected: virtual ~SoftMPEG4Encoder(); diff --git a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp index 5c950c7f55..26c0fca559 100644 --- a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp +++ b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.cpp @@ -788,7 +788,9 @@ void SoftVPXEncoder::onQueueFilled(OMX_U32 /* portIndex */) { if (inputBufferHeader->nTimeStamp > mLastTimestamp) { frameDuration = (uint32_t)(inputBufferHeader->nTimeStamp - mLastTimestamp); } else { - frameDuration = (uint32_t)(((uint64_t)1000000 << 16) / mFramerate); + // Use default of 30 fps in case of 0 frame rate. + uint32_t framerate = mFramerate ?: (30 << 16); + frameDuration = (uint32_t)(((uint64_t)1000000 << 16) / framerate); } mLastTimestamp = inputBufferHeader->nTimeStamp; codec_return = vpx_codec_encode( @@ -842,6 +844,11 @@ void SoftVPXEncoder::onQueueFilled(OMX_U32 /* portIndex */) { } } +void SoftVPXEncoder::onReset() { + releaseEncoder(); + mLastTimestamp = 0x7FFFFFFFFFFFFFFFLL; +} + } // namespace android diff --git a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.h b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.h index cd0a0cf518..2033e6441e 100644 --- a/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.h +++ b/media/libstagefright/codecs/on2/enc/SoftVPXEncoder.h @@ -87,6 +87,8 @@ struct SoftVPXEncoder : public SoftVideoEncoderOMXComponent { // encoding of the frame virtual void onQueueFilled(OMX_U32 portIndex); + virtual void onReset(); + private: enum TemporalReferences { // For 1 layer case: reference all (last, golden, and alt ref), but only From 2f8cc602a6e11f77eff79d6dda68cdb1dea5660b Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Thu, 9 Mar 2017 15:01:55 -0800 Subject: [PATCH 2/6] Fix integer overflow and divide-by-zero Bug: 35763994 Test: ran CTS with and without fix AOSP-Change-Id: If835e97ce578d4fa567e33e349e48fb7b2559e0e (cherry picked from commit 8538a603ef992e75f29336499cb783f3ec19f18c) CVE-2017-0603 Change-Id: I043e7c709915decc693879adafe5337189021131 --- media/libstagefright/AMRExtractor.cpp | 2 +- media/libstagefright/NuMediaExtractor.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/media/libstagefright/AMRExtractor.cpp b/media/libstagefright/AMRExtractor.cpp index a6fb3d8dda..9c0740397d 100644 --- a/media/libstagefright/AMRExtractor.cpp +++ b/media/libstagefright/AMRExtractor.cpp @@ -253,7 +253,7 @@ status_t AMRSource::read( int64_t seekTimeUs; ReadOptions::SeekMode mode; - if (options && options->getSeekTo(&seekTimeUs, &mode)) { + if (mOffsetTableLength > 0 && options && options->getSeekTo(&seekTimeUs, &mode)) { size_t size; int64_t seekFrame = seekTimeUs / 20000ll; // 20ms per frame. mCurrentTimeUs = seekFrame * 20000ll; diff --git a/media/libstagefright/NuMediaExtractor.cpp b/media/libstagefright/NuMediaExtractor.cpp index f24cf3ad17..6de2bb3a4d 100644 --- a/media/libstagefright/NuMediaExtractor.cpp +++ b/media/libstagefright/NuMediaExtractor.cpp @@ -542,7 +542,7 @@ bool NuMediaExtractor::getTotalBitrate(int64_t *bitrate) const { } off64_t size; - if (mDurationUs >= 0 && mDataSource->getSize(&size) == OK) { + if (mDurationUs > 0 && mDataSource->getSize(&size) == OK) { *bitrate = size * 8000000ll / mDurationUs; // in bits/sec return true; } From abf8512afa2f61798d0fa6ab4386e8a00618d743 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Fri, 10 Mar 2017 11:28:44 -0800 Subject: [PATCH 3/6] Fix out of bounds access Bug: 34618607 AOSP-Change-Id: I84f0ef948414d0b2d54e8948b6c30b8ae4da2b36 (cherry picked from commit d1c19c57f66d91ea8033c8fa6510a8760a6e663b) CVE-2017-0588 Change-Id: I11dee84a2bf7da9d794f94696af56a78967faea6 --- media/libstagefright/id3/ID3.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/media/libstagefright/id3/ID3.cpp b/media/libstagefright/id3/ID3.cpp index 8944d83fb8..fee13eac66 100644 --- a/media/libstagefright/id3/ID3.cpp +++ b/media/libstagefright/id3/ID3.cpp @@ -379,7 +379,7 @@ bool ID3::removeUnsynchronizationV2_4(bool iTunesHack) { flags &= ~1; } - if (flags & 2) { + if ((flags & 2) && (dataSize >= 2)) { // This file has "unsynchronization", so we have to replace occurrences // of 0xff 0x00 with just 0xff in order to get the real data. @@ -395,11 +395,15 @@ bool ID3::removeUnsynchronizationV2_4(bool iTunesHack) { mData[writeOffset++] = mData[readOffset++]; } // move the remaining data following this frame - memmove(&mData[writeOffset], &mData[readOffset], oldSize - readOffset); + if (readOffset <= oldSize) { + memmove(&mData[writeOffset], &mData[readOffset], oldSize - readOffset); + } else { + ALOGE("b/34618607 (%zu %zu %zu %zu)", readOffset, writeOffset, oldSize, mSize); + android_errorWriteLog(0x534e4554, "34618607"); + } - flags &= ~2; } - + flags &= ~2; if (flags != prevFlags || iTunesHack) { WriteSyncsafeInteger(&mData[offset + 4], dataSize); mData[offset + 8] = flags >> 8; From 72816ee6c055c152222e6b67ffafbf3613d32e66 Mon Sep 17 00:00:00 2001 From: Andy Hung Date: Mon, 13 Feb 2017 18:48:39 -0800 Subject: [PATCH 4/6] AudioFlinger: Check framecount overflow when creating track Test: Native POC Bug: 34749571 AOSP-Change-Id: I7529658e52ac7e64d162eb5338f10fb25eaa8fe7 (cherry picked from commit 1883f69de5f2c4e71df58d5b71d7c39f9779b50c) (cherry picked from commit eaa3969f757291f151efedde17ec529b8659024d) CVE-2017-0597 Change-Id: I2a9338daba9a6aae882daa176cf4c8445de316e6 --- services/audioflinger/Tracks.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp index f3b537526b..77a929a05c 100644 --- a/services/audioflinger/Tracks.cpp +++ b/services/audioflinger/Tracks.cpp @@ -114,9 +114,24 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase( mUid = clientUid; // ALOGD("Creating track with %d buffers @ %d bytes", bufferCount, bufferSize); + + size_t bufferSize = buffer == NULL ? roundup(frameCount) : frameCount; + // check overflow when computing bufferSize due to multiplication by mFrameSize. + if (bufferSize < frameCount // roundup rounds down for values above UINT_MAX / 2 + || mFrameSize == 0 // format needs to be correct + || bufferSize > SIZE_MAX / mFrameSize) { + android_errorWriteLog(0x534e4554, "34749571"); + return; + } + bufferSize *= mFrameSize; + size_t size = sizeof(audio_track_cblk_t); - size_t bufferSize = (buffer == NULL ? roundup(frameCount) : frameCount) * mFrameSize; if (buffer == NULL && alloc == ALLOC_CBLK) { + // check overflow when computing allocation size for streaming tracks. + if (size > SIZE_MAX - bufferSize) { + android_errorWriteLog(0x534e4554, "34749571"); + return; + } size += bufferSize; } From 3681cdb84d78f2e81a04ea7d4c1ae3a3df8b0de6 Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Fri, 3 Mar 2017 13:37:27 -0800 Subject: [PATCH 5/6] Fix NPDs in h263 decoder Bug: 35269635 Test: decoded PoC with and without patch AOSP-Change-Id: I636a14360c7801cc5bca63c9cb44d1d235df8fd8 (cherry picked from commit 2ad2a92318a3b9daf78ebcdc597085adbf32600d) CVE-2017-0600 Change-Id: Iff929daa479816bb2c0363705d14ad2ee5e11a13 --- .../codecs/m4v_h263/dec/src/mb_motion_comp.cpp | 18 +++++++++++++++++- .../codecs/m4v_h263/dec/src/pvdec_api.cpp | 7 +++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/media/libstagefright/codecs/m4v_h263/dec/src/mb_motion_comp.cpp b/media/libstagefright/codecs/m4v_h263/dec/src/mb_motion_comp.cpp index fbc7be1aa1..877723d758 100644 --- a/media/libstagefright/codecs/m4v_h263/dec/src/mb_motion_comp.cpp +++ b/media/libstagefright/codecs/m4v_h263/dec/src/mb_motion_comp.cpp @@ -15,6 +15,10 @@ * and limitations under the License. * ------------------------------------------------------------------- */ + +#define LOG_TAG "m4v_h263" +#include + /* ------------------------------------------------------------------------------ INPUT AND OUTPUT DEFINITIONS @@ -236,6 +240,11 @@ void MBMotionComp( /* Pointer to previous luminance frame */ c_prev = prev->yChan; + if (!c_prev) { + ALOGE("b/35269635"); + android_errorWriteLog(0x534e4554, "35269635"); + return; + } pred_block = video->mblock->pred_block; @@ -574,7 +583,14 @@ void SkippedMBMotionComp( /* zero motion compensation for previous frame */ /*mby*width + mbx;*/ - c_prev = prev->yChan + offset; + c_prev = prev->yChan; + if (!c_prev) { + ALOGE("b/35269635"); + android_errorWriteLog(0x534e4554, "35269635"); + return; + } + c_prev += offset; + /*by*width_uv + bx;*/ cu_prev = prev->uChan + (offset >> 2) + (xpos >> 2); /*by*width_uv + bx;*/ diff --git a/media/libstagefright/codecs/m4v_h263/dec/src/pvdec_api.cpp b/media/libstagefright/codecs/m4v_h263/dec/src/pvdec_api.cpp index c1720c6390..8d5d0712b3 100644 --- a/media/libstagefright/codecs/m4v_h263/dec/src/pvdec_api.cpp +++ b/media/libstagefright/codecs/m4v_h263/dec/src/pvdec_api.cpp @@ -15,6 +15,8 @@ * and limitations under the License. * ------------------------------------------------------------------- */ +#define LOG_TAG "pvdec_api" +#include #include "mp4dec_lib.h" #include "vlc_decode.h" #include "bitstream.h" @@ -1335,6 +1337,11 @@ Bool PVDecodeVopBody(VideoDecControls *decCtrl, int32 buffer_size[]) } } + if (!video->prevVop->yChan) { + ALOGE("b/35269635"); + android_errorWriteLog(0x534e4554, "35269635"); + return PV_FALSE; + } oscl_memcpy(currVop->yChan, video->prevVop->yChan, (decCtrl->size*3) / 2); video->prevVop = prevVop; From 54b384f4898e24967da8c86898aa5ab45e336bfb Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Mon, 13 Mar 2017 11:59:57 -0700 Subject: [PATCH 6/6] Add bounds check in SoftAACEncoder2::onQueueFilled() Original code blindly copied some header information into the user-supplied buffer without checking for sufficient space. The code does check when it gets to filling the data -- it's just the header copies that weren't checked. Bug: 34617444 Test: ran POC before/after AOSP-Change-Id: I6e80ec90616f6cd02bb8316cd2d6e309b7e4729d (cherry picked from commit 6231243626b8b9c57593b1f0ee417f2c4af4c0aa) CVE-2017-0594 Change-Id: Ie431b848ffa24700e3e4d84c0e98af99bec9ae5e --- media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp index a89b43aa06..63096f117e 100644 --- a/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp +++ b/media/libstagefright/codecs/aacenc/SoftAACEncoder2.cpp @@ -477,6 +477,15 @@ void SoftAACEncoder2::onQueueFilled(OMX_U32 /* portIndex */) { BufferInfo *outInfo = *outQueue.begin(); OMX_BUFFERHEADERTYPE *outHeader = outInfo->mHeader; + + if (outHeader->nOffset + encInfo.confSize > outHeader->nAllocLen) { + ALOGE("b/34617444"); + android_errorWriteLog(0x534e4554,"34617444"); + notify(OMX_EventError, OMX_ErrorUndefined, 0, NULL); + mSignalledError = true; + return; + } + outHeader->nFilledLen = encInfo.confSize; outHeader->nFlags = OMX_BUFFERFLAG_CODECCONFIG;