From 6e811d78e2719988ae291181f5b133af32ce62d8 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Thu, 23 Apr 2015 14:46:27 +0200 Subject: [ABRT PATCH] dbus: process only valid sub-directories of the dump location Must have correct rights and must be a direct sub-directory of the dump location. This issue was discovered by Florian Weimer of Red Hat Product Security. Related: #1214451 Signed-off-by: Jakub Filak --- src/dbus/abrt-dbus.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/dbus/abrt-dbus.c b/src/dbus/abrt-dbus.c index 308a9af..7400dff 100644 --- a/src/dbus/abrt-dbus.c +++ b/src/dbus/abrt-dbus.c @@ -132,18 +132,34 @@ static uid_t get_caller_uid(GDBusConnection *connection, GDBusMethodInvocation * return caller_uid; } -static bool allowed_problem_dir(const char *dir_name) +bool allowed_problem_dir(const char *dir_name) { -//HACK HACK HACK! Disabled for now until we fix clients (abrt-gui) to not pass /home/user/.cache/abrt/spool + if (!dir_is_in_dump_location(dir_name)) + { + error_msg("Bad problem directory name '%s', should start with: '%s'", dir_name, g_settings_dump_location); + return false; + } + + /* We cannot test correct permissions yet because we still need to chown + * dump directories before reporting and Chowing changes the file owner to + * the reporter, which causes this test to fail and prevents users from + * getting problem data after reporting it. + * + * Fortunately, libreport has been hardened against hard link and symbolic + * link attacks and refuses to work with such files, so this test isn't + * really necessary, however, we will use it once we get rid of the + * chowning files. + * + * abrt-server refuses to run post-create on directories that have + * incorrect owner (not "root:(abrt|root)"), incorrect permissions (other + * bits are not 0) and are complete (post-create finished). So, there is no + * way to run security sensitive event scripts (post-create) on crafted + * problem directories. + */ #if 0 - unsigned len = strlen(g_settings_dump_location); - - /* If doesn't start with "g_settings_dump_location[/]"... */ - if (strncmp(dir_name, g_settings_dump_location, len) != 0 - || (dir_name[len] != '/' && dir_name[len] != '\0') - /* or contains "/." anywhere (-> might contain ".." component) */ - || strstr(dir_name + len, "/.") - ) { + if (!dir_has_correct_permissions(dir_name)) + { + error_msg("Problem directory '%s' isn't owned by root:abrt or others are not restricted from access", dir_name); return false; } #endif -- 1.8.3.1