Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebased: Various security and functionality related bugfixes (multiple DoS, memory leaks) #200

Merged
merged 9 commits into from Jul 22, 2017
only allow dynamic UnregisterHandler for external handlers, thereby f…
…ixing 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

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.
  • Loading branch information
mgerstner committed Jul 18, 2017
commit bb80e9c7a798f035768260ebdadffb6eb0786178
32 changes: 29 additions & 3 deletions main.c
Expand Up @@ -91,6 +91,12 @@ int tcmur_register_handler(struct tcmur_handler *handler)
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;
Expand All @@ -103,6 +109,16 @@ bool tcmur_unregister_handler(struct tcmur_handler *handler)
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))
Expand Down Expand Up @@ -315,7 +331,7 @@ on_handler_appeared(GDBusConnection *connection,

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"));
Expand All @@ -340,7 +356,7 @@ on_handler_vanished(GDBusConnection *connection,
g_variant_new("(bs)", FALSE, reason));
g_free(reason);
}
tcmur_unregister_handler(handler);
tcmur_unregister_dbus_handler(handler);
dbus_unexport_handler(handler);
}

Expand All @@ -366,6 +382,8 @@ on_register_handler(TCMUService1HandlerManager1 *interface,
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,
Expand Down Expand Up @@ -394,8 +412,16 @@ on_unregister_handler(TCMUService1HandlerManager1 *interface,
"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);
Expand Down
9 changes: 9 additions & 0 deletions tcmu-runner.h
Expand Up @@ -115,6 +115,15 @@ struct tcmur_handler {
int (*lock)(struct tcmu_device *dev);
int (*unlock)(struct tcmu_device *dev);
int (*has_lock)(struct tcmu_device *dev);

/*
* 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;
};

/*
Expand Down