From 14afae1a40138bf96aab89fd05f5d395ef06a61a Mon Sep 17 00:00:00 2001 From: Misa Date: Fri, 12 Jun 2020 21:55:41 -0700 Subject: [PATCH] Add bounds checks to script commands that didn't have them Continuing from #280, another potential source of out-of-bounds indexing (and thus, Undefined Behavior badness) comes from script commands. A majority of them don't do any input validation at all, which means the potential for out-of-bounds indexing and segfaulting in custom levels. So it's always good to add bounds checks to them. Interesting note, the only existing command that has bounds checks is the flag() command. That means you can't turn out-of-bounds flags on or off. But there's no bounds checks for ifflag(), or customifflag(), which means you CAN index out-of-bounds with those commands! That's a bit bad to do, so. Also, I decided to add the bounds checks for playef() at the musicclass::playef() level, instead of just the level of the playef() command. I don't know of any other cases outside of the command where musicclass::playef() will index out of bounds, but musicclass is the one containing the indexed vector anyway, I wanted to cover more cases, and it's better to be safe than sorry. --- desktop_version/src/Music.cpp | 4 ++++ desktop_version/src/Script.cpp | 41 +++++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/desktop_version/src/Music.cpp b/desktop_version/src/Music.cpp index aaa23406..25432762 100644 --- a/desktop_version/src/Music.cpp +++ b/desktop_version/src/Music.cpp @@ -391,6 +391,10 @@ void musicclass::changemusicarea(int x, int y) void musicclass::playef(int t) { + if (t < 0 || t >= (int) soundTracks.size()) + { + return; + } int channel; channel = Mix_PlayChannel(-1, soundTracks[t].sound, 0); diff --git a/desktop_version/src/Script.cpp b/desktop_version/src/Script.cpp index 19d740b7..0a6bd660 100644 --- a/desktop_version/src/Script.cpp +++ b/desktop_version/src/Script.cpp @@ -97,12 +97,16 @@ void scriptclass::run() int temprx=ss_toi(words[1])-1; int tempry=ss_toi(words[2])-1; int curlevel=temprx+(ed.maxwidth*(tempry)); - ed.level[curlevel].warpdir=ss_toi(words[3]); + bool inbounds = curlevel >= 0 && curlevel < 400; + if (inbounds) + { + ed.level[curlevel].warpdir=ss_toi(words[3]); + } //If screen warping, then override all that: graphics.backgrounddrawn = false; //Do we update our own room? - if(game.roomx-100==temprx && game.roomy-100==tempry){ + if(inbounds && game.roomx-100==temprx && game.roomy-100==tempry){ map.warpx=false; map.warpy=false; if(ed.level[curlevel].warpdir==0){ map.background = 1; @@ -132,7 +136,8 @@ void scriptclass::run() } if (words[0] == "ifwarp") { - if (ed.level[ss_toi(words[1])-1+(ed.maxwidth*(ss_toi(words[2])-1))].warpdir == ss_toi(words[3])) + int room = ss_toi(words[1])-1+(ed.maxwidth*(ss_toi(words[2])-1)); + if (room >= 0 && room < 400 && ed.level[room].warpdir == ss_toi(words[3])) { load("custom_"+words[4]); position--; @@ -174,7 +179,8 @@ void scriptclass::run() } else if (words[0] == "customifflag") { - if (obj.flags[ss_toi(words[1])]) + int flag = ss_toi(words[1]); + if (flag >= 0 && flag < (int) obj.flags.size() && obj.flags[flag]) { load("custom_"+words[2]); position--; @@ -1284,7 +1290,8 @@ void scriptclass::run() } else if (words[0] == "ifexplored") { - if (map.explored[ss_toi(words[1]) + (20 * ss_toi(words[2]))] == 1) + int room = ss_toi(words[1]) + (20 * ss_toi(words[2])); + if (room >= 0 && room < (int) map.explored.size() && map.explored[room] == 1) { load(words[3]); position--; @@ -1308,7 +1315,8 @@ void scriptclass::run() } else if (words[0] == "ifflag") { - if (obj.flags[ss_toi(words[1])]) + int flag = ss_toi(words[1]); + if (flag >= 0 && flag < (int) obj.flags.size() && obj.flags[flag]) { load(words[2]); position--; @@ -1316,7 +1324,8 @@ void scriptclass::run() } else if (words[0] == "ifcrewlost") { - if (game.crewstats[ss_toi(words[1])]==false) + int crewmate = ss_toi(words[1]); + if (crewmate >= 0 && crewmate < (int) game.crewstats.size() && game.crewstats[crewmate]==false) { load(words[2]); position--; @@ -1340,11 +1349,19 @@ void scriptclass::run() } else if (words[0] == "hidecoordinates") { - map.explored[ss_toi(words[1]) + (20 * ss_toi(words[2]))] = 0; + int room = ss_toi(words[1]) + (20 * ss_toi(words[2])); + if (room >= 0 && room < (int) map.explored.size()) + { + map.explored[room] = 0; + } } else if (words[0] == "showcoordinates") { - map.explored[ss_toi(words[1]) + (20 * ss_toi(words[2]))] = 1; + int room = ss_toi(words[1]) + (20 * ss_toi(words[2])); + if (room >= 0 && room < (int) map.explored.size()) + { + map.explored[room] = 1; + } } else if (words[0] == "hideship") { @@ -1900,7 +1917,11 @@ void scriptclass::run() music.haltdasmusik(); music.playef(3); - obj.collect[ss_toi(words[1])] = true; + int trinket = ss_toi(words[1]); + if (trinket >= 0 && trinket < (int) obj.collect.size()) + { + obj.collect[trinket] = true; + } graphics.textboxremovefast();