From 0bfb03ea68eb745172feccfc0f01b2ad13ff33bb Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
Date: Fri, 24 Apr 2015 13:48:32 +0200
Subject: [PATCH] dbus: avoid race-conditions in tests for dum dir availability
Florian Weimer <fweimer@redhat.com>
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 <jfilak@redhat.com>
---
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