Bug 1472332 - tcmu-runner: Various security and functionality related bugfixes (multiple DoS, memory leaks)
tcmu-runner: Various security and functionality related bugfixes (multiple Do...
Product: Red Hat Gluster Storage
Classification: Red Hat
Component: tcmu-runner (Show other bugs)
Unspecified Unspecified
unspecified Severity high
: ---
: RHGS 3.3.1
Assigned To: Prasanna Kumar Kalever
Sweta Anandpara
: ZStream
Depends On:
Blocks: 1475687
  Show dependency treegraph
Reported: 2017-07-18 09:17 EDT by Prasanna Kumar Kalever
Modified: 2017-11-28 22:33 EST (History)
5 users (show)

See Also:
Fixed In Version: tcmu-runner-1.2.0-16.el7rhgs
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2017-11-28 22:33:56 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2017:3277 normal SHIPPED_LIVE Moderate: tcmu-runner security update 2017-11-29 03:32:51 EST

  None (edit)
Description Prasanna Kumar Kalever 2017-07-18 09:17:46 EDT
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!
Comment 12 Sweta Anandpara 2017-11-13 04:31:19 EST
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.
Comment 15 errata-xmlrpc 2017-11-28 22:33:56 EST
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.


Note You need to log in before you can comment on or make changes to this bug.