From 0bfb03ea68eb745172feccfc0f01b2ad13ff33bb Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Fri, 24 Apr 2015 13:48:32 +0200 Subject: [PATCH] dbus: avoid race-conditions in tests for dum dir availability Florian Weimer dump_dir_accessible_by_uid() is fundamentally insecure because it opens up a classic time-of-check-time-of-use race between this function and and dd_opendir(). Related: #1214745 Signed-off-by: Jakub Filak --- src/dbus/abrt-dbus.c | 225 +++++++++++++++++++++----------------------------- src/lib/problem_api.c | 21 +++-- 2 files changed, 109 insertions(+), 137 deletions(-) diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c index 585fcd7..0f7ac2d 100644 --- a/src/dbus/abrt-dbus.c +++ b/src/dbus/abrt-dbus.c @@ -201,6 +201,69 @@ static void return_InvalidProblemDir_error(GDBusMethodInvocation *invocation, co free(msg); } +enum { + OPEN_FAIL_NO_REPLY = 1 << 0, + OPEN_AUTH_ASK = 1 << 1, + OPEN_AUTH_FAIL = 1 << 2, +}; + +static struct dump_dir *open_dump_directory(GDBusMethodInvocation *invocation, + const gchar *caller, uid_t caller_uid, const char *problem_dir, int dd_flags, int flags) +{ + if (!allowed_problem_dir(problem_dir)) + { + log("UID=%d attempted to access not allowed problem directory '%s'", + caller_uid, problem_dir); + if (!(flags & OPEN_FAIL_NO_REPLY)) + return_InvalidProblemDir_error(invocation, problem_dir); + return NULL; + } + + struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_FD_ONLY); + if (dd == NULL) + { + perror_msg("can't open problem directory '%s'", problem_dir); + if (!(flags & OPEN_FAIL_NO_REPLY)) + return_InvalidProblemDir_error(invocation, problem_dir); + return NULL; + } + + if (!dd_accessible_by_uid(dd, caller_uid)) + { + if (errno == ENOTDIR) + { + log_notice("Requested directory does not exist '%s'", problem_dir); + if (!(flags & OPEN_FAIL_NO_REPLY)) + return_InvalidProblemDir_error(invocation, problem_dir); + dd_close(dd); + return NULL; + } + + if ( !(flags & OPEN_AUTH_ASK) + || polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes) + { + log_notice("not authorized"); + if (!(flags & OPEN_FAIL_NO_REPLY)) + g_dbus_method_invocation_return_dbus_error(invocation, + "org.freedesktop.problems.AuthFailure", + _("Not Authorized")); + dd_close(dd); + return NULL; + } + } + + dd = dd_fdopendir(dd, dd_flags); + if (dd == NULL) + { + log_notice("Can't open the problem '%s' with flags %x0", problem_dir, dd_flags); + if (!(flags & OPEN_FAIL_NO_REPLY)) + g_dbus_method_invocation_return_dbus_error(invocation, + "org.freedesktop.problems.Failure", + _("Can't open the problem")); + } + return dd; +} + /* * Checks element's rights and does not open directory if element is protected. * Checks problem's rights and does not open directory if user hasn't got @@ -237,35 +300,8 @@ static struct dump_dir *open_directory_for_modification_of_element( } } - if (!dump_dir_accessible_by_uid(problem_id, caller_uid)) - { - if (errno == ENOTDIR) - { - log_notice("'%s' is not a valid problem directory", problem_id); - return_InvalidProblemDir_error(invocation, problem_id); - } - else - { - log_notice("UID(%d) is not Authorized to access '%s'", caller_uid, problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.AuthFailure", - _("Not Authorized")); - } - - return NULL; - } - - struct dump_dir *dd = dd_opendir(problem_id, /* flags : */ 0); - if (!dd) - { /* This should not happen because of the access check above */ - log_notice("Can't access the problem '%s' for modification", problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.Failure", - _("Can't access the problem for modification")); - return NULL; - } - - return dd; + return open_dump_directory(invocation, /*caller*/NULL, caller_uid, problem_id, /*Read/Write*/0, + OPEN_AUTH_FAIL); } @@ -421,7 +457,15 @@ static void handle_method_call(GDBusConnection *connection, return; } - int ddstat = dump_dir_stat_for_uid(problem_dir, caller_uid); + struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_FD_ONLY); + if (dd == NULL) + { + perror_msg("can't open problem directory '%s'", problem_dir); + return_InvalidProblemDir_error(invocation, problem_dir); + return; + } + + int ddstat = dd_stat_for_uid(dd, caller_uid); if (ddstat < 0) { if (errno == ENOTDIR) @@ -435,6 +479,7 @@ static void handle_method_call(GDBusConnection *connection, return_InvalidProblemDir_error(invocation, problem_dir); + dd_close(dd); return; } @@ -442,6 +487,7 @@ static void handle_method_call(GDBusConnection *connection, { //caller seems to be in group with access to this dir, so no action needed log_notice("caller has access to the requested directory %s", problem_dir); g_dbus_method_invocation_return_value(invocation, NULL); + dd_close(dd); return; } @@ -452,10 +498,11 @@ static void handle_method_call(GDBusConnection *connection, g_dbus_method_invocation_return_dbus_error(invocation, "org.freedesktop.problems.AuthFailure", _("Not Authorized")); + dd_close(dd); return; } - struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); + dd = dd_fdopendir(dd, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); if (!dd) { return_InvalidProblemDir_error(invocation, problem_dir); @@ -483,37 +530,10 @@ static void handle_method_call(GDBusConnection *connection, g_variant_get_child(parameters, 0, "&s", &problem_dir); log_notice("problem_dir:'%s'", problem_dir); - if (!allowed_problem_dir(problem_dir)) - { - return_InvalidProblemDir_error(invocation, problem_dir); - return; - } - - if (!dump_dir_accessible_by_uid(problem_dir, caller_uid)) - { - if (errno == ENOTDIR) - { - log_notice("Requested directory does not exist '%s'", problem_dir); - return_InvalidProblemDir_error(invocation, problem_dir); - return; - } - - if (polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes) - { - log_notice("not authorized"); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.AuthFailure", - _("Not Authorized")); - return; - } - } - - struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); + struct dump_dir *dd = open_dump_directory(invocation, caller, caller_uid, + problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES , OPEN_AUTH_ASK); if (!dd) - { - return_InvalidProblemDir_error(invocation, problem_dir); return; - } /* Get 2nd param - vector of element names */ GVariant *array = g_variant_get_child_value(parameters, 1); @@ -560,32 +580,10 @@ 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) - { - log_notice("Unauthorized access : '%s'", problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.AuthFailure", - _("Not Authorized")); - return; - } - - struct dump_dir *dd = dd_opendir(problem_id, DD_OPEN_READONLY); - if (dd == NULL) - { - log_notice("Can't access the problem '%s' for reading", problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.Failure", - _("Can't access the problem for reading")); + struct dump_dir *dd = open_dump_directory(invocation, caller, caller_uid, + problem_id, DD_OPEN_READONLY, OPEN_AUTH_ASK); + if (!dd) return; - } problem_data_t *pd = create_problem_data_from_dump_dir(dd); dd_close(dd); @@ -629,12 +627,6 @@ 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); @@ -694,12 +686,6 @@ 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) @@ -732,33 +718,10 @@ 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); + struct dump_dir *dd = open_dump_directory(invocation, caller, caller_uid, + problem_id, DD_OPEN_READONLY, OPEN_AUTH_ASK); if (!dd) - { - log_notice("Can't access the problem '%s'", problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.Failure", - _("Can't access the problem")); - 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) - { - dd_close(dd); - log_notice("Unauthorized access : '%s'", problem_id); - g_dbus_method_invocation_return_dbus_error(invocation, - "org.freedesktop.problems.AuthFailure", - _("Not Authorized")); return; - } int ret = dd_exist(dd, element); dd_close(dd); @@ -793,20 +756,18 @@ static void handle_method_call(GDBusConnection *connection, for (GList *l = problem_dirs; l; l = l->next) { const char *dir_name = (const char*)l->data; - if (!dump_dir_accessible_by_uid(dir_name, caller_uid)) + + struct dump_dir *dd = open_dump_directory(invocation, caller, caller_uid, + dir_name, /*Read/Write*/0, OPEN_FAIL_NO_REPLY | OPEN_AUTH_ASK); + + if (dd) { - if (errno == ENOTDIR) + if (dd_delete(dd) != 0) { - log_notice("Requested directory does not exist '%s'", dir_name); - continue; - } - - if (polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes) - { // if user didn't provide correct credentials, just move to the next dir - continue; + error_msg("Failed to delete problem directory '%s'", dir_name); + dd_close(dd); } } - delete_dump_dir(dir_name); } g_dbus_method_invocation_return_value(invocation, NULL); diff --git a/src/lib/problem_api.c b/src/lib/problem_api.c index 86222cf..96a49fc 100644 --- a/src/lib/problem_api.c +++ b/src/lib/problem_api.c @@ -46,7 +46,17 @@ int for_each_problem_in_dir(const char *path, continue; /* skip "." and ".." */ char *full_name = concat_path_file(path, dent->d_name); - if (caller_uid == -1 || dump_dir_accessible_by_uid(full_name, caller_uid)) + + struct dump_dir *dd = dd_opendir(full_name, DD_OPEN_FD_ONLY + | DD_FAIL_QUIETLY_ENOENT + | DD_FAIL_QUIETLY_EACCES); + if (dd == NULL) + { + VERB2 perror_msg("can't open problem directory '%s'", full_name); + continue; + } + + if (caller_uid == -1 || dd_accessible_by_uid(dd, caller_uid)) { /* Silently ignore *any* errors, not only EACCES. * We saw "lock file is locked by process PID" error @@ -55,14 +65,15 @@ int for_each_problem_in_dir(const char *path, int sv_logmode = logmode; /* Silently ignore errors only in the silent log level. */ logmode = g_verbose == 0 ? 0: sv_logmode; - struct dump_dir *dd = dd_opendir(full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK); + dd = dd_fdopendir(dd, DD_OPEN_READONLY | DD_DONT_WAIT_FOR_LOCK); logmode = sv_logmode; if (dd) - { brk = callback ? callback(dd, arg) : 0; - dd_close(dd); - } } + + if (dd) + dd_close(dd); + free(full_name); if (brk) break; -- 2.1.0