Skip to content
Permalink
Browse files Browse the repository at this point in the history
dbus: avoid race-conditions in tests for dum dir availability
Florian Weimer <fweimer@redhat.com>

    dump_dir_accessible_by_uid() is fundamentally insecure because it
    opens up a classic time-of-check-time-of-use race between this
    function and and dd_opendir().

Related: #1214745

Signed-off-by: Jakub Filak <jfilak@redhat.com>
  • Loading branch information
Jakub Filak committed May 4, 2015
1 parent 6e811d7 commit 7814554
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 10 deletions.
66 changes: 58 additions & 8 deletions src/dbus/abrt-dbus.c
Expand Up @@ -245,7 +245,15 @@ static struct dump_dir *open_directory_for_modification_of_element(
}
}

if (!dump_dir_accessible_by_uid(problem_id, caller_uid))
int dir_fd = dd_openfd(problem_id);
if (dir_fd < 0)
{
perror_msg("can't open problem directory '%s'", problem_id);
return_InvalidProblemDir_error(invocation, problem_id);
return NULL;
}

if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid))
{
if (errno == ENOTDIR)
{
Expand All @@ -260,10 +268,11 @@ static struct dump_dir *open_directory_for_modification_of_element(
_("Not Authorized"));
}

close(dir_fd);
return NULL;
}

struct dump_dir *dd = dd_opendir(problem_id, /* flags : */ 0);
struct dump_dir *dd = dd_fdopendir(dir_fd, problem_id, /* flags : */ 0);
if (!dd)
{ /* This should not happen because of the access check above */
log_notice("Can't access the problem '%s' for modification", problem_id);
Expand Down Expand Up @@ -429,7 +438,15 @@ static void handle_method_call(GDBusConnection *connection,
return;
}

int ddstat = dump_dir_stat_for_uid(problem_dir, caller_uid);
int dir_fd = dd_openfd(problem_dir);
if (dir_fd < 0)
{
perror_msg("can't open problem directory '%s'", problem_dir);
return_InvalidProblemDir_error(invocation, problem_dir);
return;
}

int ddstat = fdump_dir_stat_for_uid(dir_fd, caller_uid);
if (ddstat < 0)
{
if (errno == ENOTDIR)
Expand All @@ -443,13 +460,15 @@ static void handle_method_call(GDBusConnection *connection,

return_InvalidProblemDir_error(invocation, problem_dir);

close(dir_fd);
return;
}

if (ddstat & DD_STAT_OWNED_BY_UID)
{ //caller seems to be in group with access to this dir, so no action needed
log_notice("caller has access to the requested directory %s", problem_dir);
g_dbus_method_invocation_return_value(invocation, NULL);
close(dir_fd);
return;
}

Expand All @@ -460,10 +479,11 @@ static void handle_method_call(GDBusConnection *connection,
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.AuthFailure",
_("Not Authorized"));
close(dir_fd);
return;
}

struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES);
struct dump_dir *dd = dd_fdopendir(dir_fd, problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES);
if (!dd)
{
return_InvalidProblemDir_error(invocation, problem_dir);
Expand Down Expand Up @@ -497,12 +517,21 @@ static void handle_method_call(GDBusConnection *connection,
return;
}

if (!dump_dir_accessible_by_uid(problem_dir, caller_uid))
int dir_fd = dd_openfd(problem_dir);
if (dir_fd < 0)
{
perror_msg("can't open problem directory '%s'", problem_dir);
return_InvalidProblemDir_error(invocation, problem_dir);
return;
}

if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid))
{
if (errno == ENOTDIR)
{
log_notice("Requested directory does not exist '%s'", problem_dir);
return_InvalidProblemDir_error(invocation, problem_dir);
close(dir_fd);
return;
}

Expand All @@ -512,11 +541,12 @@ static void handle_method_call(GDBusConnection *connection,
g_dbus_method_invocation_return_dbus_error(invocation,
"org.freedesktop.problems.AuthFailure",
_("Not Authorized"));
close(dir_fd);
return;
}
}

struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES);
struct dump_dir *dd = dd_fdopendir(dir_fd, problem_dir, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES);
if (!dd)
{
return_InvalidProblemDir_error(invocation, problem_dir);
Expand Down Expand Up @@ -677,20 +707,40 @@ static void handle_method_call(GDBusConnection *connection,
for (GList *l = problem_dirs; l; l = l->next)
{
const char *dir_name = (const char*)l->data;
if (!dump_dir_accessible_by_uid(dir_name, caller_uid))

int dir_fd = dd_openfd(dir_name);
if (dir_fd < 0)
{
perror_msg("can't open problem directory '%s'", dir_name);
return_InvalidProblemDir_error(invocation, dir_name);
return;
}

if (!fdump_dir_accessible_by_uid(dir_fd, caller_uid))
{
if (errno == ENOTDIR)
{
log_notice("Requested directory does not exist '%s'", dir_name);
close(dir_fd);
continue;
}

if (polkit_check_authorization_dname(caller, "org.freedesktop.problems.getall") != PolkitYes)
{ // if user didn't provide correct credentials, just move to the next dir
close(dir_fd);
continue;
}
}
delete_dump_dir(dir_name);

struct dump_dir *dd = dd_fdopendir(dir_fd, dir_name, /*flags:*/ 0);
if (dd)
{
if (dd_delete(dd) != 0)
{
error_msg("Failed to delete problem directory '%s'", dir_name);
dd_close(dd);
}
}
}

g_dbus_method_invocation_return_value(invocation, NULL);
Expand Down
15 changes: 13 additions & 2 deletions src/lib/problem_api.c
Expand Up @@ -46,22 +46,33 @@ int for_each_problem_in_dir(const char *path,
continue; /* skip "." and ".." */

char *full_name = concat_path_file(path, dent->d_name);
if (caller_uid == -1 || dump_dir_accessible_by_uid(full_name, caller_uid))

int dir_fd = dd_openfd(full_name);
if (dir_fd < 0)
{
VERB2 perror_msg("can't open problem directory '%s'", full_name);
continue;
}

if (caller_uid == -1 || fdump_dir_accessible_by_uid(dir_fd, caller_uid))
{
/* Silently ignore *any* errors, not only EACCES.
* We saw "lock file is locked by process PID" error
* when we raced with wizard.
*/
int sv_logmode = logmode;
logmode = 0;
struct dump_dir *dd = dd_opendir(full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK);
struct dump_dir *dd = dd_fdopendir(dir_fd, full_name, DD_OPEN_READONLY | DD_FAIL_QUIETLY_EACCES | DD_DONT_WAIT_FOR_LOCK);
logmode = sv_logmode;
if (dd)
{
brk = callback ? callback(dd, arg) : 0;
dd_close(dd);
}
}
else
close(dir_fd);

free(full_name);
if (brk)
break;
Expand Down

0 comments on commit 7814554

Please sign in to comment.