[fixed]TGA files with illegal palette entries cause buffer overflow?

[erlehmann]

A developer of Minetest, sfan5, has found a problem in the TGA loader code that Minetest inherited from Irrlicht.
Since sfan5 does not plan to report it upstream (“currently busy fixing the other bugs AFL found”), I am doing it:

The issue happens when a colormapped TGA file has a pixel that refers to a non-existent colormap entry.
Here is an example: You have 4 colors in your colormap for your image, a pixel refers to the 120th color.

I suggest to check each TGA payload pixel for colormapped images against the size of the colormap.

Here is the IrrlichtMT (Minetest fork of Irrlicht) issue: <https://github.com/minetest/irrlicht/issues/236>

[erlehmann]

To quote myself regarding another thing I found:

Besides throwing up on illegal colors, it seems to me that the TGA loader code cheats on color depth for palette images. As far as I understand it, it converts all colors to ECF_A1R5G5B5. In the case of the test image, it looks to me like that code reduces the color depth from 8 bit per color to 5 bit per color. If done correctly, the effect is not noticeable to the bare eye (at least not mine) for pixel arts anyway (and I have employed it in the past), but it is a bit cheaty (and probably undesired for colormapped images that have partially transparent colors).

[CuteAlien]

OK, have to read in detail later. I remember I got another fix about checking sizes for TGA before (also from sfan5) which is applied already (in svn trunk r6387).

Your tga_samples.zip does not seem to contain the actual .tga file which makes it crash. Do you have that around as well by any chance?

[erlehmann]

The tga_samples.zip does indeed contain the actual file which makes it corrupt memory (not sure if crash).

Just rename id:000000.bin to crash.tga and you got the file.

[erlehmann]

CuteAlien, maybe this patch I just created for tga_encoder helps you to figure out the exact error condition:
https://git.minetest.land/erlehmann/tga ... eca77f062c

You can easily create this yourself using a hex editor by the way.

[CuteAlien]

That one will unlikely ever crash (it only reads memory it shouldn't, so at most an information leak - which in this case is restricted to 256 bytes, but could be up to 64k).

Thought I noticed later examples (like id_000013) actually will crash and write into invalid memory. Sadly check for palette a bit expensive (as all per pixel operations on cpu). For the 256 entries case I could easily just use a big enough array, but I guess for 16-bit entries it's a bit overkill *sigh* (edit: But luckily turned out we don't support 16-bit lookup for palettes there, so easy and cheap fix was possible).

The later examples with real crashes seem cheaper easier to fix.

I'll see what I can do. But if we go for safety in all loaders... bit tricky. TGA is still a simple format, for BMP it's already harder to fix (and that's still a relative simple format). I started with BMP once, but gave up after a while, so I still know quite a few crashes for that one.Those loaders were written back in times where only speed mattered and people expected to have control over the images they fed their games.

[CuteAlien]

OK, all fixes got put into svn trunk.
r6532 fixes reading from bad palette entries
r6533 prevents downsampling all palette images to ECF_A1R5G5B5 (thought 24 bit will become 32 bit now, at least no loss...)
r6534 fixes overwriting memory when RLE info was bad (probably worst bug here)
r6535 fixes another bug which could lead to crashes and maybe memory overwriting caused by a bad image size calculation.
Note that it still can crash when memory runs out, but that's fine.

This fixes all I've seen in sfan5's fuzzing tests. If I missed something please update me.
Thanks for passing this report on!

[sfan5]

I also fuzzed the other image and mesh formats our fork still supports and have published my fixes here:
https://github.com/minetest/irrlicht/co ... 5cedac4e0a (affects BMP, X, B3D, OBJ)

Also, the 4-bit RLE decompression for BMPs is probably totally broken. Search for "bug?" in the diff.

[CuteAlien]

Thanks, I'll check (and probably add) them.
About bmp - I found once a manually created test-suite for those: http://entropymine.com/jason/bmpsuite/b ... suite.html
Last time I tested badrle4ter.bmp, reallybig.bmp and rletopdown.bmp from there still crashed, but I'll give them another run once I've applied your patches.
And quite a few which did not work so far.

edit: Yeah - "bug?" looks also like a bug to me. I think that bmpsuite had a few 4 bit tests which looked strange in the past, will test if they improve by increasing pointer instead of value :-) That second bmp patch is generally really nice work! I had to think quite a bit for a few of those checks until I believed you were right.

[CuteAlien]

OK, except last one they are all in one form or the other in 1.8 and trunk branches now.
Didn't apply the error changing one for X loader so far as I'm not sure yet if that's a good idea in general. You mentioned in the patch that it's a pretty big problem, but didn't mention what the problem there actually is. If it only results in half-broken meshes it shouldn't be too bad for the engine (as long as there are log infos about it). If this causes other troubles - would then be nice to have a test-case so I can see what's going on.

And if there are more patches coming - they are very welcome, but please use a new thread! Once a bug-thread is handled I kinda prefer if it stays that way and new bugs get their own threads ;-) If there are many bugs in a single thread and I don't handle them all the unhandled ones tend to get forgotten.

Also rle4 bmp loader works now. But I'm pretty sure if rle8 has delta info (bmp's are allowed to skip pixels) those will also be broken. Just haven't found a test-image for that case yet. (Edit: turns out rle8 with delta seem good, thought with early line ends it leaves some unitinitalized colors)

Edit: Did a few more tests for BMP and above mentioned test-suite no longer has any crashes!