Blob Blame Raw
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