Visleafs values read from .bsp files are not sanitized and have potential to break R_MarkSurfaces()

[Diordany]

This is closely related to #92 and #130, regarding a loop in R_MarkSurfaces().

First of all, @ericwa pointed out how misleading the naming is when dealing with the leafs array (#130), because after observing through the debugger, you can notice that numleafs, which is used to allocate leafs, gets replaced with the unsanitized value of visleafs here. I'll leave the naming issue for what it is, because it was already discussed in #130.

The issue I want to address is that the loop discussed in #92 and #130 can be easily exploited by corrupting the worldmodel in a .bsp file, and then loading it in QuakeSpasm. This is due to the lack of sanitization. To prove and demonstrate this, I cobbled together a small Python script that patches the visleafs data of the world model in a .bsp file. After loading the corrupt file, the loop uses the corrupt visleafs data for iteration.

I've only investigated the iteration of the loop in question, so I'm not aware of any other effects of the corrupted visleafs data.

[ericwa]

There are many, e.g.:

• out->firstmarksurface = loadmodel->marksurfaces + (unsigned short)LittleShort(in->firstmarksurface); //johnfitz -- unsigned short

•  p = LittleLong(in->visofs);
   if (p == -1)
       out->compressed_vis = NULL;
   else
   out->compressed_vis = loadmodel->visdata + p;
  

• loadmodel->textures[miptex];

[andrey-budko]

Out of curiosity, I checked ezQuake, and all of the examples mentioned above seem to be explicitly validated there.

https://github.com/QW-Group/ezquake-source/blob/888798fbda2875bc45e94a81e23b22be2a627288/src/r_brushmodel_load.c#L1397

https://github.com/QW-Group/ezquake-source/blob/888798fbda2875bc45e94a81e23b22be2a627288/src/r_brushmodel_load.c#L1020

https://github.com/QW-Group/ezquake-source/blob/888798fbda2875bc45e94a81e23b22be2a627288/src/r_brushmodel_load.c#L1408

[Diordany]

Thanks @ericwa and @andrey-budko. I'll look into ezQuake later.

[Diordany]

I haven't looked into firstmarksurface, visofs and miptex (or any other BSP data) yet, but it seems like the loop in R_MarkLeaves() from ezQuake breaks in a similar way with the corrupt visleafs loaded.