OpenTTD

Tasklist

FS#4746 - Fix BMP RLE decoding

Attached to Project: OpenTTD
Opened by Matt D. (monoid) - Thursday, 25 August 2011, 15:16 GMT
Last edited by Michael Lutz (michi_cc) - Friday, 02 September 2011, 20:18 GMT
Type Patch
Category Core
Status Closed
Assigned To No-one
Operating System All
Severity Medium
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch fixes errors in the decoding of RLE-compressed BMP format images. The write pointer of the decoder can be positioned anywhere in address space using a series of end-of-line and delta RLE instructions, and then arbitrary data written, leading to i.e. heap/stack modification.

A sample exploit, leading to arbitrary code execution on OpenTTD 1.1.2 on a WinXP SP2 test machine is available upon request.
This task depends upon

Closed by  Michael Lutz (michi_cc)
Friday, 02 September 2011, 20:18 GMT
Reason for closing:  Implemented
Additional comments about closing:  In r22871 and r22872.
Comment by Michael Lutz (michi_cc) - Thursday, 25 August 2011, 15:53 GMT
Patch seems good, I'm wondering why the code would use "y != 0 || x < info->width" instead of "y != 0 && x < info->width" for the while condition though. Anything I'm missing on the RLE encoding here?
Comment by Michael Lutz (michi_cc) - Thursday, 25 August 2011, 19:51 GMT
Using && instead of || would fix a part of the exploit, but would return the wrong return value, so extra y == 0 checks are needed.

The broken code is in trunk since the TGP merge at r5946 which means 0.5.0 and later.
Comment by Matt D. (monoid) - Friday, 26 August 2011, 15:08 GMT
I've taken the opportunity to fix/clean up the BMP RLE decoding routines further in this new patch.

It successfully decodes the attached 4-bit RLE BMP image, whereas 1.1.2 fails to decode it. I closely followed the 'specs' at http://www.fileformat.info/format/bmp/egff.htm , and paid attention to the remarks shown at http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/bmp/BMPImageReader.cpp?rev=77429#L503 (it is actually images with these overlong uncompressed rows that 1.1.2 currently chokes on).

It is easier codewise to deal with an uninverted y-coordinate, and simply invert it when using it to get the pixel write pointer position.
Comment by Matt D. (monoid) - Friday, 26 August 2011, 15:16 GMT
Er, that's actually an 8-bit image (with 16 colours) >_>.

Either way, it doesn't currently load in 1.1.2, due to it not handling overlong uncompressed rows as mentioned.

Also, should the BMP-internal file buffering stuff be removed, and replaced with the generic FioReadXXX() system?
Comment by Matt D. (monoid) - Friday, 26 August 2011, 16:25 GMT
Fixed the patch, added some necessary y-coordinate checks.
Comment by Matt D. (monoid) - Wednesday, 31 August 2011, 09:02 GMT
This additional patch fixes up the error reporting when a heightmap image fails to load - currently the error condition is ignored.

Loading...