From 7814554e0827ece778ca88fd90832bd4d05520b1 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Fri, 24 Apr 2015 13:48:32 +0200 Subject: [ABRT 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++------- src/lib/problem_api.c | 15 ++++++++++-- 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c index 7400dff..9e1844a 100644 --- a/src/dbus/abrt-dbus.c +++ b/src/dbus/abrt-dbus.c @@ -245,7 +245,15 @@ static struct dump_dir *open_directory_for_modification_of_element( } } - if (!dump_dir_accessible_by_uid(problem_id, caller_uid)) + int dir_fd = dd_openfd(problem_id); + if (dir_fd < 0) + { + perror_msg("can't open problem directory '%s'", problem_id); + return_InvalidProblemDir_error(invocation, problem_id); + return NULL; + } + + if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid)) { if (errno == ENOTDIR) { @@ -260,10 +268,11 @@ static struct dump_dir *open_directory_for_modification_of_element( _("Not Authorized")); } + close(dir_fd); return NULL; } - struct dump_dir *dd = dd_opendir(problem_id, /* flags : */ 0); + struct dump_dir *dd = dd_fdopendir(dir_fd, 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); @@ -429,7 +438,15 @@ static void handle_method_call(GDBusConnection *connection, return; } - int ddstat = dump_dir_stat_for_uid(problem_dir, caller_uid); + int dir_fd = dd_openfd(problem_dir); + if (dir_fd < 0) + { + perror_msg("can't open problem directory '%s'", problem_dir); + return_InvalidProblemDir_error(invocation, problem_dir); + return; + } + + int ddstat = fdump_dir_stat_for_uid(dir_fd, caller_uid); if (ddstat < 0) { if (errno == ENOTDIR) @@ -443,6 +460,7 @@ static void handle_method_call(GDBusConnection *connection, return_InvalidProblemDir_error(invocation, problem_dir); + close(dir_fd); return; } @@ -450,6 +468,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); + close(dir_fd); return; } @@ -460,10 +479,11 @@ static void handle_method_call(GDBusConnection *connection, g_dbus_method_invocation_return_dbus_error(invocation, "org.freedesktop.problems.AuthFailure", _("Not Authorized")); + close(dir_fd); return; } - struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); + struct dump_dir *dd = dd_fdopendir(dir_fd, problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); if (!dd) { return_InvalidProblemDir_error(invocation, problem_dir); @@ -497,12 +517,21 @@ static void handle_method_call(GDBusConnection *connection, return; } - if (!dump_dir_accessible_by_uid(problem_dir, caller_uid)) + int dir_fd = dd_openfd(problem_dir); + if (dir_fd < 0) + { + perror_msg("can't open problem directory '%s'", problem_dir); + return_InvalidProblemDir_error(invocation, problem_dir); + return; + } + + if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid)) { if (errno == ENOTDIR) { log_notice("Requested directory does not exist '%s'", problem_dir); return_InvalidProblemDir_error(invocation, problem_dir); + close(dir_fd); return; } @@ -512,11 +541,12 @@ static void handle_method_call(GDBusConnection *connection, g_dbus_method_invocation_return_dbus_error(invocation, "org.freedesktop.problems.AuthFailure", _("Not Authorized")); + close(dir_fd); return; } } - struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); + struct dump_dir *dd = dd_fdopendir(dir_fd, problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES); if (!dd) { return_InvalidProblemDir_error(invocation, problem_dir); @@ -677,20 +707,40 @@ 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)) + + int dir_fd = dd_openfd(dir_name); + if (dir_fd < 0) + { + perror_msg("can't open problem directory '%s'", dir_name); + return_InvalidProblemDir_error(invocation, dir_name); + return; + } + + if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid)) { if (errno == ENOTDIR) { log_notice("Requested directory does not exist '%s'", dir_name); + close(dir_fd); 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 + close(dir_fd); continue; } } - delete_dump_dir(dir_name); + + struct dump_dir *dd = dd_fdopendir(dir_fd, dir_name, /*flags:*/ 0); + if (dd) + { + if (dd_delete(dd) != 0) + { + error_msg("Failed to delete problem directory '%s'", dir_name); + dd_close(dd); + } + } } g_dbus_method_invocation_return_value(invocation, NULL); diff --git a/src/lib/problem_api.c b/src/lib/problem_api.c index c2b4b1c..b343882 100644 --- a/src/lib/problem_api.c +++ b/src/lib/problem_api.c @@ -46,7 +46,15 @@ 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)) + + int dir_fd = dd_openfd(full_name); + if (dir_fd < 0) + { + VERB2 perror_msg("can't open problem directory '%s'", full_name); + continue; + } + + if (caller_uid == -1 || fdump_dir_accessible_by_uid(dir_fd, caller_uid)) { /* Silently ignore *any* errors, not only EACCES. * We saw "lock file is locked by process PID" error @@ -54,7 +62,7 @@ int for_each_problem_in_dir(const char *path, */ int sv_logmode = logmode; logmode = 0; - struct dump_dir *dd = dd_opendir(full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK); + struct dump_dir *dd = dd_fdopendir(dir_fd, full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK); logmode = sv_logmode; if (dd) { @@ -62,6 +70,9 @@ int for_each_problem_in_dir(const char *path, dd_close(dd); } } + else + close(dir_fd); + free(full_name); if (brk) break; -- 1.8.3.1