Fix various security vulnerabilites found by libFuzzer, part 2 #6418
Conversation
at least, issue also a corresponding PR against master. Note also that 7.6 branch will receive little maintenance at that point |
|
wow thanks @MaxKellermann. This will spawn us to have another 7.6..x release with this security fix. Great find. |
as far as I can tell, the vulnerability is if you read from corrupted / hostile shapefiles. It isn't something that is triggered by remote requests, so the severity is much less important |
The canonical macro is _WIN32. WIN32 usually exists as well, but is a non-standard macro. See https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-160
Fixes various -Wunused warnings.
Fixes various -Wunused warnings.
With some compile-time options, pointObj has less than 4 fields, so this patch also fixes the build with those options.
Fixes build breakage.
Traverse the string only once. Also, this removes code which triggered -Wsign-compare.
The data in panParts is never checked. The only check was "numpoints<=0", but that is not enough. Three very bad things can happen: - arbitrary huge values, leading to allocations of up to two billion elements (INT_MAX), bypassing the 50 million limit which was previously put on "nPoints" - overflowing the "pabyRec" buffer in the memcpy() call - integer overflow in the malloc() call, writing past the allocated buffer The latter is probably enough for remote code execution. Vulnerability found with libFuzzer.
After freeing the "line" field, we need to clear it, or else it will be freed again in msFreeShape(). In two code paths, the "numlines" field was not cleared, which could lead to a use-after-free bug in msFreeShape(), which in turn could either crash or lead to another double-free bug in msFreeShape(). Vulnerability found with libFuzzer.
With a crafted shapefile, it was possible to put msShapefileWhichShapes() into an extremely long loop, calling msSHPReadBounds() over and over, even if all of those calls fail. This patch adds error checking, and if an error occurs, msShapefileWhichShapes() gives up, because after an I/O error, there is no reasonable chance that anything will ever work properly. Vulnerability found by libFuzzer.
Correct. |
|
Unfortunately shapefiles are still widely used by MapServer users, it's still an important fix and I think should warrant a security release. |
Agree. These are severe vulnerabilities. Just because we can't currently imagine a way to exploit a server remotely doesn't mean that somebody with more creativity can't find a way. Better be too careful than sorry. The coding style in this project is very fragile towards such vulnerabilities. For example, the code checking buffer sizes looks fragile and obscure; stuff like: Lines 770 to 773 in 1c14821 So many magic numbers! Where do they come from? Near that code fragment, I don't see any buffer size verification. Maybe there is verification somewhere else, but it is not obvious at all that this code is correct; quite contrary, you made it extremely difficult for a human to verify this piece of code. Much of the code deals with 32 bit signed integers for sizes. That's why you always need an additional What about integer overflows? Lines 617 to 620 in 1c14821 This will easily overflow, leading to a SfRealloc() call with a negative integer. Oh yes, SfRealloc() takes a signed integer, WTF. What happens with this negative integer when it's casted back to size_t for realloc()?
Maybe you could argue that this and that limit in the code prevents this integer overflow. Maybe, maybe somebody finds a way around this. My point is: the MapServer code is horribly fragile, it's the anti-style for making secure code. Everywhere I look, I see potential for vulnerabilities. Of course, it's plain C, and getting error code paths right in plain C is utterly difficult. Just look at the many memory leaks, or worse, the double-free bugs I found (leaks are just DoS, but double-free often leads to remote code execution). C is simply the wrong tool for the job. You could easily improve the code quality by switching all sources to a C++ compiler and use RAII to manage allocations. (Some argue that C++ isn't safe enough and everybody should be using Rust, but I'm a C++ fanboy, and C++ is certainly a great improvement over plain C, and switching is mostly trivial.) |
|
@MaxKellermann, your candor is appreciated. Can you elaborate on "mostly trivial"? |
More like commit 098eb92 (by @rouault). That is: rename The whole Second step (optional): small syntactic stuff like Third step (the interesting part): switch to safe C++ data structures, use RAII to free resources automatically, wrap legacy code / thirdparty C libraries in safe C++ wrappers. This eliminates all of the ugly cleanup code like: Lines 1354 to 1364 in 1c14821 ... which is very fragile, because it's easy to forget something, and if you add a new struct field, you have to adjust all error code paths. Fourth step: migrate the error handling to C++ exceptions. |
… size (CID 1503560)
Yet another buffer overflow found by libFuzzer.
|
Added a backport of 8dc4104 and a fix for #6421 (comment) |
This PR starts with a few build breakage fixes which occur when almost all optional features are disabled at compile time.
The last 3 commits fix security vulnerabilities I found with libFuzzer, one of which qualifies for remote code execution, the worst kind of vulnerability.
A crash report from Google Play of an open source app using a copy of MapServer code (https://github.com/XCSoar/XCSoar/) contained a crash due to one of these vulnerabilities, so I investigated and turned on libFuzzer again, which quickly found more vulnerabilities.
I used this program (specific to my app's internal APIs wrapping our my copy of the MapServer code) to run libFuzzer: https://github.com/XCSoar/XCSoar/blob/master/fuzzer/src/FuzzTopographyFile.cpp
This is similar to my previous PR earlier this year: #6319
Back then, @rouault asked me to change the PR to the "main" branch; do you want me to do the same here? (My way of managing my open source apps is that I always merge bug fixes to the "stable" branch first, and then merge all "stable" bug fixes into the "main" branch.)