Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: Echelon9 on November 29, 2008, 09:18:20 pm

Title: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
Post by: Echelon9 on November 29, 2008, 09:18:20 pm
Just a warning that the recent r4963 commit to address "the message variable parsing the right way" has introduced an array out-of-bounds read on the Sexp_variables[MAX_SEXP_VARIABLES] array.

Sexp_variables[] is defined at sexp.cpp:2169 as:

Code: [Select]
sexp_variable Sexp_variables[MAX_SEXP_VARIABLES];
Elements in an array begin numbering at 0 (not 1), so the valid elements are 0 -> MAX_SEXP_VARIABLES-1.

Thus the for loop at sexp.cpp:20197 sets the initial value for i as off-by-one, and the subsequent operation on the next line "if (Sexp_variables[ i ].type & SEXP_VARIABLE_SET)" accesses memory outside the Sexp_variables[] array. This could lead to a number of access violation, information disclosure or subsequent memory corruption bugs, depending on what data the compiler places in adjacent memory.

Code: [Select]
for (int i=MAX_SEXP_VARIABLES; i >= 0; i--) {          // <-- Uh-oh!!
if (Sexp_variables[i].type & SEXP_VARIABLE_SET) {
// check case sensitive
// check number of chars in variable name
if ( !strncmp(startpos, Sexp_variables[i].variable_name, strlen(Sexp_variables[i].variable_name)) ) {
return i;
}
}
}

Unfortunately, due to the issues with Mantis preventing me from registering an account I can't lodge it there, but just a heads up that the proposed change I would make would be:

Code: [Select]
// Fixed code to address array out-of-bounds read
for (int i=MAX_SEXP_VARIABLES-1; i >= 0; i--) {
Title: Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
Post by: FUBAR-BDHR on November 29, 2008, 10:36:07 pm
I put a link to this thread in Mantis.

Strange thing is I am not getting any errors or warnings when I test this. 
Title: Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
Post by: Echelon9 on November 29, 2008, 11:25:57 pm
Strange thing is I am not getting any errors or warnings when I test this. 

These sort of out-of-bound array bugs can be fickle in their impact on testing. It depends on the data that the compiler places after the array (as this is on the stack) and what the program does with that adjacent data afterwards. Usually, out-of-bound writes are much more problematic, but it is good habit to fix these bugs once found, as a habit of secure coding.

In this particular case, I believe the possibility of an outright crash in testing is low. This is in part why this class of bug is so insidious, and difficult to track down.

The function get_index_sexp_variable_name_special() merely does some checks, and then returns i or -1. However, if the function calling get_index_sexp_variable_name_special() were then to trust the value of i returned is sane (which it might not be should it be set to MAX_SEXP_VARIABLES) then later use of i with Sexp_variables[] could lead to more critical bugs.

The long and the short is that it could lead to operations on Sexp_variables[] actually affecting other memory, and that is a problem.
Title: Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
Post by: chief1983 on November 30, 2008, 04:08:07 am
Glad my suspicions were verified, I was already aware this was probably an issue with the commit but hadn't heard any reports until now that it actually broke anything.  It'll be fixed shortly I'm sure.
Title: Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
Post by: phreak on November 30, 2008, 08:26:47 am
fixed.  i remember there was a reason why i never do reverse for loops.