From c5cd9d25473e2fea29f7fa609c81ae821e50f118 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Thu, 23 Apr 2015 14:46:27 +0200 Subject: [PATCH] dbus: process only valid sub-directories of the dump location Must have correct rights and must be a direct sub-directory of the dump location. This issue was discovered by Florian Weimer of Red Hat Product Security. Related: #1214451 Signed-off-by: Jakub Filak --- src/dbus/abrt-dbus.c | 46 ++++++++++++++++++++++++++++++++++------------ src/lib/problem_api.c | 7 +++++++ 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c index 4eeff41..585fcd7 100644 --- a/src/dbus/abrt-dbus.c +++ b/src/dbus/abrt-dbus.c @@ -141,21 +141,20 @@ static uid_t get_caller_uid(GDBusConnection *connection, GDBusMethodInvocation * return caller_uid; } -static bool allowed_problem_dir(const char *dir_name) +bool allowed_problem_dir(const char *dir_name) { -//HACK HACK HACK! Disabled for now until we fix clients (abrt-gui) to not pass /home/user/.cache/abrt/spool -#if 0 - unsigned len = strlen(g_settings_dump_location); - - /* If doesn't start with "g_settings_dump_location[/]"... */ - if (strncmp(dir_name, g_settings_dump_location, len) != 0 - || (dir_name[len] != '/' && dir_name[len] != '\0') - /* or contains "/." anywhere (-> might contain ".." component) */ - || strstr(dir_name + len, "/.") - ) { + if (!dir_is_in_dump_location(dir_name)) + { + error_msg("Bad problem directory name '%s', should start with: '%s'", dir_name, g_settings_dump_location); return false; } -#endif + + if (!dir_has_correct_permissions(dir_name, DD_PERM_DAEMONS)) + { + error_msg("Problem directory '%s' has invalid owner, groop or mode", dir_name); + return false; + } + return true; } @@ -561,6 +560,12 @@ static void handle_method_call(GDBusConnection *connection, g_variant_get(parameters, "(&s)", &problem_id); + if (!allowed_problem_dir(problem_id)) + { + return_InvalidProblemDir_error(invocation, problem_id); + return; + } + int ddstat = dump_dir_stat_for_uid(problem_id, caller_uid); if ((ddstat & DD_STAT_ACCESSIBLE_BY_UID) == 0 && polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes) @@ -624,6 +629,12 @@ static void handle_method_call(GDBusConnection *connection, g_variant_get(parameters, "(&s&s&s)", &problem_id, &element, &value); + if (!allowed_problem_dir(problem_id)) + { + return_InvalidProblemDir_error(invocation, problem_id); + return; + } + if (element == NULL || element[0] == '\0' || strlen(element) > 64) { log_notice("'%s' is not a valid element name of '%s'", element, problem_id); @@ -683,6 +694,12 @@ static void handle_method_call(GDBusConnection *connection, g_variant_get(parameters, "(&s&s)", &problem_id, &element); + if (!allowed_problem_dir(problem_id)) + { + return_InvalidProblemDir_error(invocation, problem_id); + return; + } + struct dump_dir *dd = open_directory_for_modification_of_element( invocation, caller_uid, problem_id, element); if (!dd) @@ -715,6 +732,11 @@ static void handle_method_call(GDBusConnection *connection, g_variant_get(parameters, "(&s&s)", &problem_id, &element); + if (!allowed_problem_dir(problem_id)) + { + return_InvalidProblemDir_error(invocation, problem_id); + return; + } struct dump_dir *dd = dd_opendir(problem_id, DD_OPEN_READONLY); if (!dd) diff --git a/src/lib/problem_api.c b/src/lib/problem_api.c index 07707db..86222cf 100644 --- a/src/lib/problem_api.c +++ b/src/lib/problem_api.c @@ -76,6 +76,13 @@ int for_each_problem_in_dir(const char *path, static int add_dirname_to_GList(struct dump_dir *dd, void *arg) { + if (!dir_has_correct_permissions(dd->dd_dirname, DD_PERM_DAEMONS)) + { + log("Ignoring '%s': invalid owner, group or mode", dd->dd_dirname); + /*Do not break*/ + return 0; + } + GList **list = arg; *list = g_list_prepend(*list, xstrdup(dd->dd_dirname)); return 0; -- 2.1.0