From daa89c982e6a56acf543d34160b967bd42cfc380 Mon Sep 17 00:00:00 2001 From: frosch Date: Sat, 3 Sep 2011 18:56:34 +0000 Subject: [PATCH] (svn r22886) [1.1] -Backport from trunk: - Fix: Harden memory allocation (r22881, r22880, r22875) - Fix: Validate image dimensions before loading [FS#4747] (r22878, r22877, r22874, r22873) - Fix: Perform stricter checks on RLE compressed BMP images [FS#4746] (r22872, r22871) --- src/bmp.cpp | 152 +++++++++++++++++++-------------- src/core/alloc_func.hpp | 33 ++++++- src/fontcache.cpp | 3 + src/heightmap.cpp | 25 +++++- src/lang/english.txt | 2 + src/misc/binaryheap.hpp | 2 + src/misc/blob.hpp | 1 + src/misc/fixedsizearray.hpp | 3 + src/openttd.cpp | 9 +- src/pathfinder/npf/queue.cpp | 3 + src/saveload/saveload.h | 6 -- src/script/squirrel_helper.hpp | 3 + src/sound.cpp | 3 +- src/sound/win32_s.cpp | 3 + src/spriteloader/png.cpp | 17 +++- src/stdafx.h | 5 ++ 16 files changed, 188 insertions(+), 82 deletions(-) diff --git a/src/bmp.cpp b/src/bmp.cpp index 2c85c768b4..a93785d4f4 100644 --- a/src/bmp.cpp +++ b/src/bmp.cpp @@ -130,53 +130,63 @@ static inline bool BmpRead4(BmpBuffer *buffer, BmpInfo *info, BmpData *data) */ static inline bool BmpRead4Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) { - uint i; uint x = 0; uint y = info->height - 1; - byte n, c, b; byte *pixel = &data->bitmap[y * info->width]; while (y != 0 || x < info->width) { if (EndOfBuffer(buffer)) return false; // the file is shorter than expected - n = ReadByte(buffer); - c = ReadByte(buffer); + + byte n = ReadByte(buffer); + byte c = ReadByte(buffer); if (n == 0) { switch (c) { - case 0: // end of line - x = 0; - pixel = &data->bitmap[--y * info->width]; - break; - case 1: // end of bitmap - x = info->width; - y = 0; - pixel = NULL; - break; - case 2: // delta - x += ReadByte(buffer); - i = ReadByte(buffer); - if (x >= info->width || (y == 0 && i > 0)) return false; - y -= i; - pixel = &data->bitmap[y * info->width + x]; - break; - default: // uncompressed - i = 0; - while (i++ < c) { - if (EndOfBuffer(buffer) || x >= info->width) return false; - b = ReadByte(buffer); - *pixel++ = GB(b, 4, 4); - x++; - if (x < info->width && i++ < c) { - *pixel++ = GB(b, 0, 4); - x++; - } + case 0: // end of line + x = 0; + if (y == 0) return false; + pixel = &data->bitmap[--y * info->width]; + break; + + case 1: // end of bitmap + return true; + + case 2: { // delta + if (EndOfBuffer(buffer)) return false; + byte dx = ReadByte(buffer); + byte dy = ReadByte(buffer); + + /* Check for over- and underflow. */ + if (x + dx >= info->width || x + dx < x || dy > y) return false; + + x += dx; + y -= dy; + pixel = &data->bitmap[y * info->width + x]; + break; + } + + default: { // uncompressed + uint i = 0; + while (i++ < c) { + if (EndOfBuffer(buffer) || x >= info->width) return false; + byte b = ReadByte(buffer); + *pixel++ = GB(b, 4, 4); + x++; + if (i++ < c) { + if (x >= info->width) return false; + *pixel++ = GB(b, 0, 4); + x++; + } + } + /* Padding for 16 bit align */ + SkipBytes(buffer, ((c + 1) / 2) % 2); + break; } - /* Padding for 16 bit align */ - SkipBytes(buffer, ((c + 1) / 2) % 2); - break; } } else { - i = 0; - while (i++ < n) { - if (EndOfBuffer(buffer) || x >= info->width) return false; + /* Apparently it is common to encounter BMPs where the count of + * pixels to be written is higher than the remaining line width. + * Ignore the superfluous pixels instead of reporting an error. */ + uint i = 0; + while (x < info->width && i++ < n) { *pixel++ = GB(c, 4, 4); x++; if (x < info->width && i++ < n) { @@ -213,43 +223,55 @@ static inline bool BmpRead8(BmpBuffer *buffer, BmpInfo *info, BmpData *data) */ static inline bool BmpRead8Rle(BmpBuffer *buffer, BmpInfo *info, BmpData *data) { - uint i; uint x = 0; uint y = info->height - 1; - byte n, c; byte *pixel = &data->bitmap[y * info->width]; while (y != 0 || x < info->width) { if (EndOfBuffer(buffer)) return false; // the file is shorter than expected - n = ReadByte(buffer); - c = ReadByte(buffer); + + byte n = ReadByte(buffer); + byte c = ReadByte(buffer); if (n == 0) { switch (c) { - case 0: // end of line - x = 0; - pixel = &data->bitmap[--y * info->width]; - break; - case 1: // end of bitmap - x = info->width; - y = 0; - pixel = NULL; - break; - case 2: // delta - x += ReadByte(buffer); - i = ReadByte(buffer); - if (x >= info->width || (y == 0 && i > 0)) 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); - /* Padding for 16 bit align */ - SkipBytes(buffer, c % 2); - break; + case 0: // end of line + x = 0; + if (y == 0) return false; + pixel = &data->bitmap[--y * info->width]; + break; + + case 1: // end of bitmap + return true; + + case 2: { // delta + if (EndOfBuffer(buffer)) return false; + byte dx = ReadByte(buffer); + byte dy = ReadByte(buffer); + + /* Check for over- and underflow. */ + if (x + dx >= info->width || x + dx < x || dy > y) return false; + + x += dx; + y -= dy; + pixel = &data->bitmap[y * info->width + x]; + break; + } + + default: { // uncompressed + for (uint 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; + } } } else { - for (i = 0; i < n; i++) { - if (x >= info->width) return false; + /* Apparently it is common to encounter BMPs where the count of + * pixels to be written is higher than the remaining line width. + * Ignore the superfluous pixels instead of reporting an error. */ + for (uint i = 0; x < info->width && i < n; i++) { *pixel++ = c; x++; } diff --git a/src/core/alloc_func.hpp b/src/core/alloc_func.hpp index 6f70627edb..4ba8c5f53a 100644 --- a/src/core/alloc_func.hpp +++ b/src/core/alloc_func.hpp @@ -22,6 +22,29 @@ void NORETURN MallocError(size_t size); void NORETURN ReallocError(size_t size); +/** + * Checks whether allocating memory would overflow size_t. + * + * @param element_size Size of the structure to allocate. + * @param num_elements Number of elements to allocate. + */ +static inline void CheckAllocationConstraints(size_t element_size, size_t num_elements) +{ + if (num_elements > SIZE_MAX / element_size) MallocError(SIZE_MAX); +} + +/** + * Checks whether allocating memory would overflow size_t. + * + * @tparam T Structure to allocate. + * @param num_elements Number of elements to allocate. + */ +template +static inline void CheckAllocationConstraints(size_t num_elements) +{ + CheckAllocationConstraints(sizeof(T), num_elements); +} + /** * Simplified allocation function that allocates the specified number of * elements of the given type. It also explicitly casts it to the requested @@ -42,6 +65,9 @@ static FORCEINLINE T *MallocT(size_t num_elements) */ if (num_elements == 0) return NULL; + /* Ensure the size does not overflow. */ + CheckAllocationConstraints(num_elements); + T *t_ptr = (T*)malloc(num_elements * sizeof(T)); if (t_ptr == NULL) MallocError(num_elements * sizeof(T)); return t_ptr; @@ -96,12 +122,17 @@ static FORCEINLINE T *ReallocT(T *t_ptr, size_t num_elements) return NULL; } + /* Ensure the size does not overflow. */ + CheckAllocationConstraints(num_elements); + t_ptr = (T*)realloc(t_ptr, num_elements * sizeof(T)); if (t_ptr == NULL) ReallocError(num_elements * sizeof(T)); return t_ptr; } /** alloca() has to be called in the parent function, so define AllocaM() as a macro */ -#define AllocaM(T, num_elements) ((T*)alloca((num_elements) * sizeof(T))) +#define AllocaM(T, num_elements) \ + (CheckAllocationConstraints(num_elements), \ + (T*)alloca((num_elements) * sizeof(T))) #endif /* ALLOC_FUNC_HPP */ diff --git a/src/fontcache.cpp b/src/fontcache.cpp index b279c3463c..616c54a9ef 100644 --- a/src/fontcache.cpp +++ b/src/fontcache.cpp @@ -1034,6 +1034,9 @@ const Sprite *GetGlyph(FontSize size, WChar key) 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 a/src/heightmap.cpp b/src/heightmap.cpp index 8790da59e6..64f535943a 100644 --- a/src/heightmap.cpp +++ b/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, WL_ERROR); + 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, WL_ERROR); + fclose(f); + BmpDestroyData(&data); + return false; + } + if (map != NULL) { if (!BmpReadBitmap(&buffer, &info, &data)) { ShowErrorMessage(STR_ERROR_BMPMAP, STR_ERROR_BMPMAP_IMAGE_TYPE, WL_ERROR); diff --git a/src/lang/english.txt b/src/lang/english.txt index c8253fc405..ad04d7579e 100644 --- a/src/lang/english.txt +++ b/src/lang/english.txt @@ -3445,6 +3445,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 a/src/misc/binaryheap.hpp b/src/misc/binaryheap.hpp index c0aee7f2dc..81948c78f5 100644 --- a/src/misc/binaryheap.hpp +++ b/src/misc/binaryheap.hpp @@ -198,6 +198,8 @@ public: FORCEINLINE void Include(T *new_item) { if (this->IsFull()) { + assert(this->capacity < UINT_MAX / 2); + this->capacity *= 2; this->data = ReallocT(this->data, this->capacity + 1); } diff --git a/src/misc/blob.hpp b/src/misc/blob.hpp index bd83904eab..94459a3659 100644 --- a/src/misc/blob.hpp +++ b/src/misc/blob.hpp @@ -260,6 +260,7 @@ public: if (Capacity() >= new_size) return; /* calculate minimum block size we need to allocate * and ask allocation policy for some reasonable block size */ + assert(new_size < SIZE_MAX - header_size - tail_reserve); new_size = AllocPolicy(header_size + new_size + tail_reserve); /* allocate new block and setup header */ diff --git a/src/misc/fixedsizearray.hpp b/src/misc/fixedsizearray.hpp index c9e4ea59ac..32010f1d96 100644 --- a/src/misc/fixedsizearray.hpp +++ b/src/misc/fixedsizearray.hpp @@ -53,6 +53,9 @@ public: /** Default constructor. Preallocate space for items and header, then initialize header. */ FixedSizeArray() { + /* Ensure the size won't overflow. */ + assert_compile(C < (SIZE_MAX - HeaderSize) / Tsize); + /* allocate block for header + items (don't construct items) */ data = (T*)((MallocT(HeaderSize + C * Tsize)) + HeaderSize); SizeRef() = 0; // initial number of items diff --git a/src/openttd.cpp b/src/openttd.cpp index 4041cf8c7f..4329f5ee46 100644 --- a/src/openttd.cpp +++ b/src/openttd.cpp @@ -577,11 +577,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) DEBUG(net, 0, "Starting dedicated version %s", _openttd_revision); diff --git a/src/pathfinder/npf/queue.cpp b/src/pathfinder/npf/queue.cpp index b235c3b0eb..f964ece24b 100644 --- a/src/pathfinder/npf/queue.cpp +++ b/src/pathfinder/npf/queue.cpp @@ -234,6 +234,9 @@ void Hash::Init(Hash_HashProc *hash, uint num_buckets) /* Allocate space for the Hash, the buckets and the bucket flags */ uint i; + /* Ensure the size won't overflow. */ + CheckAllocationConstraints(sizeof(*this->buckets) + sizeof(*this->buckets_in_use), num_buckets); + this->hash = hash; this->size = 0; this->num_buckets = num_buckets; diff --git a/src/saveload/saveload.h b/src/saveload/saveload.h index 7c85965326..cd5602aa2e 100644 --- a/src/saveload/saveload.h +++ b/src/saveload/saveload.h @@ -15,12 +15,6 @@ #include "../fileio_type.h" #include "../strings_type.h" -#ifdef SIZE_MAX -#undef SIZE_MAX -#endif - -#define SIZE_MAX ((size_t)-1) - /** Save or load result codes. */ enum SaveOrLoadResult { SL_OK = 0, ///< completed successfully diff --git a/src/script/squirrel_helper.hpp b/src/script/squirrel_helper.hpp index a7d0bf7ba2..babdf74b3b 100644 --- a/src/script/squirrel_helper.hpp +++ b/src/script/squirrel_helper.hpp @@ -118,6 +118,9 @@ namespace SQConvert { 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 a/src/sound.cpp b/src/sound.cpp index 060eb60732..438b67d94a 100644 --- a/src/sound.cpp +++ b/src/sound.cpp @@ -111,7 +111,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 a/src/sound/win32_s.cpp b/src/sound/win32_s.cpp index c0e5da5d20..d241f45c81 100644 --- a/src/sound/win32_s.cpp +++ b/src/sound/win32_s.cpp @@ -15,6 +15,7 @@ #include "../mixer.h" #include "../core/alloc_func.hpp" #include "../core/bitmath_func.hpp" +#include "../core/math_func.hpp" #include "win32_s.h" #include #include @@ -63,7 +64,9 @@ const char *SoundDriver_Win32::Start(const char * const *parm) 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 a/src/spriteloader/png.cpp b/src/spriteloader/png.cpp index fa66ae3f1a..60c84048b3 100644 --- a/src/spriteloader/png.cpp +++ b/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; } diff --git a/src/stdafx.h b/src/stdafx.h index b226e44f4a..44180b6137 100644 --- a/src/stdafx.h +++ b/src/stdafx.h @@ -63,6 +63,10 @@ #include #include +#ifndef SIZE_MAX + #define SIZE_MAX ((size_t)-1) +#endif + #if defined(UNIX) || defined(__MINGW32__) #include #endif @@ -335,6 +339,7 @@ assert_compile(sizeof(uint64) == 8); assert_compile(sizeof(uint32) == 4); assert_compile(sizeof(uint16) == 2); assert_compile(sizeof(uint8) == 1); +assert_compile(SIZE_MAX >= UINT32_MAX); #ifndef M_PI_2 #define M_PI_2 1.57079632679489661923