Subject: fix for vulnerability CVE-2011-3343 for OpenTTD 0.6.0 - 0.6.3 (Multiple buffer overflows in validation of external data) From: OpenTTD developer team Origin: backport, https://github.com/OpenTTD/OpenTTD/commit/6c7cbb1 https://github.com/OpenTTD/OpenTTD/commit/65637d8 https://github.com/OpenTTD/OpenTTD/commit/73624ab Bug: https://github.com/OpenTTD/OpenTTD/issues/4746 https://github.com/OpenTTD/OpenTTD/issues/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 c5677fb..4ee1964 100644 --- src/bmp.cpp +++ src/bmp.cpp @@ -135,6 +135,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 @@ -145,7 +146,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; @@ -218,6 +219,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 @@ -228,13 +230,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 4774279..74d0024 100644 --- src/fontcache.cpp +++ src/fontcache.cpp @@ -422,6 +422,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) error("Font glyph is too large"); + /* FreeType has rendered the glyph, now we allocate a sprite and copy the image into it */ sprite.data = CallocT(width * height); sprite.width = width; diff --git src/heightmap.cpp src/heightmap.cpp index c04ebd8..e2645a5 100644 --- src/heightmap.cpp +++ src/heightmap.cpp @@ -139,6 +139,14 @@ static bool ReadHeightmapPNG(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_ptr->width * info_ptr->height >= (size_t)-1) { + ShowErrorMessage(STR_ERROR_HEIGHTMAP_TOO_LARGE, STR_PNGMAP_ERROR, 0, 0); + fclose(fp); + png_destroy_read_struct(&png_ptr, &info_ptr, NULL); + return false; + } + if (map != NULL) { *map = MallocT(info_ptr->width * info_ptr->height); @@ -248,6 +256,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_HEIGHTMAP_TOO_LARGE, STR_BMPMAP_ERROR, 0, 0); + fclose(f); + BmpDestroyData(&data); + return false; + } + if (map != NULL) { if (!BmpReadBitmap(&buffer, &info, &data)) { ShowErrorMessage(STR_BMPMAP_ERR_IMAGE_TYPE, STR_BMPMAP_ERROR, 0, 0); diff --git src/lang/english.txt src/lang/english.txt index 19b4e35..35402ae 100644 --- src/lang/english.txt +++ src/lang/english.txt @@ -1585,6 +1585,8 @@ STR_PNGMAP_ERR_MISC :{WHITE}...somet STR_BMPMAP_ERROR :{WHITE}Cannot load landscape from BMP... STR_BMPMAP_ERR_IMAGE_TYPE :{WHITE}...could not convert image type. +STR_ERROR_HEIGHTMAP_TOO_LARGE :{WHITE}... image is too large + ##id 0x0800 STR_0800_COST :{TINYFONT}{RED}Cost: {CURRENCY} STR_0801_COST :{RED}Cost: {CURRENCY} diff --git src/sound.cpp src/sound.cpp index 9fed4a6..fef60b5 100644 --- src/sound.cpp +++ src/sound.cpp @@ -117,7 +117,8 @@ static bool SetBankSource(MixerChannel *mc, const FileEntry *fe) { assert(fe != NULL); - if (fe->file_size == 0) return false; + /* Check for valid sound size. */ + if (fe->file_size == 0 || fe->file_size > ((size_t)-1) - 2) return false; int8 *mem = MallocT(fe->file_size); if (mem == NULL) return false; diff --git src/sound/win32_s.cpp src/sound/win32_s.cpp index 386c6f4..2c184c6 100644 --- src/sound/win32_s.cpp +++ src/sound/win32_s.cpp @@ -60,6 +60,7 @@ static void CALLBACK waveOutProc(HWAVEOUT hwo, UINT uMsg, DWORD_PTR dwInstance, wfex.nAvgBytesPerSec = wfex.nSamplesPerSec * wfex.nBlockAlign; _bufsize = GetDriverParamInt(parm, "bufsize", (GB(GetVersion(), 0, 8) > 5) ? 2048 : 1024); + _bufsize = min(_bufsize, 65535); if (waveOutOpen(&_waveout, WAVE_MAPPER, &wfex, (DWORD_PTR)&waveOutProc, 0, CALLBACK_FUNCTION) != MMSYSERR_NOERROR) return "waveOutOpen failed"; diff --git src/spriteloader/png.cpp src/spriteloader/png.cpp index 363dce4..26440bf 100644 --- src/spriteloader/png.cpp +++ src/spriteloader/png.cpp @@ -102,7 +102,17 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i sprite->height = info_ptr->height; sprite->width = info_ptr->width; + /* Check if sprite dimensions aren't larger than what is allowed in GRF-files. */ + if (info_ptr->height > 255 || info_ptr->width > 65535) { + png_destroy_read_struct(&png_ptr, &info_ptr, &end_info); + return false; + } sprite->data = CallocT(sprite->width * sprite->height); + } else if (sprite->height != info_ptr->height || sprite->width != info_ptr->width) { + /* 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); @@ -110,6 +120,7 @@ static bool LoadPNG(SpriteLoader::Sprite *sprite, const char *filename, uint32 i if (mask && (bit_depth != 8 || color_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; }