Red Hat Bugzilla – Bug 1472332
tcmu-runner: Various security and functionality related bugfixes (multiple DoS, memory leaks)
Last modified: 2017-11-28 22:33:56 EST
Description of problem:
Various security and functionality related fixes
1. use glib signal handler for gracefully exiting the main loop upon SIGINT
This allows a more sensible checking for memory leaks with valgrind, the previous exit() left a lot of memory unreleased which shows up as false positives in valgrind.
2. fixed local DoS when UnregisterHandler was called for a not existing handler
Any user with DBUS access could cause a SEGFAULT in tcmu-runner by
running something like this:
dbus-send --system --print-reply --dest=org.kernel.TCMUService1 /org/kernel/TCMUService1/HandlerManager1 org.kernel.TCMUService1.HandlerManager1.UnregisterHandler string:123
3. only allow dynamic UnregisterHandler for external handlers, thereby fixing DoS
Trying to unregister an internal handler ended up in a SEGFAULT, because the tcmur_handler->opaque was NULL.
Way to reproduce:
dbus-send --system --print-reply --dest=org.kernel.TCMUService1 /org/kernel/TCMUService1/HandlerManager1 org.kernel.TCMUService1.HandlerManager1.UnregisterHandler string:qcow
4. Avoid putting 2 MB char array on the stack for reading config file.
It's probably not been the best idea to use the stack for this. Might break in embedded environments with smaller stack sizes like:
ulimit -s 2048
-> SEGFAULT, out of stack
Also it opens the door for vulnerabilites like stack clashing to jump over the stack guard page.
Also it confuses valgrind in its default configuration, giving us this
Warning: client switching stacks? SP change: 0x1fff000080 --> 0x1ffee00060
to suppress, use: --max-stackframe=2097184 or greater
Thus use the heap for this
5. free internal handlers to fix a static memory leak when exiting
after this valgrind shows no more "definitely lost" bytes on simple start/exit of tcmu-runner.
6. fixed a number of memory leaks with (de)registering of dbus handlers
A number of dynamic memory leaks that could be triggered by any user
with access to DBus have been fixed:
- deregistering a previously registered dbus handler failed to free the
dynamically allocated handler data
- trying to register a dbus handler that doesn't exist failed to free
the dynamically allocated handler data
- trying to register a dbus handler with an invalid name (e.g. starting
with a number like "0memory" caused the g_bus_watch_name() call to fail,
the callback never came in, no response was ever sent to the requestor
dbus-send --system --print-reply --dest=org.kernel.TCMUService1 /org/kernel/TCMUService1/HandlerManager1 org.kernel.TCMUService1.HandlerManager1.RegisterHandler string:0memory string:stuff
7. removed libtcmu_log.h from public header
8. removed all check_config callback implementations to avoid security issues
see github issue #194
qcow.c contained an information leak, could test for existance of any
file in the system
file_example.c and file_optical.c allow also to test for existance of any file, plus to temporarily create empty new files anywhere in the file system. This also involves a race condition, if a file didn't exist in the first place, but would be created in-between by some other process, then the file would be deleted by the check_config implementation.
9. Fix compiler warnings!
Please refer comment 3, 4 and 5 of bug https://bugzilla.redhat.com/show_bug.cgi?id=1494511.
It is the same patch that has gone in for both the bugs. Moving this bug to verified for RHGS 3.3.1.
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.
For information on the advisory, and where to find the updated
files, follow the link below.
If the solution does not work for you, open a new bug report.