Author Topic: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197  (Read 2498 times)

0 Members and 1 Guest are viewing this topic.

Offline Echelon9

  • 210
r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
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--) {

 

Offline FUBAR-BDHR

  • Self-Propelled Trouble Magnet
  • 212
  • Master Drunk
    • 165th Beer Drinking Hell Raisers
Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
I put a link to this thread in Mantis.

Strange thing is I am not getting any errors or warnings when I test this. 
No-one ever listens to Zathras. Quite mad, they say. It is good that Zathras does not mind. He's even grown to like it. Oh yes. -Zathras

 

Offline Echelon9

  • 210
Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
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.

 

Offline chief1983

  • Still lacks a custom title
  • Moderator
  • 212
  • ⬇️⬆️⬅️⬅️🅰➡️⬇️
    • Minecraft
    • Skype
    • Steam
    • Twitter
    • Fate of the Galaxy
Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
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.
Fate of the Galaxy - Now Hiring!  Apply within | Diaspora | SCP Home | Collada Importer for PCS2
Karajorma's 'How to report bugs' | Mantis
#freespace | #scp-swc | #diaspora | #SCP | #hard-light on EsperNet

"You may not sell or otherwise commercially exploit the source or things you created based on the source." -- Excerpt from FSO license, for reference

Nuclear1:  Jesus Christ zack you're a little too hamyurger for HLP right now...
iamzack:  i dont have hamynerge i just want ptatoc hips D:
redsniper:  Platonic hips?!
iamzack:  lays

 

Offline phreak

  • Gun Phreak
  • 211
  • -1
Re: r4963: Sexp_variables[] Array out-of-bounds read in sexp.cpp:20197
fixed.  i remember there was a reason why i never do reverse for loops.
Offically approved by Ebola Virus Man :wtf:
phreakscp - gtalk
phreak317#7583 - discord