From 16b2ebdb678b7475bacb80dd59e949055d3f856c Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Fri, 24 Apr 2015 11:42:18 +0200 Subject: [LIBREPORT PATCH] lib: add alternative dd functions accepting fds Florian Weimer : 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 --- 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