Skip to content
Permalink
Browse files Browse the repository at this point in the history
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 <jfilak@redhat.com>
  • Loading branch information
Jakub Filak committed May 4, 2015
1 parent c796c76 commit b7f8bd2
Show file tree
Hide file tree
Showing 6 changed files with 158 additions and 33 deletions.
42 changes: 10 additions & 32 deletions src/daemon/abrt-server.c
Expand Up @@ -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)
{
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions src/include/libabrt.h
Expand Up @@ -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;
Expand Down
56 changes: 56 additions & 0 deletions src/lib/hooklib.c
Expand Up @@ -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;
}
3 changes: 2 additions & 1 deletion tests/Makefile.am
Expand Up @@ -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
Expand Down
85 changes: 85 additions & 0 deletions 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;
}
]])
1 change: 1 addition & 0 deletions tests/testsuite.at
Expand Up @@ -4,3 +4,4 @@
m4_include([koops-parser.at])
m4_include([pyhook.at])
m4_include([ignored_problems.at])
m4_include([hooklib.at])

0 comments on commit b7f8bd2

Please sign in to comment.