[PATCH] Rocks N Diamonds 4.3.2.2 segfaults in LoadLevel_CUSX/GRPX on corrupt level files generated with AFL++

Found a bug in R'n'D? Report it here!

Moderators: Flumminator, Zomis

Post Reply
Quipyowert
Posts: 1
Joined: Thu Aug 18, 2022 2:07 am

[PATCH] Rocks N Diamonds 4.3.2.2 segfaults in LoadLevel_CUSX/GRPX on corrupt level files generated with AFL++

Post by Quipyowert »

Rocks N Diamonds v4.3.2.2 commit 1d7ec871 (artsoft/master) segfaults when loading level files generated with the American Fuzzy Lop++ 4.01a fuzzer starting with an empty level.

The command I used to find these crashing inputs in RnD was

Code: Select all

afl-fuzz -i input -o output ./rocksndiamonds -e 'dump level @@'
The crash in LoadLevel_CUSX is because RnD tries to access element 10 and beyond of

Code: Select all

ei->change_page
which is a ten element array. LoadLevel_GRPX crashes because

Code: Select all

ei->group
is NULL and then tries to dereference it.

I compiled SDL2, SDL_image, SDL_mixer, and SDL_net myself because the SDL2 package in the openSUSE repos was older than the version that introduced SDL_OpenURL.

I tried to debug this issue before, but I kept encountering

Code: Select all

<optimized out>
variables. This time I compiled with -O0 and was able to figure out a simple patch to fix the issue.

I have attached the zip of the levels which crash RnD and the Valgrind output from running Valgrind 3.18.1 on rocksndiamonds like this:

Code: Select all

valgrind ./rocksndiamonds -e 'dump level crashes/id:000001,sig:11,src:000801+000501,time:100941896,execs:2536255,op:splice,rep:8'
. LoadLevel_GRPX-1d7ec871.txt contains the output of that command and LoadLevel_CUSX1d7ec871.txt contains the output of running Valgrind on Rocksndiamonds running

Code: Select all

dump level
on the id:000000,sig:11,src:000370+000640,time:250246985,execs:5048867,op:splice,rep:4 file.

Environment:
openSUSE 15.4 in Windows Subsystem for Linux running in Windows 10
Rocks n Diamonds 4.3.2.2
AFL++ 4.01a
Valgrind 3.18.1
SDL 2.23.1
SDL_image 2.5.2
SDL_mixer 2.5.2
SDL_net 2.1.0


Here is a patch which seems to fix the crashes:

Code: Select all

diff --git a/src/files.c b/src/files.c
index 8889f810..5633786d 100644
--- a/src/files.c
+++ b/src/files.c
@@ -3356,6 +3356,8 @@ static int LoadLevel_CUSX(File *file, int chunk_size, struct LevelInfo *level)
   while (!checkEndOfFile(file))
   {
     struct ElementChangeInfo *change = &ei->change_page[xx_current_change_page];
+    if (xx_current_change_page >= ei->num_change_pages)
+      break;
 
     xx_change = *change;	// copy change data into temporary buffer
 
@@ -3383,6 +3385,8 @@ static int LoadLevel_GRPX(File *file, int chunk_size, struct LevelInfo *level)
   int real_chunk_size = 2;
   struct ElementInfo *ei = &element_info[element];
   struct ElementGroupInfo *group = ei->group;
+  if (!group)
+    return 0;
 
   xx_ei = *ei;		// copy element data into temporary buffer
   xx_group = *group;	// copy group data into temporary buffer
Actually now that I think about it, LoadLevel_GRPX should probably return -1 when the group is NULL, because 0 might mean it successfully read 0 bytes or that the function failed; -1 is more clearly an error.

Originally reported here: GitHub issue #7

crashinglevels.zip
LoadLevel_CUSX-1d7ec871.txt
LoadLevel_GRPX-1d7ec871.txt
User avatar
Holger
Site Admin
Posts: 4073
Joined: Fri Jun 18, 2004 4:13 pm
Location: Germany
Contact:

Re: [PATCH] Rocks N Diamonds 4.3.2.2 segfaults in LoadLevel_CUSX/GRPX on corrupt level files generated with AFL++

Post by Holger »

Hello Quipyowert, welcome to the forum! :-)

Thanks a lot for fuzzing R'n'D, for publishing your results (and methods! and test environment!) and for providing me with a patch that works just fine! :D

For the first part of the patch, I have done the check one line earlier, to prevent calculating an invalid pointer (even if it is not used then).

For the second part of the patch (besides the small change of using the expression "group != NULL" instead of "!group"), I also implemented your suggestion to use "return -1" instead of "return 0" here, to be able to better check the results and output a different warning message.

Finally, I also added stopping to read the remaining chunks of a level file after encountering a chunk that is either broken (returns "-1" for whatever parse error) or has an invalid chunk size (as it is likely that garbage will follow in these cases).

You can have a look at the latest version of the code with your patches (and the additional improvements just mentioned) here:

git@build.artsoft.org:~/rocksndiamonds.git

Be warned: Although this repository will always contain the latest development version of the R'n'D code, it is not stable. That is, I regularly do forced pushes to this repository (usually as a result of rebasing the code)! Therefore, always use "git.artsoft.org" if you want stable or released code. The GitHub repository does not always contain the latest version of the code, but I try to update it with every public release.

Regarding the GitHub repository: Just by chance I have seen (or did you tell me about it in the past? not sure) that you created four pull requests with more fixes and changes at the GitHub repo -- why did they never send me a notification for this?! :shock: Probably it is my own fault, and I have to check my notification settings, though. :?

Thank you again for the debugging work you have put into my game, and for the patch which will improve it!

Update:
I compiled SDL2, SDL_image, SDL_mixer, and SDL_net myself because the SDL2 package in the openSUSE repos was older than the version that introduced SDL_OpenURL.
Nearly forgot to mention that the latest version of the code at "git@build.artsoft.org" also contains a fix for systems with older versions of the SDL libraries (which are shipped with quite some current distributions). It checks during compilation if the installed version of the SDL library contains "SDL_OpenURL()", and only uses this function if SDL is new enough (and else just prints a warning instead of opening a URL at runtime).
Post Reply