From f15b6e0f53ceb5ba450e93e406272ffe8a398cbd Mon Sep 17 00:00:00 2001

From: Jakub Filak <jfilak@redhat.com>

Date: Mon, 1 Jun 2015 12:03:55 +0200

Subject: [PATCH] hooks: use root for owner of all dump directories

This patch has two goals:

* avoid hard and symbolic link attacks (race conditions)

* keep security sensitive data private (in the near future, a problem

  directories will contain elements accessible by privileged

  users while the directory itself will be accessible by

  non-privileged users (dmesg, journald, /var/log/messages)

Related: #1212868 (CVE-2015-1870), #1212861 (CVE-2015-1869)

Signed-off-by: Jakub Filak <jfilak@redhat.com>

---

 src/daemon/abrt-server.c     |  2 +-

 src/dbus/abrt-dbus.c         | 11 ++++-------

 src/hooks/abrt-hook-ccpp.c   | 18 +++++++++---------

 src/lib/hooklib.c            |  2 +-

 src/plugins/abrt-dump-xorg.c | 15 ++++-----------

 src/plugins/oops-utils.c     | 15 ++++-----------

 6 files changed, 23 insertions(+), 40 deletions(-)

diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c

index 90339ab..d7556e2 100644

--- a/src/daemon/abrt-server.c

+++ b/src/daemon/abrt-server.c

@@ -391,7 +391,7 @@ static int create_problem_dir(GHashTable *problem_info, unsigned pid)

     /* No need to check the path length, as all variables used are limited,

      * and dd_create() fails if the path is too long.

      */

-    struct dump_dir *dd = dd_create(path, client_uid, DEFAULT_DUMP_DIR_MODE);

+    struct dump_dir *dd = dd_create(path, /*fs owner*/0, DEFAULT_DUMP_DIR_MODE);

     if (!dd)

     {

         error_msg_and_die("Error creating problem directory '%s'", path);

diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c

index 62f331b..44778a2 100644

--- a/src/dbus/abrt-dbus.c

+++ b/src/dbus/abrt-dbus.c

@@ -506,13 +506,10 @@ static void handle_method_call(GDBusConnection *connection,

             return;

         }

-        if (ddstat & DD_STAT_OWNED_BY_UID)

-        {   //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;

-        }

+        /* It might happen that we will do chowing even if the UID is alreay fs

+         * owner, but DD_STAT_OWNED_BY_UID no longer denotes fs owner and this

+         * method has to ensure file system ownership for the uid.

+         */

         if ((ddstat & DD_STAT_ACCESSIBLE_BY_UID) == 0 &&

                 polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes)

diff --git a/src/hooks/abrt-hook-ccpp.c b/src/hooks/abrt-hook-ccpp.c

index 59fcfce..06dd670 100644

--- a/src/hooks/abrt-hook-ccpp.c

+++ b/src/hooks/abrt-hook-ccpp.c

@@ -707,14 +707,17 @@ int main(int argc, char** argv)

         return create_user_core(user_core_fd, pid, ulimit_c);

     }

-    /* use fsuid instead of uid, so we don't expose any sensitive

-     * information of suided app in /var/tmp/abrt

+    /* If you don't want to have fs owner as root then:

      *

-     * dd_create_skeleton() creates a new directory and leaves ownership to

-     * the current user, hence, we have to call dd_reset_ownership() after the

-     * directory is populated.

+     * - use fsuid instead of uid for fs owner, so we don't expose any

+     *   sensitive information of suided app in /var/(tmp|spool)/abrt

+     *

+     * - use dd_create_skeleton() and dd_reset_ownership(), when you finish

+     *   creating the new dump directory, to prevent the real owner to write to

+     *   the directory until the hook is done (avoid race conditions and defend

+     *   hard and symbolic link attacs)

      */

-    dd = dd_create_skeleton(path, fsuid, DEFAULT_DUMP_DIR_MODE, /*no flags*/0);

+    dd = dd_create(path, /*fs owner*/0, DEFAULT_DUMP_DIR_MODE);

     if (dd)

     {

         char *rootdir = get_rootdir(pid);

@@ -886,9 +889,6 @@ int main(int argc, char** argv)

         if (tid > 0 && setting_CreateCoreBacktrace)

             create_core_backtrace(tid, executable, signal_no, dd);

-        /* And finally set the right uid and gid */

-        dd_reset_ownership(dd);

-

         /* We close dumpdir before we start catering for crash storm case.

          * Otherwise, delete_dump_dir's from other concurrent

          * CCpp's won't be able to delete our dump (their delete_dump_dir

diff --git a/src/lib/hooklib.c b/src/lib/hooklib.c

index 0a8d703..39a6ef4 100644

--- a/src/lib/hooklib.c

+++ b/src/lib/hooklib.c

@@ -410,7 +410,7 @@ char* problem_data_save(problem_data_t *pd)

 {

     load_abrt_conf();

-    struct dump_dir *dd = create_dump_dir_from_problem_data(pd, g_settings_dump_location);

+    struct dump_dir *dd = create_dump_dir_from_problem_data_ext(pd, g_settings_dump_location, /*fs owner*/0);

     char *problem_id = NULL;

     if (dd)

diff --git a/src/plugins/abrt-dump-xorg.c b/src/plugins/abrt-dump-xorg.c

index 3500629..477ec9c 100644

--- a/src/plugins/abrt-dump-xorg.c

+++ b/src/plugins/abrt-dump-xorg.c

@@ -73,15 +73,6 @@ static void save_bt_to_dump_dir(const char *bt, const char *exe, const char *rea

 {

     time_t t = time(NULL);

     const char *iso_date = iso_date_string(&t);

-    /* dump should be readable by all if we're run with -x */

-    uid_t my_euid = (uid_t)-1L;

-    mode_t mode = DEFAULT_DUMP_DIR_MODE | S_IROTH;

-    /* and readable only for the owner otherwise */

-    if (!(g_opts & OPT_x))

-    {

-        mode = DEFAULT_DUMP_DIR_MODE;

-        my_euid = geteuid();

-    }

     pid_t my_pid = getpid();

@@ -89,10 +80,10 @@ static void save_bt_to_dump_dir(const char *bt, const char *exe, const char *rea

     sprintf(base, "xorg-%s-%lu-%u", iso_date, (long)my_pid, g_bt_count);

     char *path = concat_path_file(debug_dumps_dir, base);

-    struct dump_dir *dd = dd_create(path, /*uid:*/ my_euid, mode);

+    struct dump_dir *dd = dd_create(path, /*fs owner*/0, DEFAULT_DUMP_DIR_MODE);

     if (dd)

     {

-        dd_create_basic_files(dd, /*uid:*/ my_euid, NULL);

+        dd_create_basic_files(dd, /*no uid*/(uid_t)-1L, NULL);

         dd_save_text(dd, FILENAME_ABRT_VERSION, VERSION);

         dd_save_text(dd, FILENAME_ANALYZER, "xorg");

         dd_save_text(dd, FILENAME_TYPE, "xorg");

@@ -111,6 +102,8 @@ static void save_bt_to_dump_dir(const char *bt, const char *exe, const char *rea

                 exe = "/usr/bin/Xorg";

         }

         dd_save_text(dd, FILENAME_EXECUTABLE, exe);

+        if (!(g_opts & OPT_x))

+            dd_set_no_owner(dd);

         dd_close(dd);

         notify_new_path(path);

     }

diff --git a/src/plugins/oops-utils.c b/src/plugins/oops-utils.c

index ea6c639..bb6a79c 100644

--- a/src/plugins/oops-utils.c

+++ b/src/plugins/oops-utils.c

@@ -92,15 +92,6 @@ unsigned abrt_oops_create_dump_dirs(GList *oops_list, const char *dump_location,

     time_t t = time(NULL);

     const char *iso_date = iso_date_string(&t);

-    /* dump should be readable by all if we're run with -x */

-    uid_t my_euid = (uid_t)-1L;

-    mode_t mode = DEFAULT_DUMP_DIR_MODE | S_IROTH;

-    /* and readable only for the owner otherwise */

-    if (!(flags & ABRT_OOPS_WORLD_READABLE))

-    {

-        mode = DEFAULT_DUMP_DIR_MODE;

-        my_euid = geteuid();

-    }

     pid_t my_pid = getpid();

     unsigned idx = 0;

@@ -111,10 +102,10 @@ unsigned abrt_oops_create_dump_dirs(GList *oops_list, const char *dump_location,

         sprintf(base, "oops-%s-%lu-%lu", iso_date, (long)my_pid, (long)idx);

         char *path = concat_path_file(dump_location, base);

-        struct dump_dir *dd = dd_create(path, /*uid:*/ my_euid, mode);

+        struct dump_dir *dd = dd_create(path, /*fs owner*/0, DEFAULT_DUMP_DIR_MODE);

         if (dd)

         {

-            dd_create_basic_files(dd, /*uid:*/ my_euid, NULL);

+            dd_create_basic_files(dd, /*no uid*/(uid_t)-1L, NULL);

             abrt_oops_save_data_in_dump_dir(dd, (char*)g_list_nth_data(oops_list, idx++), proc_modules);

             dd_save_text(dd, FILENAME_ABRT_VERSION, VERSION);

             dd_save_text(dd, FILENAME_ANALYZER, "Kerneloops");

@@ -127,6 +118,8 @@ unsigned abrt_oops_create_dump_dirs(GList *oops_list, const char *dump_location,

                 dd_save_text(dd, "fips_enabled", fips_enabled);

             if (suspend_stats)

                 dd_save_text(dd, "suspend_stats", suspend_stats);

+            if ((flags & ABRT_OOPS_WORLD_READABLE))

+                dd_set_no_owner(dd);

             dd_close(dd);

             notify_new_path(path);

         }

-- 

2.1.0