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