summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRay Essick <essick@google.com>2016-11-02 14:17:57 -0700
committermh0rst <mhorst@tzi.de>2017-01-13 11:50:13 +0100
commit8dea6a22058328109dc1fcb7450ca5553f35b4df (patch)
tree0b2fe97c99c02823400ed55207cf739fd10c5aa8
parent621ca73010f3954566b27c6554ce992cc6069670 (diff)
downloadframeworks_av-8dea6a22058328109dc1fcb7450ca5553f35b4df.zip
frameworks_av-8dea6a22058328109dc1fcb7450ca5553f35b4df.tar.gz
frameworks_av-8dea6a22058328109dc1fcb7450ca5553f35b4df.tar.bz2
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)
-rw-r--r--media/libstagefright/id3/ID3.cpp56
1 files changed, 39 insertions, 17 deletions
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;