Hello community, here is the log from the commit of package tcmu-runner for openSUSE:Factory checked in at 2017-07-21 22:50:24 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Comparing /work/SRC/openSUSE:Factory/tcmu-runner (Old) and /work/SRC/openSUSE:Factory/.tcmu-runner.new (New) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Package is "tcmu-runner" Fri Jul 21 22:50:24 2017 rev:3 rq:511671 version:1.2.0 Changes: -------- --- /work/SRC/openSUSE:Factory/tcmu-runner/tcmu-runner.changes 2017-05-17 10:54:22.842603393 +0200 +++ /work/SRC/openSUSE:Factory/.tcmu-runner.new/tcmu-runner.changes 2017-07-21 22:50:27.479767357 +0200 @@ -1,0 +2,19 @@ +Thu Jul 20 01:53:01 UTC 2017 - [email protected] + +- One more update from Security Audit, adding one more patch: + - removed-all-check_config-callback-implementations-to-avoid-security-issues + (bsc#1049491) + +------------------------------------------------------------------- +Thu Jul 20 01:18:42 UTC 2017 - [email protected] + +- Updates based on Security Audit, adding 4 patches: + * glfs-discard-glfs_check_config (bsc#1049485) + * fixed-local-dos-when-unregisterhandler-was-called-for-a-not-existing-handler + (bsc#1049488) + * only-allow-dynamic-unregisterhandler-for-external-handlers-thereby-fixing-dos + (bsc#1049489) + * fixed-a-number-of-memory-leaks-with-deregistering-of-dbus-handlers + (bsc#1049490) + +------------------------------------------------------------------- New: ---- fixed-a-number-of-memory-leaks-with-deregistering-of-dbus-handlers fixed-local-dos-when-unregisterhandler-was-called-for-a-not-existing-handler glfs-discard-glfs_check_config only-allow-dynamic-unregisterhandler-for-external-handlers-thereby-fixing-dos removed-all-check_config-callback-implementations-to-avoid-security-issues ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Other differences: ------------------ ++++++ tcmu-runner.spec ++++++ --- /var/tmp/diff_new_pack.0SUu8b/_old 2017-07-21 22:50:28.359643239 +0200 +++ /var/tmp/diff_new_pack.0SUu8b/_new 2017-07-21 22:50:28.363642675 +0200 @@ -35,6 +35,11 @@ Url: https://github.com/agrover/%{name} Source: %{name}-%{version}.tar.gz Patch1: %{name}-handler_file-add-libtcmu.patch +Patch2: glfs-discard-glfs_check_config +Patch3: fixed-local-dos-when-unregisterhandler-was-called-for-a-not-existing-handler +Patch4: only-allow-dynamic-unregisterhandler-for-external-handlers-thereby-fixing-dos +Patch5: fixed-a-number-of-memory-leaks-with-deregistering-of-dbus-handlers +Patch6: removed-all-check_config-callback-implementations-to-avoid-security-issues BuildRoot: %{_tmppath}/%{name}-%{version}-build BuildRequires: cmake BuildRequires: glib2-devel @@ -123,6 +128,11 @@ %prep %setup %patch1 -p1 +%patch2 -p1 +%patch3 -p1 +%patch4 -p1 +%patch5 -p1 +%patch6 -p1 %build CMAKE_OPTIONS="\ ++++++ fixed-a-number-of-memory-leaks-with-deregistering-of-dbus-handlers ++++++ From: Matthias Gerstner <[email protected]> Date: Fri, 14 Jul 2017 16:17:09 +0200 Subject: fixed a number of memory leaks with (de)registering of dbus handlers Git-commit: 7a78eda52d973d3edc06fea84ad874678d6055f0 References: bsc#1049490 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 o Example: dbus-send --system --print-reply --dest=org.kernel.TCMUService1 /org/kernel/TCMUService1/HandlerManager1 org.kernel.TCMUService1.HandlerManager1.RegisterHandler string:0memory string:stuff These bugs can all lead to a kind of local DoS, wasting memory in the tcmu-runner daemon, which can be triggered by any local user with access to the system bus. Acked-by: Lee Duncan <[email protected]> --- main.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) --- a/main.c +++ b/main.c @@ -102,6 +102,14 @@ bool tcmur_unregister_handler(struct tcm return false; } +static void free_dbus_handler(struct tcmur_handler *handler) +{ + g_free((char*)handler->opaque); + g_free((char*)handler->subtype); + g_free((char*)handler->cfg_desc); + g_free(handler); +} + static bool tcmur_unregister_dbus_handler(struct tcmur_handler *handler) { bool ret = false; @@ -109,6 +117,10 @@ static bool tcmur_unregister_dbus_handle ret = tcmur_unregister_handler(handler); + if (ret == true) { + free_dbus_handler(handler); + } + return ret; } @@ -562,8 +574,8 @@ on_handler_vanished(GDBusConnection *con g_variant_new("(bs)", FALSE, reason)); g_free(reason); } - tcmur_unregister_dbus_handler(handler); dbus_unexport_handler(handler); + tcmur_unregister_dbus_handler(handler); } static gboolean @@ -598,8 +610,15 @@ on_register_handler(TCMUService1HandlerM on_handler_vanished, handler, NULL); + if (info->watcher_id == 0) { + // probably an invalid name, roll back and report an error + free_dbus_handler(handler); + + g_dbus_method_invocation_return_value(invocation, + g_variant_new("(bs)", FALSE, + "failed to watch for DBus handler name")); + } g_free(bus_name); - handler->opaque = info; return TRUE; } @@ -626,11 +645,9 @@ on_unregister_handler(TCMUService1Handle } dbus_unexport_handler(handler); + g_bus_unwatch_name(info->watcher_id); tcmur_unregister_dbus_handler(handler); - g_bus_unwatch_name(info->watcher_id); - g_free(info); - g_free(handler); g_dbus_method_invocation_return_value(invocation, g_variant_new("(bs)", TRUE, "succeeded")); return TRUE; ++++++ fixed-local-dos-when-unregisterhandler-was-called-for-a-not-existing-handler ++++++ From: Matthias Gerstner <[email protected]> Date: Fri, 14 Jul 2017 14:23:05 +0200 Subject: fixed local DoS when UnregisterHandler was called for a not existing handler Git-commit: e2d953050766ac538615a811c64b34358614edce References: bsc#1049488 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 Acked-by: Lee Duncan <[email protected]> --- main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/main.c +++ b/main.c @@ -592,7 +592,7 @@ on_unregister_handler(TCMUService1Handle gpointer user_data) { struct tcmur_handler *handler = find_handler_by_subtype(subtype); - struct dbus_info *info = handler->opaque; + struct dbus_info *info = handler ? handler->opaque : NULL; if (!handler) { g_dbus_method_invocation_return_value(invocation, ++++++ glfs-discard-glfs_check_config ++++++ From: Prasanna Kumar Kalever <[email protected]> Date: Sun, 21 May 2017 15:14:05 +0530 Subject: glfs: discard glfs_check_config Git-commit: 61bd03e600d2abf309173e9186f4d465bb1b7157 References: bsc#1049485 Signed-off-by: Prasanna Kumar Kalever <[email protected]> Acked-by: Lee Duncan <[email protected]> --- glfs.c | 54 ------------------------------------------------------ 1 file changed, 54 deletions(-) --- a/glfs.c +++ b/glfs.c @@ -403,58 +403,6 @@ static char* tcmu_get_path( struct tcmu_ return config; } - -static bool glfs_check_config(const char *cfgstring, char **reason) -{ - char *path; - glfs_t *fs = NULL; - glfs_fd_t *gfd = NULL; - gluster_server *hosts; /* gluster server defination */ - bool result = true; - - path = strchr(cfgstring, '/'); - if (!path) { - if (asprintf(reason, "No path found") == -1) - *reason = NULL; - result = false; - goto done; - } - path += 1; /* get past '/' */ - - fs = tcmu_create_glfs_object(path, &hosts); - if (!fs) { - tcmu_err("tcmu_create_glfs_object failed\n"); - goto done; - } - - gfd = glfs_open(fs, hosts->path, ALLOWED_BSOFLAGS); - if (!gfd) { - if (asprintf(reason, "glfs_open failed: %m") == -1) - *reason = NULL; - result = false; - goto unref; - } - - if (glfs_access(fs, hosts->path, R_OK|W_OK) == -1) { - if (asprintf(reason, "glfs_access file not present, or not writable") == -1) - *reason = NULL; - result = false; - goto unref; - } - - goto done; - -unref: - gluster_cache_refresh(fs, path); - -done: - if (gfd) - glfs_close(gfd); - gluster_free_server(hosts); - - return result; -} - static int tcmu_glfs_open(struct tcmu_device *dev) { struct glfs_state *gfsp; @@ -575,8 +523,6 @@ struct tcmur_handler glfs_handler = { .subtype = "glfs", .cfg_desc = glfs_cfg_desc, - .check_config = glfs_check_config, - .open = tcmu_glfs_open, .close = tcmu_glfs_close, .read = tcmu_glfs_read, ++++++ only-allow-dynamic-unregisterhandler-for-external-handlers-thereby-fixing-dos ++++++ From: Matthias Gerstner <[email protected]> Date: Fri, 14 Jul 2017 15:11:54 +0200 Subject: only allow dynamic UnregisterHandler for external handlers, thereby fixing DoS Git-commit: bb80e9c7a798f035768260ebdadffb6eb0786178 References: bsc#1049489 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 we use a newly introduced boolean in struct tcmur_handler for keeping track of external handlers. As suggested by mikechristie adjusting the public data structure is acceptable. Acked-by: Lee Duncan <[email protected]> --- main.c | 32 +++++++++++++++++++++++++++++--- tcmu-runner.h | 9 +++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) --- a/main.c +++ b/main.c @@ -84,6 +84,12 @@ int tcmur_register_handler(struct tcmur_ return 0; } +static int tcmur_register_dbus_handler(struct tcmur_handler *handler) +{ + assert(handler->_is_dbus_handler == true); + return tcmur_register_handler(handler); +} + bool tcmur_unregister_handler(struct tcmur_handler *handler) { int i; @@ -96,6 +102,16 @@ bool tcmur_unregister_handler(struct tcm return false; } +static bool tcmur_unregister_dbus_handler(struct tcmur_handler *handler) +{ + bool ret = false; + assert(handler->_is_dbus_handler == true); + + ret = tcmur_unregister_handler(handler); + + return ret; +} + static int is_handler(const struct dirent *dirent) { if (strncmp(dirent->d_name, "handler_", 8)) @@ -521,7 +537,7 @@ on_handler_appeared(GDBusConnection *con if (info->register_invocation) { info->connection = connection; - tcmur_register_handler(handler); + tcmur_register_dbus_handler(handler); dbus_export_handler(handler, G_CALLBACK(on_dbus_check_config)); g_dbus_method_invocation_return_value(info->register_invocation, g_variant_new("(bs)", TRUE, "succeeded")); @@ -546,7 +562,7 @@ on_handler_vanished(GDBusConnection *con g_variant_new("(bs)", FALSE, reason)); g_free(reason); } - tcmur_unregister_handler(handler); + tcmur_unregister_dbus_handler(handler); dbus_unexport_handler(handler); } @@ -572,6 +588,8 @@ on_register_handler(TCMUService1HandlerM handler->handle_cmd = dbus_handler_handle_cmd; info = g_new0(struct dbus_info, 1); + handler->opaque = info; + handler->_is_dbus_handler = 1; info->register_invocation = invocation; info->watcher_id = g_bus_watch_name(G_BUS_TYPE_SYSTEM, bus_name, @@ -600,8 +618,16 @@ on_unregister_handler(TCMUService1Handle "unknown subtype")); return TRUE; } + else if (handler->_is_dbus_handler != 1) { + g_dbus_method_invocation_return_value(invocation, + g_variant_new("(bs)", FALSE, + "cannot unregister internal handler")); + return TRUE; + } + dbus_unexport_handler(handler); - tcmur_unregister_handler(handler); + tcmur_unregister_dbus_handler(handler); + g_bus_unwatch_name(info->watcher_id); g_free(info); g_free(handler); --- a/tcmu-runner.h +++ b/tcmu-runner.h @@ -72,6 +72,15 @@ struct tcmur_handler { ssize_t (*write)(struct tcmu_device *, struct iovec *, size_t, off_t); ssize_t (*read)(struct tcmu_device *, struct iovec *, size_t, off_t); int (*flush)(struct tcmu_device *); + + /* + * internal field, don't touch this + * + * indicates to tcmu-runner whether this is an internal handler loaded + * via dlopen or an external handler registered via dbus. In the + * latter case opaque will point to a struct dbus_info. + */ + bool _is_dbus_handler; }; /* ++++++ removed-all-check_config-callback-implementations-to-avoid-security-issues ++++++ From: Matthias Gerstner <[email protected]> Date: Mon, 17 Jul 2017 11:35:25 +0200 Subject: removed all check_config callback implementations to avoid security issues Git-commit: 8cf8208775022301adaa59c240bb7f93742d1329 References: bsc#1049491 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. Acked-by: Lee Duncan <[email protected]> --- consumer.c | 7 ------ file_example.c | 31 ---------------------------- file_optical.c | 57 ----------------------------------------------------- qcow.c | 23 --------------------- tcmu-synthesizer.c | 11 ---------- 5 files changed, 129 deletions(-) --- a/consumer.c +++ b/consumer.c @@ -54,11 +54,6 @@ static int set_medium_error(uint8_t *sen return tcmu_set_sense_data(sense, MEDIUM_ERROR, ASC_READ_ERROR, NULL); } -static bool foo_check_config(const char *cfgstring, char **reason) -{ - return true; -} - static int foo_open(struct tcmu_device *dev) { /* open the backing file */ @@ -137,8 +132,6 @@ static struct tcmulib_handler foo_handle .subtype = "foo", .cfg_desc = "a description goes here", - .check_config = foo_check_config, - .added = foo_open, .removed = foo_close, }; --- a/file_example.c +++ b/file_example.c @@ -143,35 +143,6 @@ file_handler_destroy(struct file_handler } #endif /* ASYNC_FILE_HANDLER */ -static bool file_check_config(const char *cfgstring, char **reason) -{ - char *path; - int fd; - - path = strchr(cfgstring, '/'); - if (!path) { - if (asprintf(reason, "No path found") == -1) - *reason = NULL; - return false; - } - path += 1; /* get past '/' */ - - if (access(path, W_OK) != -1) - return true; /* File exists and is writable */ - - /* We also support creating the file, so see if we can create it */ - fd = creat(path, S_IRUSR | S_IWUSR); - if (fd == -1) { - if (asprintf(reason, "Could not create file") == -1) - *reason = NULL; - return false; - } - - unlink(path); - - return true; -} - static int file_open(struct tcmu_device *dev) { struct file_state *state; @@ -401,8 +372,6 @@ static const char file_cfg_desc[] = static struct tcmur_handler file_handler = { .cfg_desc = file_cfg_desc, - .check_config = file_check_config, - .open = file_open, .close = file_close, #ifdef ASYNC_FILE_HANDLER --- a/file_optical.c +++ b/file_optical.c @@ -176,61 +176,6 @@ static void fbo_handler_destroy(struct f pthread_mutex_destroy(&h->mtx); } -static bool fbo_check_config(const char *cfgstring, char **reason) -{ - char *options; - char *path; - int fd; - - tcmu_dbg("check: cfgstring %s\n", cfgstring); - options = strchr(cfgstring, '/'); - if (!options) { - if (asprintf(reason, "Invalid cfgstring") == -1) - *reason = NULL; - return false; - } - options += 1; /* get past '/' */ - while (options[0] != '/') { - if (strncasecmp(options, "ro/", 3)) { - if (asprintf(reason, "Unknown option %s\n", - options) == -1) - *reason = NULL; - return false; - } - - options = strchr(options, '/'); - if (!options) { - if (asprintf(reason, "Invalid cfgstring") == -1) - *reason = NULL; - return false; - } - options += 1; - } - - path = options; - if (!path) { - if (asprintf(reason, "No path found") == -1) - *reason = NULL; - return false; - } - - if (access(path, R_OK) != -1) - return true; /* File exists */ - - /* We also support creating the file, so see if we can create it */ - /* NB: If we're creating it, then we'll need write permission */ - fd = creat(path, S_IRUSR | S_IWUSR); - if (fd == -1) { - if (asprintf(reason, "Could not create file") == -1) - *reason = NULL; - return false; - } - - unlink(path); - - return true; -} - /* Note: this is called per lun, not per mapping */ static int fbo_open(struct tcmu_device *dev) { @@ -1764,8 +1709,6 @@ static const char fbo_cfg_desc[] = static struct tcmur_handler fbo_handler = { .cfg_desc = fbo_cfg_desc, - .check_config = fbo_check_config, - .open = fbo_open, .close = fbo_close, .name = "File-backed optical Handler", --- a/qcow.c +++ b/qcow.c @@ -1389,27 +1389,6 @@ static struct bdev_ops raw_ops = { /* TCMU QCOW Handler */ -static bool qcow_check_config(const char *cfgstring, char **reason) -{ - char *path; - - path = strchr(cfgstring, '/'); - if (!path) { - if (asprintf(reason, "No path found") == -1) - *reason = NULL; - return false; - } - path += 1; /* get past '/' */ - - if (access(path, R_OK|W_OK) == -1) { - if (asprintf(reason, "File not present, or not writable") == -1) - *reason = NULL; - return false; - } - - return true; /* File exists and is writable */ -} - static int qcow_open(struct tcmu_device *dev) { struct bdev *bdev; @@ -1574,8 +1553,6 @@ static struct tcmur_handler qcow_handler .subtype = "qcow", .cfg_desc = qcow_cfg_desc, - .check_config = qcow_check_config, - .open = qcow_open, .close = qcow_close, .handle_cmd = qcow_handle_cmd, --- a/tcmu-synthesizer.c +++ b/tcmu-synthesizer.c @@ -35,16 +35,6 @@ typedef struct { int watcher_id; } syn_dev_t; -static bool syn_check_config(const char *cfgstring, char **reason) -{ - tcmu_dbg("syn check config\n"); - if (strcmp(cfgstring, "syn/null")) { - asprintf(reason, "invalid option"); - return false; - } - return true; -} - static int syn_handle_cmd(struct tcmu_device *dev, uint8_t *cdb, struct iovec *iovec, size_t iov_cnt, uint8_t *sense) @@ -147,7 +137,6 @@ struct tcmulib_handler syn_handler = { .cfg_desc = "valid options:\n" "null: a nop storage where R/W requests are completed " "immediately, like the null_blk device.", - .check_config = syn_check_config, .added = syn_added, .removed = syn_removed, };
