Skip to content

Fix a series of buffer overflows in SOC (primarily Character section) loading

X.organic requested to merge X.organic/SRB2:deh-buffer-unclog into next

This merge request fixes a series of loosely related buffer overflows in dehacked.c and deh_soc.c, in SOC reading functions. They come up primarily with -D_FORTIFY_SOURCE set to 1 or 2 (default in Arch's makepkg and in Gentoo's Portage), where they readily trip buffer overflow detection. Without that flag, they generally go undetected but still may cause harm.

Notably, the buffer overflow in readPlayer is triggered by certain character SOCs. One easy way to test this is by taking Legacy Espio (https://mb.srb2.org/threads/legacy-espio-v2-95.29286/) and replacing the newline after PlayerText = in the OBJCTCFG with a space, so that the CSS text starts on the same line. This causes the strcpy in (former) deh_soc.c:242 to copy about 90 characters, making the following strcat copy about 355 characters but go just beyond the boundaries of description[num].notes, causing a buffer overflow caught only with -D_FORTIFY_SOURCE.

The dead code removed from DEH_LoadDehackedFile in the last commit can also trigger buffer overflows – note that origpos is of size 128 whereas size can approach MAXLINELEN (defined as 1024) – though those rarely happen by accident. If abused too heavily, this gets caught even without -D_FORTIFY_SOURCE by the stack smashing protector. Last time I had a buffer overflow there by accident, a bad PlayerText managed not to trip the -D_FORTIFY_SOURCE check but still cause a buffer overflow, resulting in random syntax errors that lead to ChrispyChars's PlayerText to get read in this context instead of in readPlayer, ending in an overflow here. As the code I removed – origpos, size and all of that – could potentially end up being used for something in the future, I'm open to suggestions on that part. I've considered leaving it in but with an overflow fix added.

Merge request reports

Loading