Code Execution in source ports.

[kgsws]

Since i have found multiple code execution paths in original Doom source code, and this source code is base for many source ports, here's a list of possible code execution paths.

With all modern OS protections, i can't tell if any of these bugs lead to code execution. But it's trivial to fix these anyway.

Function FindResponseFile in d_main.c.

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/d_main.c#L722C6-L722C22
This function is supposed to load command line arguments from a file. This function allocates fixed amount of 100 arguments, you can overflow it by providing more in load file.

This function is pretty much useless, just remove it. Many modern ports did just that.

 

Function PIT_CheckLine in p_map.c.

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_map.c#L242

Spechit overflow. This could overwrite some global variables.

Just add numspechit check. if(ld->special && numspechit < MAXSPECIALCROSS)

 

Function PIT_AddLineIntercepts and PIT_AddThingIntercepts in p_maputl.c.

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_maputl.c#L606

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_maputl.c#L616

Intercept overflow. This could overwrite some global variables. Some modern source ports emulate this for "all ghosts" bug.

Just add intercept_p check. if(intercept_p < intercepts + MAXINTERCEPTS)

 

File r_data.c.
Various structures with signed variables which should not get negative.
https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/r_data.c#L73

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/r_data.c#L88

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/r_data.c#L106

And maybe more ...

Using unsigned types is a quick but incomplete fix.

If you want to fix width / height / patch range properly, you should also check for some kind of MAX limit. Everywhere you load these structures from lumps.

Function M_LoadDefaults in m_misc.c.
https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/m_misc.c#L373
This function loads configuration entries from the file. Configuration text has space for 100 characters but format string for parsing "%79s %[^\n]\n" does not have any limit. This can lead to stack overflow.
Modern source ports fixed this issue by replacing original format string with this one: "%79s %99[^\n]\n". This will limit configuration text size to 100 bytes.

This function also has issue with string vs integer handling. You can provide integer in place of string. Which would likely lead to a crash, since it is only a pointer read access to bad memory address.

However, in original game, strings are only used for chat macros. Just keep this in mind if you intend to extend original config file with new entries.

Function P_SpawnMapThing in p_mobj.c.
https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_mobj.c#L732

It is possible to create things with negative doomednums. These will be interpreted as "negative" player starts. Every player start is stored in playerstarts array, which leads to negative indexing of array.
With careful selection of negative player start to avoid player-in-game check here, it is possible to overwrite any global variable, that happens to exist at specific negative index, with contents of this map thing.

This is, indeed, confirmed to allow code execution in DOS EXE.
To fix this, update mapthing_t structure with unsigned short type;. This will also allow using doomednums >= 32768.
Alternatively, some source ports simply return from P_SpawnMapThing if mthing->type <= 0.

 

Function Z_Malloc in z_zone.c.

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/z_zone.c#L185
This function accepts negative size. There are ways to sneak in negative value for allocation in specially crafted lumps in WAD file.

Negative allocation passes and, as a side effect, next free heap block entry will be positioned before current one. This heap block entry will overwrite previously allocated data. Furthermore, any new allocation will provide pointer to already used memory, allowing you to overwrite even more existing data, including existing heap block entry pointers.

This is, indeed, confirmed to allow code execution in DOS EXE.

To fix this, change every integer to unsigned integer. Including memblock_t structure.

Alternatively, call I_Error when size < 0. (this fix is easier)

 

File p_saveg.c functions P_UnArchivePlayers and P_UnArchiveThinkers.

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_saveg.c#L104

https://github.com/id-Software/DOOM/blob/master/linuxdoom-1.10/p_saveg.c#L302
This allows loading out of bounds states. Are out of bounds states useful? Maybe!

In DOS EXE:
1) It is possible to use state index which points to array of player_t structures. This is also loaded from save game file. Therefore, it is possible to create arbitrary state, including code action pointer!

2) It is possible to find state index where state action pointer overlaps with pointer to allocated sectors, which you can control from save game file. Therefore, it is possible to hide executable code in sectors.

Basicaly, any location where you read state index could be an entry point.

To fix this, do not blindly accept state index from save game file. Always check if it's in allowed range.

 

 

Now. This is not a full listing of all existing exploitable bugs in Doom code, these are only some i know about and, most importatnly, some are even present in existing source ports.

Also remember that these bugs might apply to other idtech 1 games - heretic, hexen, strife ...
(here's confirmed code execution in Hexen ACS)

Edited July 21 by kgsws

[Kinsie]

Cheers for sharing.

 

My understanding is that GZDoom has something resembling a responsible disclosure policy - which is to say that Zombie found some stuff and reported it privately, and it was disclosed after an official release fixing it was put out - but I'm not sure what the exact protocol for reporting such things is is beyond figuring out the correct person to DM and the right platform to DM them on (Discord? Github? ZDF? Fuckin' smoke signals?!?)

[GooberMan]

I'm gonna sneak in and say "don't change signed to unsigned integers". All the data has been cooked with signed, so all you'll be doing is allocating huge chunks of data for stuff that is either broken or malicious and then get random out of memory errors. Capture them with <0 checks and error message appropriately (especially when it comes to allocations).

[slowfade]

Interesting, thanks for compiling the list. I particularly like the one with P_SpawnMapThing as it only requires the player playing the map (if I understood correctly).

 

Not too long ago when I was mapping in vanilla, using action 37 (W1 floor lower to lowest floor (change texture)), I noticed it had a problem if the lowering sector had too many linedefs. This is known, yes, but my question is just whether it has been investigated as a possible way to cause code execution?

[Gez]

15 hours ago, Kinsie said:

Cheers for sharing.

 

My understanding is that GZDoom has something resembling a responsible disclosure policy - which is to say that Zombie found some stuff and reported it privately, and it was disclosed after an official release fixing it was put out - but I'm not sure what the exact protocol for reporting such things is is beyond figuring out the correct person to DM and the right platform to DM them on (Discord? Github? ZDF? Fuckin' smoke signals?!?)

You can PM the entire developer group on ZDF, and that's what has been used a few times.

[kgsws]

11 hours ago, GooberMan said:

I'm gonna sneak in and say "don't change signed to unsigned integers". All the data has been cooked with signed, so all you'll be doing is allocating huge chunks of data for stuff that is either broken or malicious and then get random out of memory errors. Capture them with <0 checks and error message appropriately (especially when it comes to allocations).

Well, places i have mentioned should be safe to change into unsigned. No existing and valid data should contain negative values on places i have mentioned.

Plus, some overflow bugs are true even for too high signed values.

 

2 hours ago, slowfade said:

Interesting, thanks for compiling the list. I particularly like the one with P_SpawnMapThing as it only requires the player playing the map (if I understood correctly).

Well, Z_Malloc combined with signed values with various negative values in texture data leads to code execution when you load the WAD. Even before graphics mode starts.

That's what ACE Engine uses.

 

2 hours ago, slowfade said:

Not too long ago when I was mapping in vanilla, using action 37 (W1 floor lower to lowest floor (change texture)), I noticed it had a problem if the lowering sector had too many linedefs. This is known, yes, but my question is just whether it has been investigated as a possible way to cause code execution?

Actually there are overflow bugs like that, yeah. I should add those.

[slowfade]

I kind of shudder to think what could've happened if these exploits had been found at the time when people just downloaded wads and ran them natively in the original Doom exes.

[Gez]

43 minutes ago, slowfade said:

I kind of shudder to think what could've happened if these exploits had been found at the time when people just downloaded wads and ran them natively in the original Doom exes.

I think it'd have forced id software to release a patch that fixed known exploits (perhaps repeatedly, as more exploits got discovered) and it probably would have resulted in a few more complevels to handle for demo playback.

[GooberMan]

1 hour ago, kgsws said:

Well, places i have mentioned should be safe to change into unsigned. No existing and valid data should contain negative values on places i have mentioned.

 

So let's take a texture with width and height of -1 each. Cast to unsigned, that's 65535x65535 texels if it's a compositable texture. Which is 4294836225. Which is just shy of 4 gigabytes. Best you can do with signed values is 1 gigabyte.

 

Being able to fill memory with a valid range is an entirely different thing. Don't reinterpret data to however you see fit just because it can break another way.

 

(I'm pretty strict on these points, since these kinds of assumptions are basically game security 101 - and especially since there's three decades worth of tools in this community that work with signed values.)

[kgsws]

1 minute ago, GooberMan said:

So let's take a texture with width and height of -1 each. Cast to unsigned, that's 65535x65535 texels if it's a compositable texture. Which is 4294836225. Which is just shy of 4 gigabytes. Best you can do with signed values is 1 gigabyte.

With fixed Z_Malloc, allocating this much memory would simply fail.

 

3 minutes ago, GooberMan said:

Don't reinterpret data to however you see fit just because it can break another way.

My point is that when doom was coded, they clearly did not care about signed vs unsigned values. There are no assumptions about negative vs positive values. Therefore, keeping signed values vs using unsigned values does not really matter if you don't also add other checks. Both cases are broken in some way.

(except in a few places where they use negative values to convey special meaning)

 

Anyway, if you want to fix width / height / patch range properly, you should also check for some kind of top limit. You could also check, as you have mentioned, for negative values.

I'm gonna mention "negative check" in my original post.

[Quasar]

Most level values are already extended to unsigned in advanced ports anyways because this is how we support larger levels. You can't have a negative sector or sidedef offset in Eternity in a binary map because those values are treated as unsigned anyways. So the debate over whether to extend or not is moot in those points for most implementations at this point. Otherwise, I'd also argue that you can always also add a sanity check for excessive size, whether or not the values are signed or unsigned. What valid use case do community tools have for negatively sized images for example? Either you're going to do if(w <= 0 || h <= 0) invalid; or you're going to do if(w * h > SANITY_THRESHOLD) invalid; - I see no difference except the latter lets you support reasonably larger images and the former does not.

[GooberMan]

1 hour ago, kgsws said:

With fixed Z_Malloc, allocating this much memory would simply fail

 

No it wouldn't. Rum and Raisin's Z_Malloc wraps in to malloc/dlmalloc. It would keep allocating 4 gigabytes and slow the system down as you hit physical RAM limit and start swapping virtual memory.

[GooberMan]

23 minutes ago, Quasar said:

Most level values are already extended to unsigned in advanced ports anyways because this is how we support larger levels.

Yes, and you need to special case things with heuristics to handle it.

Finding security holes is one thing. Taking an approach that creates more problems is another.

[kgsws]

51 minutes ago, GooberMan said:

No it wouldn't. Rum and Raisin's Z_Malloc wraps in to malloc/dlmalloc. It would keep allocating 4 gigabytes and slow the system down as you hit physical RAM limit and start swapping virtual memory.

OK, for custom implementations it would not. Either way, code execution path is gone.

 

47 minutes ago, GooberMan said:

Yes, and you need to special case things with heuristics to handle it.

That's one way to do this. I would use one structure with unsigned values and do something like this for old maps when parsing:

if(vanilla_format && loadsubsec->firstseg >= numsegs)
	I_Error("Invalid subsector!")

Because, technically, this check should be done anyway at some point. You would have to count segs first, of course.

 

52 minutes ago, GooberMan said:

Finding security holes is one thing. Taking an approach that creates more problems is another.

Of course range checking everywhere is better.