From c9ac5dfdafed1c66beae090cafa97002764e0ca3 Mon Sep 17 00:00:00 2001 From: Wonsik Kim Date: Mon, 7 Sep 2015 15:52:27 +0900 Subject: Avoid size_t overflow in base64 decoding once again Switch to foundation base64 function in OggExtractor and fix the issue there. Bug: 23707088 Change-Id: I999ae911177c88dc13f9ee9796ca93c5928b20b0 --- media/libstagefright/OggExtractor.cpp | 102 ++++------------------------- media/libstagefright/foundation/base64.cpp | 11 +++- 2 files changed, 20 insertions(+), 93 deletions(-) (limited to 'media') diff --git a/media/libstagefright/OggExtractor.cpp b/media/libstagefright/OggExtractor.cpp index 130f5a5..10dce3b 100644 --- a/media/libstagefright/OggExtractor.cpp +++ b/media/libstagefright/OggExtractor.cpp @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -830,93 +831,18 @@ 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); - size_t flacSize; - uint8_t *flac = DecodeBase64((const char *)data, size, &flacSize); - - if (flac == NULL) { + sp flacBuffer = decodeBase64(AString((const char *)data, size)); + if (flacBuffer == 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; @@ -926,24 +852,24 @@ static void extractAlbumArt( char type[128]; if (flacSize < 8) { - goto exit; + return; } picType = U32_AT(flac); if (picType != 3) { // This is not a front cover. - goto exit; + return; } typeLen = U32_AT(&flac[4]); if (typeLen > sizeof(type) - 1) { - goto exit; + return; } // we've already checked above that flacSize >= 8 if (flacSize - 8 < typeLen) { - goto exit; + return; } memcpy(type, &flac[8], typeLen); @@ -953,7 +879,7 @@ static void extractAlbumArt( if (!strcmp(type, "-->")) { // This is not inline cover art, but an external url instead. - goto exit; + return; } descLen = U32_AT(&flac[8 + typeLen]); @@ -961,7 +887,7 @@ static void extractAlbumArt( if (flacSize < 32 || flacSize - 32 < typeLen || flacSize - 32 - typeLen < descLen) { - goto exit; + return; } dataLen = U32_AT(&flac[8 + typeLen + 4 + descLen + 16]); @@ -969,7 +895,7 @@ static void extractAlbumArt( // we've already checked above that (flacSize - 32 - typeLen - descLen) >= 0 if (flacSize - 32 - typeLen - descLen < dataLen) { - goto exit; + return; } ALOGV("got image data, %zu trailing bytes", @@ -979,10 +905,6 @@ 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 dcf5bef..7da7db9 100644 --- a/media/libstagefright/foundation/base64.cpp +++ b/media/libstagefright/foundation/base64.cpp @@ -22,11 +22,11 @@ namespace android { sp decodeBase64(const AString &s) { - if ((s.size() % 4) != 0) { + size_t n = s.size(); + if ((n % 4) != 0) { return NULL; } - size_t n = s.size(); size_t padding = 0; if (n >= 1 && s.c_str()[n - 1] == '=') { padding = 1; @@ -40,11 +40,16 @@ sp decodeBase64(const AString &s) { } } - size_t outLen = 3 * s.size() / 4 - padding; + // 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; 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