From 16b2ebdb678b7475bacb80dd59e949055d3f856c Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
Date: Fri, 24 Apr 2015 11:42:18 +0200
Subject: [LIBREPORT PATCH] lib: add alternative dd functions accepting fds
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(). At least re-checking after
dd_opendir() with the stored file descriptor is needed.
Related: #1214745
Signed-off-by: Jakub Filak <jfilak@redhat.com>
---
src/include/dump_dir.h | 8 +++++
src/lib/dump_dir.c | 82 +++++++++++++++++++++++++++++++++++++++++---------
2 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/src/include/dump_dir.h b/src/include/dump_dir.h
index d675362..07b119a 100644
--- a/src/include/dump_dir.h
+++ b/src/include/dump_dir.h
@@ -68,7 +68,13 @@ struct dump_dir {
void dd_close(struct dump_dir *dd);
+/* Opens the given path and returns the resulting file descriptor.
+ */
+int dd_openfd(const char *dir);
struct dump_dir *dd_opendir(const char *dir, int flags);
+/* Skips dd_openfd(dir) and uses the given file descriptor instead
+ */
+struct dump_dir *dd_fdopendir(int dir_fd, const char *dir, int flags);
struct dump_dir *dd_create_skeleton(const char *dir, uid_t uid, mode_t mode, int flags);
int dd_reset_ownership(struct dump_dir *dd);
/* Pass uid = (uid_t)-1L to disable chown'ing of newly created files
@@ -147,6 +153,7 @@ void delete_dump_dir(const char *dirname);
* Returns non zero if dump dir is accessible otherwise return 0 value.
*/
int dump_dir_accessible_by_uid(const char *dirname, uid_t uid);
+int fdump_dir_accessible_by_uid(int dir_fd, uid_t uid);
enum {
DD_STAT_ACCESSIBLE_BY_UID = 1,
@@ -161,6 +168,7 @@ enum {
* Returns negative number if error occurred otherwise returns 0 or positive number.
*/
int dump_dir_stat_for_uid(const char *dirname, uid_t uid);
+int fdump_dir_stat_for_uid(int dir_fd, uid_t uid);
/* creates not_reportable file in the problem directory and saves the
reason to it, which prevents libreport from reporting the problem
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index 16cd987..a0e96e4 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -417,12 +417,8 @@ static char* rm_trailing_slashes(const char *dir)
return xstrndup(dir, len);
}
-struct dump_dir *dd_opendir(const char *dir, int flags)
+static struct dump_dir *dd_do_open(struct dump_dir *dd, int flags)
{
- struct dump_dir *dd = dd_init();
-
- dir = dd->dd_dirname = rm_trailing_slashes(dir);
- dd->dd_fd = open(dir, O_DIRECTORY | O_NOFOLLOW);
struct stat stat_buf;
if (dd->dd_fd < 0)
goto cant_access;
@@ -440,6 +436,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
/* Directory is not writable. If it seems to be readable,
* return "read only" dd, not NULL
*
+ *
* Does the directory have 'x' flag?
*/
if (faccessat(dd->dd_fd, ".", R_OK, AT_SYMLINK_NOFOLLOW) == 0)
@@ -461,7 +458,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
* directory when run without arguments, because its option -d DIR
* defaults to "."!
*/
- error_msg("'%s' is not a problem directory", dir);
+ error_msg("'%s' is not a problem directory", dd->dd_dirname);
}
else
{
@@ -469,12 +466,12 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
if (errno == ENOENT || errno == ENOTDIR)
{
if (!(flags & DD_FAIL_QUIETLY_ENOENT))
- error_msg("'%s' does not exist", dir);
+ error_msg("'%s' does not exist", dd->dd_dirname);
}
else
{
if (!(flags & DD_FAIL_QUIETLY_EACCES))
- perror_msg("Can't access '%s'", dir);
+ perror_msg("Can't access '%s'", dd->dd_dirname);
}
}
dd_close(dd);
@@ -488,7 +485,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
/* In case caller would want to create more files, he'll need uid:gid */
if (fstat(dd->dd_fd, &stat_buf) != 0)
{
- error_msg("Can't stat '%s'", dir);
+ error_msg("Can't stat '%s'", dd->dd_dirname);
dd_close(dd);
return NULL;
}
@@ -499,6 +496,34 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
return dd;
}
+int dd_openfd(const char *dir)
+{
+ return open(dir, O_DIRECTORY | O_NOFOLLOW);
+}
+
+struct dump_dir *dd_fdopendir(int dir_fd, const char *dir, int flags)
+{
+ struct dump_dir *dd = dd_init();
+
+ dd->dd_dirname = rm_trailing_slashes(dir);
+ dd->dd_fd = dir_fd;
+
+ /* Do not interpret errno in dd_do_open() if dd_fd < 0 */
+ errno = 0;
+ return dd_do_open(dd, flags);
+}
+
+struct dump_dir *dd_opendir(const char *dir, int flags)
+{
+ struct dump_dir *dd = dd_init();
+
+ dd->dd_dirname = rm_trailing_slashes(dir);
+ /* dd_do_open validates dd_fd */
+ dd->dd_fd = dd_openfd(dir);
+
+ return dd_do_open(dd, flags);
+}
+
/* Create a fresh empty debug dump dir which is owned bu the calling user. If
* you want to create the directory with meaningful ownership you should
* consider using dd_create() function or you can modify the ownership
@@ -936,7 +961,7 @@ int dd_chown(struct dump_dir *dd, uid_t new_uid)
error_msg_and_die("dump_dir is not opened"); /* bug */
struct stat statbuf;
- if (!(stat(dd->dd_dirname, &statbuf) == 0 && S_ISDIR(statbuf.st_mode)))
+ if (!fstat(dd->dd_fd, &statbuf) == 0)
{
perror_msg("stat('%s')", dd->dd_dirname);
return 1;
@@ -1352,12 +1377,12 @@ static bool uid_in_group(uid_t uid, gid_t gid)
}
#endif
-int dump_dir_stat_for_uid(const char *dirname, uid_t uid)
+int fdump_dir_stat_for_uid(int dir_fd, uid_t uid)
{
struct stat statbuf;
- if (stat(dirname, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
+ if (fstat(dir_fd, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))
{
- log_debug("can't get stat of '%s': not a problem directory", dirname);
+ log_debug("can't get stat: not a problem directory");
errno = ENOTDIR;
return -1;
}
@@ -1367,7 +1392,7 @@ int dump_dir_stat_for_uid(const char *dirname, uid_t uid)
int ddstat = 0;
if (uid == 0 || (statbuf.st_mode & S_IROTH))
{
- log_debug("directory '%s' is accessible by %ld uid", dirname, (long)uid);
+ log_debug("directory is accessible by %ld uid", (long)uid);
ddstat |= DD_STAT_ACCESSIBLE_BY_UID;
}
@@ -1377,7 +1402,7 @@ int dump_dir_stat_for_uid(const char *dirname, uid_t uid)
if (uid_in_group(uid, statbuf.st_gid))
#endif
{
- log_debug("%ld uid owns directory '%s'", (long)uid, dirname);
+ log_debug("%ld uid owns directory", (long)uid);
ddstat |= DD_STAT_ACCESSIBLE_BY_UID;
ddstat |= DD_STAT_OWNED_BY_UID;
}
@@ -1385,6 +1410,33 @@ int dump_dir_stat_for_uid(const char *dirname, uid_t uid)
return ddstat;
}
+int dump_dir_stat_for_uid(const char *dirname, uid_t uid)
+{
+ int dir_fd = open(dirname, O_DIRECTORY | O_NOFOLLOW);
+ if (dir_fd < 0)
+ {
+ log_debug("can't open '%s': not a problem directory", dirname);
+ errno = ENOTDIR;
+ return -1;
+ }
+
+ int r = fdump_dir_stat_for_uid(dir_fd, uid);
+ close(dir_fd);
+ return r;
+}
+
+int fdump_dir_accessible_by_uid(int dir_fd, uid_t uid)
+{
+ int ddstat = fdump_dir_stat_for_uid(dir_fd, uid);
+
+ if (ddstat >= 0)
+ return ddstat & DD_STAT_ACCESSIBLE_BY_UID;
+
+ VERB3 pwarn_msg("can't determine accessibility for %ld uid", (long)uid);
+
+ return 0;
+}
+
int dump_dir_accessible_by_uid(const char *dirname, uid_t uid)
{
int ddstat = dump_dir_stat_for_uid(dirname, uid);
--
1.8.3.1