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