Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BMP RLE decoding #4746

Closed
DorpsGek opened this issue Aug 25, 2011 · 7 comments
Closed

Fix BMP RLE decoding #4746

DorpsGek opened this issue Aug 25, 2011 · 7 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Aug 25, 2011

monoid opened the ticket and wrote:

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.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/4746
@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 25, 2011

michi_cc wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10231

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 25, 2011

michi_cc wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10236

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 26, 2011

monoid wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10241

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 26, 2011

monoid wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10242

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 26, 2011

monoid wrote:

Fixed the patch, added some necessary y-coordinate checks.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10243

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 31, 2011

monoid wrote:

This additional patch fixes up the error reporting when a heightmap image fails to load - currently the error condition is ignored.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746#comment10274

@DorpsGek
Copy link
Member Author

DorpsGek commented Sep 2, 2011

michi_cc closed the ticket.

Reason for closing: Implemented

In r22871 and r22872.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4746

@DorpsGek DorpsGek closed this as completed Sep 2, 2011
@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant