Subject: fix for vulnerability CVE-2011-3343 for OpenTTD 1.0.0 - 1.0.5 (Multiple buffer overflows in validation of external data) From: OpenTTD developer team Origin: backport, http://vcs.openttd.org/svn/changeset/22871 http://vcs.openttd.org/svn/changeset/22873 http://vcs.openttd.org/svn/changeset/22874 Bug: http://bugs.openttd.org/task/4746 http://bugs.openttd.org/task/4747 In multiple places external data isn't properly checked before allocating memory, which could lead to buffer overflows and arbitrary code execution. These bugs are only exploitable locally by providing OpenTTD with invalid/manipulated images, sounds or fonts. This means an attacker either needs local access or has to trick an user into loading a manipulated image into OpenTTD. This is especially a concern with BMP files loaded as heightmaps. All except one vulnerability are caused by improper validation of input data prior to allocating memory buffers. It is possible to force allocation of a too small buffer and thus out-of-bounds writes by causing an integer overflow. Additionally in RLE-compressed BMP images, it is possible to write arbitrary data outside the allocated buffer. No patch for releases before 0.3.1 is provided, as this versions are unsupported since a long time and would require larger changes not worth the effort. diff --git src/bmp.cpp src/bmp.cpp index 64b3f14..7f571ef 100644 --- src/bmp.cpp +++ src/bmp.cpp @@ -142,6 +142,7 @@ static inline bool BmpRead4Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) switch (c) { case 0: // end of line x = 0; + if (y == 0) return false; pixel = &data->bitmap[--y * info->width]; break; case 1: // end of bitmap @@ -152,7 +153,7 @@ static inline bool BmpRead4Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) case 2: // delta x += ReadByte(buffer); i = ReadByte(buffer); - if (x >= info->width || (y == 0 && i > 0)) return false; + if (x >= info->width || i > y) return false; y -= i; pixel = &data->bitmap[y * info->width + x]; break; @@ -225,6 +226,7 @@ static inline bool BmpRead8Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) switch (c) { case 0: // end of line x = 0; + if (y == 0) return false; pixel = &data->bitmap[--y * info->width]; break; case 1: // end of bitmap @@ -235,13 +237,16 @@ static inline bool BmpRead8Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) case 2: // delta x += ReadByte(buffer); i = ReadByte(buffer); - if (x >= info->width || (y == 0 && i > 0)) return false; + if (x >= info->width || i > y) return false; y -= i; pixel = &data->bitmap[y * info->width + x]; break; default: // uncompressed - if ((x += c) > info->width) return false; - for (i = 0; i < c; i++) *pixel++ = ReadByte(buffer); + for (i = 0; i < c; i++) { + if (EndOfBuffer(buffer) || x >= info->width) return false; + *pixel++ = ReadByte(buffer); + x++; + } /* Padding for 16 bit align */ SkipBytes(buffer, c % 2); break; diff --git src/fontcache.cpp src/fontcache.cpp index 133f3e3..52af37f 100644 --- src/fontcache.cpp +++ src/fontcache.cpp @@ -996,6 +996,9 @@ static bool GetFontAAState(FontSize size) width = max(1, slot->bitmap.width + (size == FS_NORMAL)); height = max(1, slot->bitmap.rows + (size == FS_NORMAL)); + /* Limit glyph size to prevent overflows later on. */ + if (width > 256 || height > 256) usererror("Font glyph is too large"); + /* FreeType has rendered the glyph, now we allocate a sprite and copy the image into it */ sprite.AllocateData(width * height); sprite.width = width; diff --git src/heightmap.cpp src/heightmap.cpp index 68a1ecf..4ac2f60 100644 --- src/heightmap.cpp +++ src/heightmap.cpp @@ -142,13 +142,24 @@ static bool ReadHeightmapPNG(char *filename, uint *x, uint *y, byte **map) return false; } + uint width = png_get_image_width(png_ptr, info_ptr); + uint height = png_get_image_height(png_ptr, info_ptr); + + /* Check if image dimensions don't overflow a size_t to avoid memory corruption. */ + if ((uint64)width * height >= (size_t)-1) { + ShowErrorMessage(STR_ERROR_PNGMAP, STR_ERROR_HEIGHTMAP_TOO_LARGE, 0, 0); + fclose(fp); + png_destroy_read_struct(&png_ptr, &info_ptr, NULL); + return false; + } + if (map != NULL) { - *map = MallocT(png_get_image_width(png_ptr, info_ptr) * png_get_image_height(png_ptr, info_ptr)); + *map = MallocT(width * height); ReadHeightmapPNGImageData(*map, png_ptr, info_ptr); } - *x = png_get_image_width(png_ptr, info_ptr); - *y = png_get_image_height(png_ptr, info_ptr); + *x = width; + *y = height; fclose(fp); png_destroy_read_struct(&png_ptr, &info_ptr, NULL); @@ -243,6 +254,14 @@ static bool ReadHeightmapBMP(char *filename, uint *x, uint *y, byte **map) return false; } + /* Check if image dimensions don't overflow a size_t to avoid memory corruption. */ + if ((uint64)info.width * info.height >= (size_t)-1 / (info.bpp == 24 ? 3 : 1)) { + ShowErrorMessage(STR_ERROR_BMPMAP, STR_ERROR_HEIGHTMAP_TOO_LARGE, 0, 0); + fclose(f); + BmpDestroyData(&data); + return false; + } + if (map != NULL) { if (!BmpReadBitmap(&buffer, &info, &data)) { ShowErrorMessage(STR_ERROR_BMPMAP, STR_ERROR_BMPMAP_IMAGE_TYPE, 0, 0); diff --git src/lang/english.txt src/lang/english.txt index a9c4147..b025204 100644 --- src/lang/english.txt +++ src/lang/english.txt @@ -3333,6 +3333,8 @@ STR_ERROR_PNGMAP_MISC :{WHITE}... some STR_ERROR_BMPMAP :{WHITE}Can't load landscape from BMP... STR_ERROR_BMPMAP_IMAGE_TYPE :{WHITE}... could not convert image type. +STR_ERROR_HEIGHTMAP_TOO_LARGE :{WHITE}... image is too large + STR_WARNING_HEIGHTMAP_SCALE_CAPTION :{WHITE}Scale warning STR_WARNING_HEIGHTMAP_SCALE_MESSAGE :{YELLOW}Resizing source map too much is not recommended. Continue with the generation? diff --git src/openttd.cpp src/openttd.cpp index b636402..335bcc9 100644 --- src/openttd.cpp +++ src/openttd.cpp @@ -584,11 +584,12 @@ int ttd_main(int argc, char *argv[]) /* * The width and height must be at least 1 pixel and width times - * height must still fit within a 32 bits integer, this way all - * internal drawing routines work correctly. + * height times bytes per pixel must still fit within a 32 bits + * integer, even for 32 bpp video modes. This way all internal + * drawing routines work correctly. */ - _cur_resolution.width = ClampU(_cur_resolution.width, 1, UINT16_MAX); - _cur_resolution.height = ClampU(_cur_resolution.height, 1, UINT16_MAX); + _cur_resolution.width = ClampU(_cur_resolution.width, 1, UINT16_MAX / 2); + _cur_resolution.height = ClampU(_cur_resolution.height, 1, UINT16_MAX / 2); #if defined(ENABLE_NETWORK) if (dedicated_host) { diff --git src/script/squirrel_helper.hpp src/script/squirrel_helper.hpp index 67f8a21..eded9a7 100644 --- src/script/squirrel_helper.hpp +++ src/script/squirrel_helper.hpp @@ -118,6 +118,9 @@ template <> inline Array *GetParam(ForceType, HSQUIRRELVM vm, int index, SQAutoFreePointers *ptr) { + /* Sanity check of the size. */ + if (sq_getsize(vm, index) > UINT16_MAX) throw sq_throwerror(vm, _SC("an array used as parameter to a function is too large")); + SQObject obj; sq_getstackobj(vm, index, &obj); sq_pushobject(vm, obj); diff --git src/sound.cpp src/sound.cpp index e337b5e..5a87006 100644 --- src/sound.cpp +++ src/sound.cpp @@ -114,7 +114,8 @@ static bool SetBankSource(MixerChannel *mc, const SoundEntry *sound) { assert(sound != NULL); - if (sound->file_size == 0) return false; + /* Check for valid sound size. */ + if (sound->file_size == 0 || sound->file_size > ((size_t)-1) - 2) return false; int8 *mem = MallocT(sound->file_size + 2); /* Add two extra bytes so rate conversion can read these diff --git src/sound/win32_s.cpp src/sound/win32_s.cpp index c0e5da5..ef3f98f 100644 --- src/sound/win32_s.cpp +++ src/sound/win32_s.cpp @@ -63,7 +63,9 @@ static DWORD WINAPI SoundThread(LPVOID arg) wfex.nBlockAlign = (wfex.nChannels * wfex.wBitsPerSample) / 8; wfex.nAvgBytesPerSec = wfex.nSamplesPerSec * wfex.nBlockAlign; + /* Limit buffer size to prevent overflows. */ _bufsize = GetDriverParamInt(parm, "bufsize", (GB(GetVersion(), 0, 8) > 5) ? 8192 : 4096); + _bufsize = min(_bufsize, UINT16_MAX); try { if (NULL == (_event = CreateEvent(NULL, FALSE, FALSE, NULL))) throw "Failed to create event"; diff --git src/spriteloader/png.cpp src/spriteloader/png.cpp index e72f440..4378c7f 100644 --- src/spriteloader/png.cpp +++ src/spriteloader/png.cpp @@ -106,9 +106,21 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i if (strcmp("y_offs", text_ptr[i].key) == 0) sprite->y_offs = strtol(text_ptr[i].text, NULL, 0); } - sprite->height = png_get_image_height(png_ptr, info_ptr); - sprite->width = png_get_image_width(png_ptr, info_ptr); + uint height = png_get_image_height(png_ptr, info_ptr); + uint width = png_get_image_width(png_ptr, info_ptr); + /* Check if sprite dimensions aren't larger than what is allowed in GRF-files. */ + if (height > UINT8_MAX || width > UINT16_MAX) { + png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); + return false; + } + sprite->height = height; + sprite->width = width; sprite->AllocateData(sprite->width * sprite->height); + } else if (sprite->height != png_get_image_height(png_ptr, info_ptr) || sprite->width != png_get_image_width(png_ptr, info_ptr)) { + /* Make sure the mask image isn't larger than the sprite image. */ + DEBUG(misc, 0, "Ignoring mask for SpriteID %d as it isn't the same dimension as the masked sprite", id); + png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); + return true; } bit_depth = png_get_bit_depth(png_ptr, info_ptr); @@ -116,6 +128,7 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i if (mask && (bit_depth != 8 || colour_type != PNG_COLOR_TYPE_PALETTE)) { DEBUG(misc, 0, "Ignoring mask for SpriteID %d as it isn't a 8 bit palette image", id); + png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); return true; }