From 427b1c2b9b03c1fe52edb8a129e821a9ea2e0bea Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Thu, 23 Apr 2015 14:40:18 +0200 Subject: [PATCH] lib: add functions validating dump dir Move the code from abrt-server to shared library and fix the condition validating dump dir's path. As of now, abrt is allowed to process only direct sub-directories of the dump locations with appropriate mode and owner and group. Signed-off-by: Jakub Filak --- src/daemon/abrt-server.c | 36 ++++++++++++-------- src/include/libabrt.h | 9 +++++ src/lib/hooklib.c | 77 +++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 3 +- tests/hooklib.at | 85 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/testsuite.at | 1 + 6 files changed, 196 insertions(+), 15 deletions(-) create mode 100644 tests/hooklib.at diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index 287c510..8c48509 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -15,6 +15,7 @@ with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include "problem_api.h" #include "libabrt.h" /* Maximal length of backtrace. */ @@ -75,20 +76,6 @@ static unsigned total_bytes_read = 0; static uid_t client_uid = (uid_t)-1L; -static bool dir_is_in_dump_location(const char *dump_dir_name) -{ - unsigned len = strlen(g_settings_dump_location); - - if (strncmp(dump_dir_name, g_settings_dump_location, len) == 0 - && dump_dir_name[len] == '/' - /* must not contain "/." anywhere (IOW: disallow ".." component) */ - && !strstr(dump_dir_name + len, "/.") - ) { - return 1; - } - return 0; -} - /* Remove dump dir */ static int delete_path(const char *dump_dir_name) { @@ -99,6 +86,11 @@ static int delete_path(const char *dump_dir_name) error_msg("Bad problem directory name '%s', should start with: '%s'", dump_dir_name, g_settings_dump_location); return 400; /* Bad Request */ } + if (!dir_has_correct_permissions(dump_dir_name, DD_PERM_DAEMONS)) + { + error_msg("Problem directory '%s' has wrong owner or group", dump_dir_name); + return 400; /* */ + } if (!dump_dir_accessible_by_uid(dump_dir_name, client_uid)) { if (errno == ENOTDIR) @@ -153,6 +145,22 @@ static int run_post_create(const char *dirname) error_msg("Bad problem directory name '%s', should start with: '%s'", dirname, g_settings_dump_location); return 400; /* Bad Request */ } + if (!dir_has_correct_permissions(dirname, DD_PERM_EVENTS)) + { + error_msg("Problem directory '%s' has wrong owner or group", dirname); + return 400; /* */ + } + /* Check completness */ + { + struct dump_dir *dd = dd_opendir(dirname, DD_OPEN_READONLY); + const bool complete = dd && problem_dump_dir_is_complete(dd); + dd_close(dd); + if (complete) + { + error_msg("Problem directory '%s' has already been processed", dirname); + return 403; + } + } if (!dump_dir_accessible_by_uid(dirname, client_uid)) { if (errno == ENOTDIR) diff --git a/src/include/libabrt.h b/src/include/libabrt.h index abfbc97..9de222d 100644 --- a/src/include/libabrt.h +++ b/src/include/libabrt.h @@ -47,6 +47,15 @@ char *run_unstrip_n(const char *dump_dir_name, unsigned timeout_sec); #define get_backtrace abrt_get_backtrace char *get_backtrace(const char *dump_dir_name, unsigned timeout_sec, const char *debuginfo_dirs); +#define dir_is_in_dump_location abrt_dir_is_in_dump_location +bool dir_is_in_dump_location(const char *dir_name); + +enum { + DD_PERM_EVENTS = 1 << 0, + DD_PERM_DAEMONS = 1 << 1, +}; +#define dir_has_correct_permissions abrt_dir_has_correct_permissions +bool dir_has_correct_permissions(const char *dir_name, int flags); #define g_settings_nMaxCrashReportsSize abrt_g_settings_nMaxCrashReportsSize extern unsigned int g_settings_nMaxCrashReportsSize; diff --git a/src/lib/hooklib.c b/src/lib/hooklib.c index 2be4e80..c94cadf 100644 --- a/src/lib/hooklib.c +++ b/src/lib/hooklib.c @@ -475,3 +475,80 @@ int signal_is_fatal(int signal_no, const char **name) return signame != NULL; } + +bool dir_is_in_dump_location(const char *dir_name) +{ + unsigned len = strlen(g_settings_dump_location); + + /* The path must start with "g_settings_dump_location" */ + if (strncmp(dir_name, g_settings_dump_location, len) != 0) + { + log_debug("Bad parent directory: '%s' not in '%s'", g_settings_dump_location, dir_name); + return false; + } + + /* and must be a sub-directory of the g_settings_dump_location dir */ + const char *base_name = dir_name + len; + while (*base_name && *base_name == '/') + ++base_name; + + if (*(base_name - 1) != '/' || !str_is_correct_filename(base_name)) + { + log_debug("Invalid dump directory name: '%s'", base_name); + return false; + } + + /* and we are sure it is a directory */ + struct stat sb; + if (lstat(dir_name, &sb) < 0) + { + VERB2 perror_msg("stat('%s')", dir_name); + return errno== ENOENT; + } + + return S_ISDIR(sb.st_mode); +} + +bool dir_has_correct_permissions(const char *dir_name, int flags) +{ + struct stat statbuf; + if (lstat(dir_name, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode)) + { + error_msg("Path '%s' isn't directory", dir_name); + return false; + } + + /* Get ABRT's group id */ + struct group *gr = getgrnam("abrt"); + if (!gr) + { + error_msg("The group 'abrt' does not exist"); + return false; + } + + /* The group must be root or abrt. */ + const bool correct_group = statbuf.st_gid == 0 + || statbuf.st_gid == gr->gr_gid; + + /* Require owner 'root' and group 'abrt' for the event shell scripts. + * Because the shell scripts are vulnerable to hard link and symbolic link + * attacks. + */ + const bool events = statbuf.st_uid == 0 + && correct_group + && (statbuf.st_mode & S_IWGRP) == 0 + && (statbuf.st_mode & S_IWOTH) == 0; + + if ((flags & DD_PERM_EVENTS)) + return events; + + /* Be less restrictive and allow the daemos (abrtd, abrt-dbus) to work with + * dump directories having group 'root' or 'abrt'. + * + * 1. Chowning of dump directories switches the ownership to 'user':'abrt' + * 2. We want to allow users to delete their problems + * 3. We want to allow users to modify their data + * 4. The daemons are hardened agains hard link and symbolic link issues. + */ + return correct_group; +} diff --git a/tests/Makefile.am b/tests/Makefile.am index 5ef08a0..416f579 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -29,7 +29,8 @@ TESTSUITE_AT = \ testsuite.at \ pyhook.at \ koops-parser.at \ - ignored_problems.at + ignored_problems.at \ + hooklib.at EXTRA_DIST += $(TESTSUITE_AT) TESTSUITE = $(srcdir)/testsuite diff --git a/tests/hooklib.at b/tests/hooklib.at new file mode 100644 index 0000000..70631c6 --- /dev/null +++ b/tests/hooklib.at @@ -0,0 +1,85 @@ +# -*- Autotest -*- + +AT_BANNER([hooklib]) + +AT_TESTFUN([dir_is_in_dump_location], +[[ +#include "libabrt.h" +#include + +void test(char *name, bool expected) +{ + if (dir_is_in_dump_location(name) != expected) + { + fprintf(stderr, "Bad: %s", name); + abort(); + } + + free(name); +} + +int main(void) +{ + g_verbose = 3; + load_abrt_conf(); + + g_verbose = 3; + + char *name; + + assert(dir_is_in_dump_location("/") == false); + + asprintf(&name, "%s", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s..evil", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s///", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/.", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s///.", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/./", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/.///", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/..", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s///..", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/../", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/..///", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/good/../../../evil", g_settings_dump_location); + test(name, false); + + asprintf(&name, "%s/good..still", g_settings_dump_location); + test(name, true); + + asprintf(&name, "%s/good.new", g_settings_dump_location); + test(name, true); + + asprintf(&name, "%s/.meta", g_settings_dump_location); + test(name, true); + + asprintf(&name, "%s/..data", g_settings_dump_location); + test(name, true); + + return 0; +} +]]) diff --git a/tests/testsuite.at b/tests/testsuite.at index b8f363d..765de2a 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -4,3 +4,4 @@ m4_include([koops-parser.at]) m4_include([pyhook.at]) m4_include([ignored_problems.at]) +m4_include([hooklib.at]) -- 2.1.0