From 8dea6a22058328109dc1fcb7450ca5553f35b4df Mon Sep 17 00:00:00 2001 From: Ray Essick Date: Wed, 2 Nov 2016 14:17:57 -0700 Subject: DO NOT MERGE: defensive parsing of mp3 album art information several points in stagefrights mp3 album art code used strlen() to parse user-supplied strings that may be unterminated, resulting in reading beyond the end of a buffer. This changes the code to use strnlen() for 8-bit encodings and strengthens the parsing of 16-bit encodings similarly. It also reworks how we watch for the end-of-buffer to avoid all over-reads. Bug: 32377688 Test: crafted mp3's w/ good/bad cover art. See what showed in play music Change-Id: Ia9f526d71b21ef6a61acacf616b573753cd21df6 (cherry picked from commit fa0806b594e98f1aed3ebcfc6a801b4c0056f9eb) (cherry picked from commit 7a3246b870ddd11861eda2ab458b11d723c7f62c) --- media/libstagefright/id3/ID3.cpp | 56 ++++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 17 deletions(-) (limited to 'media/libstagefright') diff --git a/media/libstagefright/id3/ID3.cpp b/media/libstagefright/id3/ID3.cpp index d1fd0d9..8944d83 100644 --- a/media/libstagefright/id3/ID3.cpp +++ b/media/libstagefright/id3/ID3.cpp @@ -837,20 +837,21 @@ void ID3::Iterator::findFrame() { } } -static size_t StringSize(const uint8_t *start, uint8_t encoding) { +// return includes terminator; if unterminated, returns > limit +static size_t StringSize(const uint8_t *start, size_t limit, uint8_t encoding) { + if (encoding == 0x00 || encoding == 0x03) { // ISO 8859-1 or UTF-8 - return strlen((const char *)start) + 1; + return strnlen((const char *)start, limit) + 1; } // UCS-2 size_t n = 0; - while (start[n] != '\0' || start[n + 1] != '\0') { + while ((n+1 < limit) && (start[n] != '\0' || start[n + 1] != '\0')) { n += 2; } - - // Add size of null termination. - return n + 2; + n += 2; + return n; } const void * @@ -871,11 +872,19 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { if (mVersion == ID3_V2_3 || mVersion == ID3_V2_4) { uint8_t encoding = data[0]; - mime->setTo((const char *)&data[1]); - size_t mimeLen = strlen((const char *)&data[1]) + 1; + size_t consumed = 1; + + // *always* in an 8-bit encoding + size_t mimeLen = StringSize(&data[consumed], size - consumed, 0x00); + if (mimeLen > size - consumed) { + ALOGW("bogus album art size: mime"); + return NULL; + } + mime->setTo((const char *)&data[consumed]); + consumed += mimeLen; #if 0 - uint8_t picType = data[1 + mimeLen]; + uint8_t picType = data[consumed]; if (picType != 0x03) { // Front Cover Art it.next(); @@ -883,20 +892,30 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { } #endif - size_t descLen = StringSize(&data[2 + mimeLen], encoding); + consumed++; + if (consumed >= size) { + ALOGW("bogus album art size: pic type"); + return NULL; + } + + size_t descLen = StringSize(&data[consumed], size - consumed, encoding); + consumed += descLen; - if (size < 2 || - size - 2 < mimeLen || - size - 2 - mimeLen < descLen) { - ALOGW("bogus album art sizes"); + if (consumed >= size) { + ALOGW("bogus album art size: description"); return NULL; } - *length = size - 2 - mimeLen - descLen; - return &data[2 + mimeLen + descLen]; + *length = size - consumed; + + return &data[consumed]; } else { uint8_t encoding = data[0]; + if (size <= 5) { + return NULL; + } + if (!memcmp(&data[1], "PNG", 3)) { mime->setTo("image/png"); } else if (!memcmp(&data[1], "JPG", 3)) { @@ -916,7 +935,10 @@ ID3::getAlbumArt(size_t *length, String8 *mime) const { } #endif - size_t descLen = StringSize(&data[5], encoding); + size_t descLen = StringSize(&data[5], size - 5, encoding); + if (descLen > size - 5) { + return NULL; + } *length = size - 5 - descLen; -- cgit v1.1 From f5abeb809738e8b1f094d0601e882eab786d18de Mon Sep 17 00:00:00 2001 From: Marco Nelissen Date: Fri, 11 Nov 2016 09:20:00 -0800 Subject: Make VBRISeeker more robust Bug: 32577290 Change-Id: I9bcc9422ae7dd3ae4a38df330c9dcd7ac4941ec8 (cherry picked from commit 7fdd36418e945cf6a500018632dfb0ed8cb1a343) (cherry picked from commit 453b351ac5bd2b6619925dc966da60adf6b3126c) --- media/libstagefright/VBRISeeker.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) (limited to 'media/libstagefright') diff --git a/media/libstagefright/VBRISeeker.cpp b/media/libstagefright/VBRISeeker.cpp index 8a0fcac..5067ddc 100644 --- a/media/libstagefright/VBRISeeker.cpp +++ b/media/libstagefright/VBRISeeker.cpp @@ -83,8 +83,23 @@ sp VBRISeeker::CreateFromSource( scale, entrySize); + if (entrySize > 4) { + ALOGE("invalid VBRI entry size: %zu", entrySize); + return NULL; + } + + sp seeker = new (std::nothrow) VBRISeeker; + if (seeker == NULL) { + ALOGW("Couldn't allocate VBRISeeker"); + return NULL; + } + size_t totalEntrySize = numEntries * entrySize; - uint8_t *buffer = new uint8_t[totalEntrySize]; + uint8_t *buffer = new (std::nothrow) uint8_t[totalEntrySize]; + if (!buffer) { + ALOGW("Couldn't allocate %zu bytes", totalEntrySize); + return NULL; + } n = source->readAt(pos + sizeof(vbriHeader), buffer, totalEntrySize); if (n < (ssize_t)totalEntrySize) { @@ -94,7 +109,6 @@ sp VBRISeeker::CreateFromSource( return NULL; } - sp seeker = new VBRISeeker; seeker->mBasePos = post_id3_pos + frameSize; // only update mDurationUs if the calculated duration is valid (non zero) // otherwise, leave duration at -1 so that getDuration() and getOffsetForTime() -- cgit v1.1 From ffa026e98f239bbb17b7978cf8f10b7977ab563e Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Wed, 26 Oct 2016 17:41:56 -0700 Subject: stagefright: remove allottedSize equality check in IOMX::useBuffer This was meant for buffers shared cross-process, but we are not gaining anything from this check even if it was at the correct place. Bug: 32436178 Change-Id: I6919e8ac6e35092273e171f49f6711ba577ba2e6 (cherry picked from commit 58388aa7be1c6963eb4b8464d46938ba9b0a04b0) --- media/libstagefright/omx/OMXNodeInstance.cpp | 7 ------- 1 file changed, 7 deletions(-) (limited to 'media/libstagefright') diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index c09064f..afb7382 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -784,13 +784,6 @@ status_t OMXNodeInstance::useBuffer( } memset(data, 0, allottedSize); - // if we are not connecting the buffers, the sizes must match - if (allottedSize != params->size()) { - CLOG_ERROR(useBuffer, BAD_VALUE, SIMPLE_BUFFER(portIndex, (size_t)allottedSize, data)); - delete[] data; - return BAD_VALUE; - } - buffer_meta = new BufferMeta( params, portIndex, false /* copyToOmx */, false /* copyFromOmx */, data); } else { -- cgit v1.1 From ad990eb12c7aff3c4bcdd50cae90b2b7c20041e6 Mon Sep 17 00:00:00 2001 From: Lajos Molnar Date: Wed, 2 Nov 2016 14:59:03 -0700 Subject: IOMX: convert ANWB to Gralloc meta if using useBuffer in the same process This was disabled by a previous commit. Bug: 32436178 Change-Id: I9f9c6a372a039226d61f3651be3af207fed63e60 (cherry picked from commit 4fb1e42a16e77d7abf1d84bedbc20f901af26524) --- media/libstagefright/omx/OMXNodeInstance.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'media/libstagefright') diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp index afb7382..0c30e44 100644 --- a/media/libstagefright/omx/OMXNodeInstance.cpp +++ b/media/libstagefright/omx/OMXNodeInstance.cpp @@ -170,8 +170,10 @@ struct BufferMeta { return buf; } - bool copyToOmx() const { - return mCopyToOmx; + bool copyingOrSharingToOmx(const OMX_BUFFERHEADERTYPE *header) const { + return mCopyToOmx + // sharing buffer with client + || (mMem != NULL && mMem->pointer() == header->pBuffer); } void setGraphicBuffer(const sp &graphicBuffer) { @@ -1276,7 +1278,7 @@ status_t OMXNodeInstance::emptyBuffer( // convert incoming ANW meta buffers if component is configured for gralloc metadata mode // ignore rangeOffset in this case - if (buffer_meta->copyToOmx() + if (buffer_meta->copyingOrSharingToOmx(header) && mMetadataType[kPortIndexInput] == kMetadataBufferTypeGrallocSource && backup->capacity() >= sizeof(VideoNativeMetadata) && codec->capacity() >= sizeof(VideoGrallocMetadata) -- cgit v1.1