From 28314aef9e8a666dbb75bbd555f6566a6c991f1c Mon Sep 17 00:00:00 2001 From: Wonsik Kim Date: Fri, 11 Sep 2015 06:51:10 +0000 Subject: Revert "Avoid size_t overflow in base64 decoding once again" This reverts commit c9ac5dfdafed1c66beae090cafa97002764e0ca3. Change-Id: Iae9707bbd8641a0bb00fcda39a20eb8b8f4f5232 --- media/libstagefright/OggExtractor.cpp | 102 +++++++++++++++++++++++++---- media/libstagefright/foundation/base64.cpp | 11 +--- 2 files changed, 93 insertions(+), 20 deletions(-) (limited to 'media') diff --git a/media/libstagefright/OggExtractor.cpp b/media/libstagefright/OggExtractor.cpp index 10dce3b..130f5a5 100644 --- a/media/libstagefright/OggExtractor.cpp +++ b/media/libstagefright/OggExtractor.cpp @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -831,18 +830,93 @@ void parseVorbisComment( } +// The returned buffer should be free()d. +static uint8_t *DecodeBase64(const char *s, size_t size, size_t *outSize) { + *outSize = 0; + + if ((size % 4) != 0) { + return NULL; + } + + size_t n = size; + size_t padding = 0; + if (n >= 1 && s[n - 1] == '=') { + padding = 1; + + if (n >= 2 && s[n - 2] == '=') { + padding = 2; + } + } + + // We divide first to avoid overflow. It's OK to do this because we + // already made sure that size % 4 == 0. + size_t outLen = (size / 4) * 3 - padding; + + void *buffer = malloc(outLen); + if (buffer == NULL) { + return NULL; + } + + uint8_t *out = (uint8_t *)buffer; + size_t j = 0; + uint32_t accum = 0; + for (size_t i = 0; i < n; ++i) { + char c = s[i]; + unsigned value; + if (c >= 'A' && c <= 'Z') { + value = c - 'A'; + } else if (c >= 'a' && c <= 'z') { + value = 26 + c - 'a'; + } else if (c >= '0' && c <= '9') { + value = 52 + c - '0'; + } else if (c == '+') { + value = 62; + } else if (c == '/') { + value = 63; + } else if (c != '=') { + break; + } else { + if (i < n - padding) { + break; + } + + value = 0; + } + + accum = (accum << 6) | value; + + if (((i + 1) % 4) == 0) { + out[j++] = (accum >> 16); + + if (j < outLen) { out[j++] = (accum >> 8) & 0xff; } + if (j < outLen) { out[j++] = accum & 0xff; } + + accum = 0; + } + } + + // Check if we exited the loop early. + if (j < outLen) { + free(buffer); + return NULL; + } + + *outSize = outLen; + return (uint8_t *)buffer; +} + static void extractAlbumArt( const sp &fileMeta, const void *data, size_t size) { ALOGV("extractAlbumArt from '%s'", (const char *)data); - sp flacBuffer = decodeBase64(AString((const char *)data, size)); - if (flacBuffer == NULL) { + size_t flacSize; + uint8_t *flac = DecodeBase64((const char *)data, size, &flacSize); + + if (flac == NULL) { ALOGE("malformed base64 encoded data."); return; } - size_t flacSize = flacBuffer->size(); - uint8_t *flac = flacBuffer->data(); ALOGV("got flac of size %zu", flacSize); uint32_t picType; @@ -852,24 +926,24 @@ static void extractAlbumArt( char type[128]; if (flacSize < 8) { - return; + goto exit; } picType = U32_AT(flac); if (picType != 3) { // This is not a front cover. - return; + goto exit; } typeLen = U32_AT(&flac[4]); if (typeLen > sizeof(type) - 1) { - return; + goto exit; } // we've already checked above that flacSize >= 8 if (flacSize - 8 < typeLen) { - return; + goto exit; } memcpy(type, &flac[8], typeLen); @@ -879,7 +953,7 @@ static void extractAlbumArt( if (!strcmp(type, "-->")) { // This is not inline cover art, but an external url instead. - return; + goto exit; } descLen = U32_AT(&flac[8 + typeLen]); @@ -887,7 +961,7 @@ static void extractAlbumArt( if (flacSize < 32 || flacSize - 32 < typeLen || flacSize - 32 - typeLen < descLen) { - return; + goto exit; } dataLen = U32_AT(&flac[8 + typeLen + 4 + descLen + 16]); @@ -895,7 +969,7 @@ static void extractAlbumArt( // we've already checked above that (flacSize - 32 - typeLen - descLen) >= 0 if (flacSize - 32 - typeLen - descLen < dataLen) { - return; + goto exit; } ALOGV("got image data, %zu trailing bytes", @@ -905,6 +979,10 @@ static void extractAlbumArt( kKeyAlbumArt, 0, &flac[8 + typeLen + 4 + descLen + 20], dataLen); fileMeta->setCString(kKeyAlbumArtMIME, type); + +exit: + free(flac); + flac = NULL; } //////////////////////////////////////////////////////////////////////////////// diff --git a/media/libstagefright/foundation/base64.cpp b/media/libstagefright/foundation/base64.cpp index 7da7db9..dcf5bef 100644 --- a/media/libstagefright/foundation/base64.cpp +++ b/media/libstagefright/foundation/base64.cpp @@ -22,11 +22,11 @@ namespace android { sp decodeBase64(const AString &s) { - size_t n = s.size(); - if ((n % 4) != 0) { + if ((s.size() % 4) != 0) { return NULL; } + size_t n = s.size(); size_t padding = 0; if (n >= 1 && s.c_str()[n - 1] == '=') { padding = 1; @@ -40,16 +40,11 @@ sp decodeBase64(const AString &s) { } } - // We divide first to avoid overflow. It's OK to do this because we - // already made sure that n % 4 == 0. - size_t outLen = (n / 4) * 3 - padding; + size_t outLen = 3 * s.size() / 4 - padding; sp buffer = new ABuffer(outLen); uint8_t *out = buffer->data(); - if (out == NULL || buffer->size() < outLen) { - return NULL; - } size_t j = 0; uint32_t accum = 0; for (size_t i = 0; i < n; ++i) { -- cgit v1.1