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: Idbaf221fa2283b33e83f399562a3323dd095cc2c

diff --git a/media/libstagefright/id3/ID3.cpp b/media/libstagefright/id3/ID3.cpp
index 3592fee..92174eb 100644 (file)
--- a/media/libstagefright/id3/ID3.cpp
+++ b/media/libstagefright/id3/ID3.cpp
@@ -822,20 +822,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 *
@@ -853,11 +854,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;
 
-            uint8_t picType = data[1 + mimeLen];
 #if 0
+            uint8_t picType = data[consumed];
             if (picType != 0x03) {
                 // Front Cover Art
                 it.next();
@@ -865,20 +874,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)) {
@@ -898,7 +917,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;