From b7f8bd20b7fb5b72f003ae3fa647c1d75f4218b7 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Thu, 23 Apr 2015 14:40:18 +0200 Subject: [ABRT 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. Signed-off-by: Jakub Filak --- src/daemon/abrt-server.c | 42 ++++++------------------ src/include/libabrt.h | 4 +++ src/lib/hooklib.c | 56 +++++++++++++++++++++++++++++++ tests/Makefile.am | 3 +- tests/hooklib.at | 85 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/testsuite.at | 1 + 6 files changed, 158 insertions(+), 33 deletions(-) create mode 100644 tests/hooklib.at diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index 4d486d4..1030461 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -76,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) { @@ -100,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)) + { + error_msg("Problem directory '%s' isn't owned by root:abrt or others are not restricted from access", dump_dir_name); + return 400; /* */ + } if (!dump_dir_accessible_by_uid(dump_dir_name, client_uid)) { if (errno == ENOTDIR) @@ -154,26 +145,13 @@ 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)) + { + error_msg("Problem directory '%s' isn't owned by root:abrt or others are not restricted from access", dirname); + return 400; /* */ + } if (g_settings_privatereports) { - struct stat statbuf; - if (lstat(dirname, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode)) - { - error_msg("Path '%s' isn't directory", dirname); - return 404; /* Not Found */ - } - /* Get ABRT's group gid */ - struct group *gr = getgrnam("abrt"); - if (!gr) - { - error_msg("Group 'abrt' does not exist"); - return 500; - } - if (statbuf.st_uid != 0 || !(statbuf.st_gid == 0 || statbuf.st_gid == gr->gr_gid) || statbuf.st_mode & 07) - { - error_msg("Problem directory '%s' isn't owned by root:abrt or others are not restricted from access", dirname); - return 403; - } struct dump_dir *dd = dd_opendir(dirname, DD_OPEN_READONLY); const bool complete = dd && problem_dump_dir_is_complete(dd); dd_close(dd); diff --git a/src/include/libabrt.h b/src/include/libabrt.h index 0320c5b..5bf2397 100644 --- a/src/include/libabrt.h +++ b/src/include/libabrt.h @@ -47,6 +47,10 @@ 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); +#define dir_has_correct_permissions abrt_dir_has_correct_permissions +bool dir_has_correct_permissions(const char *dir_name); #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 fb7750d..4b20025 100644 --- a/src/lib/hooklib.c +++ b/src/lib/hooklib.c @@ -427,3 +427,59 @@ char* problem_data_save(problem_data_t *pd) log_info("problem id: '%s'", problem_id); return problem_id; } + +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) +{ + if (g_settings_privatereports) + { + 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 gid */ + struct group *gr = getgrnam("abrt"); + if (!gr) + { + error_msg("Group 'abrt' does not exist"); + return false; + } + if (statbuf.st_uid != 0 || !(statbuf.st_gid == 0 || statbuf.st_gid == gr->gr_gid) || statbuf.st_mode & 07) + return false; + } + return true; +} 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]) -- 1.8.3.1