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