Engine crashing bsp file

[Spirit]

I love crashing engines, so here is a new file that crashed every engine I tried so far (all on Linux). Please make your engines more fault tolerant and safe to use.

desp.bsp from ftp://ftp.gamers.org/pub/mirrors/ftp.ga ... parado.zip

fitz, quakespasm, joequake-gl.glx, fteqw

Hunk_Alloc: bad size: -926890864

tyr-glquake

Error: Hunk_AllocName: bad size: -926890872

darkplaces

Memory pool 0xa41b008 has sprung a leak totalling 244478 bytes (0.233MB)! Listing contents...
244478 bytes allocated at fs.c:2915
Quake Error: Mem_Alloc: out of memory (alloc at model_brush.c:1428

glquake.sdl

Error: memory overwrite in Sys_Printf

Quakeforge (nq-sdl)

Fatal Error: Draw_Pic: bad coordinates

RMQEngine-Linux32

Hunk_Alloc: failed on 1422724528 bytes

zquake, ezquake

Received signal 11, exiting...

Alright, I tried quake.exe in dosbox and got "Error: Hunk_Alloc: bad size: -926890872". So I guess the file is faulty or not a Quake map in any way. Still, don't crash please. Some of the engines screwed up the desktop resolution and colour settings. And some of the errors look like quite dangerous memory leaks (not that I know anything about that).

[Baker]

A game engine has to be able to trust the game data. There is way too much even in loading a bsp file to get to a point that "wrong" or corrupt game data won't crash it. You'd have to do so many checks.

Besides, would you really want someone to invest that kind of effort on such a thing. It would be the equivalent of TSA "enhanced patdowns". And it still wouldn't work.

Yeah, I know you discovered it wasn't actually a q1bsp but even based on the list of engines it crashed if it were a q1bsp it would makes you wonder how the author could have possibly run it.

In the end, a Quake engine has to trust that the map being run was made by someone sane who used sane compile tools.

[leileilol]

Crashes Noesis, too

There's no .TXT file either so it's a chance that it could have been a botched upload.

[Spike]

errors from hunk_alloc are sane at least.
the problem is that there are no sanity values here, and quite a few multiplications can result in negative values. such checks can be written, but its much simpler to just let the allocator complain (but yes, that can still be remotely exploited in some situations...).

quakeforge's error is the most bewildering :)
darkplaces' leak is interesting, though likely slightly harder to exploit, but still exploitable.
glquake.sdl detects an error only after it has already crashed itself - though the specific exploit may not be exploitable.
(e)zquake just crashes as if it has no checks.

[mh]

I don't think that this is reasonable; there is just no way that I agree with it.

As an engine coder you have to assume that any content you're being fed has already survived a round-trip through the standard content creation tools and that the creator has actually tested it. Otherwise you're going deep into the silly world of "trust nothing, sanity check everything", and in the end it's the PLAYER who suffers because instead of getting newer and better engine features he's getting nothing but fixes for crap like "if you change the 3,276th byte of start.bsp to 0x7a you get a crash".

This is a counterproductive tunnel. Let's not go down there.

There is no way on earth that this map was able to compile successfully - it's reporting over 800,000,000 miptex, which is why - when we multiply things out - we start getting negative numbers in Hunk_Alloc.

Sometimes, believe it or not, crashing is the only good thing to do. If you have a crash condition - more often than not - by definition your memory is in an undefined state, your stack has been trampled over and is now garbage, your register contents are worthless, you can't even trust the address of an error handling routine.

One could just as easily say "mappers: please make your content more fault tolerant and safe to use".

Or how about - radical concept! - engine coders and mappers working together for the greater good of the community on things that actually matter to both?

[Spirit]

There is no way that I agree that an engine should expect supplied content to be well formed and nice. That is a very bad idea in terms of security.

If those HunkAlloc errors are not exploitable, well ok, no harm in crashing then. It would still be nice to either present the user with a message or even leave engine running and error to the console. But never screw up the desktop or colours.

What Spike said was what I had in mind. Exploits. An engine should not be a platform for possible system compromisation in any kind. And by that, it must fail properly.

Saying that users simply should not create bad input is missing the point. Programs should try to be not exploitable. User input is to be considered malicious.

I have no idea about the innards of engines, I am saying this as an annoyed user. Sorry if this came out in a bad way.

[mh]

OK, well look at it this way. The BSP is corrupt, it's not a valid Quake BSP (it might be 99% or more valid, but it's missing that one final little tiddly bit) so - as an engine coder - you need to make a call on how do you react.

In this case it seems fair enough, but let's change the numbers a little. Say that the BSP had a value like 63,161,284 for the number of textures in it. Now something magical happens. When you multiply that value by the size of a texture struct, numbers start becoming positive again (assuming a 32 bit CPU and a 68 byte texture_t), and are small enough so that a Hunk_Alloc will actually succeed.

So not only does a content bug get through, but when it actually starts trying to use the memory it thinks it has allocated something else gets stomped on and down in flames it goes.

Yes, it's annoying for a player to be faced with engine crashes, no questions about that. But it's also annoying for an engine coder to be expected to have to fix everything, and to have blame for everything assigned to them. There is obviously a balance to be struck here, and this is where people need to work together to identify bugs at their source and agree on the best means of dealing with them.

[Spike]

the way they can be exploited is to provide numbers that overflow twice over or so, resulting in a positive number and successful allocation, with a memcpy after that overwrites stuff later in the hunk.
With the hunk being what it is, the data later in the hunk which will be 'overwritten' is known - however, that data will be re-overwritten when the later stuff is loaded, and you'll crash if you try memcpying too much data into it. However, if the hunk is data which overflowed is rewritten later, you can change bits of data which were not meant to be changed, there could be function pointers stored there perhaps.
however, most things that overflow the heap will do so in a sane way that will eventually hit the end of it and result in a segfault or so.

at the end of the day, there are safe ways to crash and there are unsafe ways to crash. stopping this crash would require sanity limits on all inputs. if the data is filled in inside a loop with every element touched, then a hunk_alloc error is perfectly sufficient, from a security perspective. If the data is memcpied in after a loop, then the data might be 'truncated', and the wrong data might be read/written.
You can still overflow any engine's stack by providing endlessly recursive nodes, resulting in a nice segfault.

If you have a crash condition - more often than not - by definition your memory is in an undefined state, your stack has been trampled over and is now garbage, your register contents are worthless, you can't even trust the address of an error handling routine.

It depends upon the cause of the fault. Your stack should never be trampled over. You should never ever allow that to happen to your own stack. Sure, if the worst already happened then you can't safely recover, but you really ought to prevent your stack from ever getting corrupted in the first place. The only times when things are in an undefined state is in a last-chance already-crashed handler (the occurrence of which happened after any potential remote injection), and before your bss is cleared at startup (or result of malloc or whatever, generalize this as startup/load time. I only mention this state for completeness).

[Spirit]

Please do not put yourself into a position like that. I am not saying that engines should accept(!) broken maps or tolerate faulty files. If a mapper fails at providing a working file than it is his own damn fault.

I am looking at this from the viewpoint that programs which crash if they receive unexpected data are possibly insecure. What if someone investigated this issue further and exploited it with a nice little payload? (I know the probability is about 0.)

You would not say "people should not open bad PDF files in Adobe Reader" either or "do not create websites that exploit browser and plant malware on your system".

[szo]

I love crashing engines, so here is a new file that crashed every engine I tried so far (all on Linux). Please make your engines more fault tolerant and safe to use.

desp.bsp from ftp://ftp.gamers.org/pub/mirrors/ftp.ga ... parado.zip

fitz, quakespasm, joequake-gl.glx, fteqw

Hunk_Alloc: bad size: -926890864

tyr-glquake

Error: Hunk_AllocName: bad size: -926890872

[...]

Well, the engine did not "crash" here, it just securely Sys_Error()'ed out. I can't see the problem here where the engine really did the right thing.

[qbism]

Well, the engine did not "crash" here, it just securely Sys_Error()'ed out. I can't see the problem here where the engine really did the right thing.

Sys_Error is not a crash, but could there be improvements?

idea 1: Callback would allow the sys_error to tell us the calling function.

idea 2: What if Hunk_AllocName just returned NULL instead of sys_error, and left it to the calling function to print to console, sys_error, or only dprint it? More hassle for the coder, but the message could be more specific.

idea 2 example: Con_Dprint "Hunk allocation error, desp.bsp is not a valid map."

[szo]

Callback would allow the sys_error to tell us the calling function.

Providing a backtrace would be a hassle, but yes, useful at times.

idea 2: What if Hunk_AllocName just returned NULL instead of sys_error, and left it to the calling function to print to console, sys_error, or ...

Well, data sent to HunkAlloc[Name]() as a size argument can be broken in many ways, ignoring the need to byteswap a data read from the disk may be one of them: In this specific example I don't know the exact reason, but considering that the engine has enough checks everywhere, HunkAllocName() does the right thing here, imho.

[qbism]

Hunk_AllocName improvement microtute

Before:

Code: Select all

   if (size < 0)
      Sys_Error ("Hunk_Alloc: bad size: %i", size);

   size = sizeof(hunk_t) + ((size+15)&~15);

   if (hunk_size - hunk_low_used - hunk_high_used < size)
      Sys_Error ("Hunk_Alloc: failed on %i bytes",size);

After:

Code: Select all

if (size < 0)
      Sys_Error ("Hunk_AllocName: bad size: %i\nname:  %s", size, name); //qbism might as well report name, too

   size = sizeof(hunk_t) + ((size+15)&~15);

   if (hunk_size - hunk_low_used - hunk_high_used < size)
      Sys_Error ("Hunk_AllocName: failed on %i bytes\nname:  %s", size, name); //qbism might as well report name, too

[frag.machine]

There is no way that I agree that an engine should expect supplied content to be well formed and nice. That is a very bad idea in terms of security.

If those HunkAlloc errors are not exploitable, well ok, no harm in crashing then. It would still be nice to either present the user with a message or even leave engine running and error to the console. But never screw up the desktop or colours.

What Spike said was what I had in mind. Exploits. An engine should not be a platform for possible system compromisation in any kind. And by that, it must fail properly.

Saying that users simply should not create bad input is missing the point. Programs should try to be not exploitable. User input is to be considered malicious.

I have no idea about the innards of engines, I am saying this as an annoyed user. Sorry if this came out in a bad way.

I could reverse this shot and say "you mappers should take more care with bogus maps you release in public".

As everyone else said, there's no way to "protect", "prevent" or "gracefully" show an error message in all cases. There are too many invalid combinations. Of course, it's the developer role to apply any reasonable validation (like version numbers, or any check that doesn't cripples the engine). But this won't never prevent a choke caused by corrupted data or a malicious payload.

The "this can lead to an exploit" line is true, but also is true that in the end, content feed to the engine is something the engine user (read: the server admin) is accounted for, anyway. It's not fair to put such a burden over the engine coders shoulders.

[Spirit]

We are turning full circle, did you read all the posts above?

It is the user who is at harm. No, he must not need to trust the mapper or the server admin who makes his engine load a map. Again, do you trust websites with an insecure browser? I hope you keep your system and installed software up to date.

I am not ranting about the HunkAlloc "exit", as I said. If that is not exploitable, then the engine is doing almost the right thing. I say almost because the error message is cryptic to the user and to a engine code oblivious person like me sure looks like an unhandled crash. I'll try to keep in mind that HunkAlloc errors are "good" from now on.

What I do rant about (and I will not accept any excuses) are the crashes that screw up the system or allow an attacker to exploit. Those fixes are not sexy, but they need to be done in my opinion.

Also I do not consider myself a mapper. Would be great if we could avoid going on barricades on two opposite sides of the river Quake.