From 427b1c2b9b03c1fe52edb8a129e821a9ea2e0bea Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
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 <jfilak@redhat.com>
---
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 <assert.h>
+
+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