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


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 , and paid attention to the remarks shown at (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.