From f15b6e0f53ceb5ba450e93e406272ffe8a398cbd Mon Sep 17 00:00:00 2001 From: Jakub Filak 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 --- 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