[4.10.0/3.88b] Buffer Overflow Vulnerability

[Knursoft]

So i was testing gzdoom and it seems there is a buffer overflow vulnerability in a IMGZImage_TryCreate function. When it reads a magic number IMGZ, the read function checks if variables FilePos (which is wad's directory pointer to the start of the lump's data) + len (which is 4, size of magic number) is greater than StartPos (wad's directory pointer to lump's data again) + Length (size of lump in bytes) variables. If it is larger, it sets len (4 bytes) to Length - FilePos + StartPos. The problem occurs when the Length and StartPos are added in if statement. These variables are 32 bit signed integers, so they cannot exceed value 0x7fffffff, but when we add for example values 0x40000000 + 0x50000000 the result will cause the integer overflow, and it will return actually less than the FilePos + len is, causing statement to be true, and setting len variable to Length - FilePos + StartPos, which will cause the mReader->Read to actually read much more bytes than the magic buffer is capable to hold, resulting overwritting variables on stack, including the return address. This vulnerability also occurs in LZDoom in which the OpenAL32.dll library is compiled without ASLR and DEP, which makes it simpler to exploit by potential attackers.

The simplest fix for this is declaring "Length" variable in files.h:67 as unsigned long.

[Graf Zahl]

The core problem here is that stdio is based on longs which on windows are 32 bit, unfortunately this currently affects the entire file access code.
In short, the entire setup is not safe for large files above 2GB. Refactoring is not easy thanks to the sloppy downconversion rules in C and C++ which make it virtually impossible to find all affected places.

[Graf Zahl]

The entire innards of FileReader now use 64 bit for all size values.

[Knursoft]

Okay, cool.