From 200edbbf40d980ad987fafa753f1c14c8778d8e1 Mon Sep 17 00:00:00 2001

From: Jakub Filak <jfilak@redhat.com>

Date: Fri, 24 Apr 2015 11:42:18 +0200

Subject: [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 |  29 +++++++++++

 src/lib/dump_dir.c     | 130 ++++++++++++++++++++++++++++++++-----------------

 2 files changed, 114 insertions(+), 45 deletions(-)

diff --git a/src/include/dump_dir.h b/src/include/dump_dir.h

index 669f7fd..b613e0b 100644

--- a/src/include/dump_dir.h

+++ b/src/include/dump_dir.h

@@ -51,6 +51,11 @@ enum {

     DD_DONT_WAIT_FOR_LOCK = (1 << 5),

     /* Create the new dump directory with parent directories (mkdir -p)*/

     DD_CREATE_PARENTS = (1 << 6),

+    /* Initializes internal data, opens file descriptors and returns the

+     * structure. This flag is useful for testing whether a directory

+     * exists and to perform stat operations.

+     */

+    DD_OPEN_FD_ONLY = (1 << 7),

 };

 struct dump_dir {

@@ -74,7 +79,21 @@ struct dump_dir {

 void dd_close(struct dump_dir *dd);

+/* Opens the given path

+ */

 struct dump_dir *dd_opendir(const char *dir, int flags);

+

+/* Re-opens a dump_dir opened with DD_OPEN_FD_ONLY.

+ *

+ * The passed dump_dir must not be used any more and the return value must be

+ * used instead.

+ *

+ * The passed flags must not contain DD_OPEN_FD_ONLY.

+ *

+ * The passed dump_dir must not be already locked.

+ */

+struct dump_dir *dd_fdopendir(struct dump_dir *dd, 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

@@ -155,6 +174,11 @@ 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);

+/* Returns the same information as dump_dir_accessible_by_uid

+ *

+ * The passed dump_dir can be opened with DD_OPEN_FD_ONLY

+ */

+int dd_accessible_by_uid(struct dump_dir *dd, uid_t uid);

 enum {

     DD_STAT_ACCESSIBLE_BY_UID = 1,

@@ -169,6 +193,11 @@ 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);

+/* Returns the same information as dump_dir_stat_for_uid

+ *

+ * The passed dump_dir can be opened with DD_OPEN_FD_ONLY

+ */

+int dd_stat_for_uid(struct dump_dir *dd, 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 7cc918a..4dcfa66 100644

--- a/src/lib/dump_dir.c

+++ b/src/lib/dump_dir.c

@@ -438,20 +438,41 @@ 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, const char *dir, int flags)

 {

-    struct dump_dir *dd = dd_init();

+    if (dir != NULL)

+    {

+        dd->dd_dirname = rm_trailing_slashes(dir);

+        /* dd_do_open validates dd_fd */

+        dd->dd_fd = open(dd->dd_dirname, O_DIRECTORY | O_NOFOLLOW);

-    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;

-    if (fstat(dd->dd_fd, &stat_buf) != 0)

-        goto cant_access;

+        struct stat stat_buf;

+        if (dd->dd_fd < 0)

+            goto cant_access;

+        if (fstat(dd->dd_fd, &stat_buf) != 0)

+            goto cant_access;

+

+        /* & 0666 should remove the executable bit */

+        dd->mode = (stat_buf.st_mode & 0666);

+

+        dd->dd_uid = (uid_t)-1L;

+        dd->dd_gid = (gid_t)-1L;

+        if (geteuid() == 0)

+        {

+            /* 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'", dd->dd_dirname);

+                dd_close(dd);

+                return NULL;

+            }

+            dd->dd_uid = stat_buf.st_uid;

+            dd->dd_gid = stat_buf.st_gid;

+        }

-    /* & 0666 should remove the executable bit */

-    dd->mode = (stat_buf.st_mode & 0666);

+        if ((flags & DD_OPEN_FD_ONLY))

+            return dd;

+    }

     errno = 0;

     if (dd_lock(dd, WAIT_FOR_OTHER_PROCESS_USLEEP, flags) < 0)

@@ -482,7 +503,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

         {

@@ -490,36 +511,41 @@ 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);

         return NULL;

     }

-    dd->dd_uid = (uid_t)-1L;

-    dd->dd_gid = (gid_t)-1L;

-    if (geteuid() == 0)

-    {

-        /* 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);

-            dd_close(dd);

-            return NULL;

-        }

-        dd->dd_uid = stat_buf.st_uid;

-        dd->dd_gid = stat_buf.st_gid;

-    }

-

     return dd;

 }

+struct dump_dir *dd_fdopendir(struct dump_dir *dd, int flags)

+{

+    if ((flags & DD_OPEN_FD_ONLY))

+        error_msg_and_die("the passed flags must not contain DD_OPEN_FD_ONLY");

+

+    if (dd->dd_fd < 0)

+        error_msg_and_die("the dump directory was not initialized yet");

+

+    if (dd->locked)

+        error_msg_and_die("the dump directory is already locked");

+

+    return dd_do_open(dd, NULL, flags);

+}

+

+struct dump_dir *dd_opendir(const char *dir, int flags)

+{

+    struct dump_dir *dd = dd_init();

+    return dd_do_open(dd, dir, 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

@@ -1384,32 +1410,22 @@ 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 dd_stat_for_uid(struct dump_dir *dd, uid_t uid)

 {

-    struct stat statbuf;

-    if (stat(dirname, &statbuf) != 0 || !S_ISDIR(statbuf.st_mode))

-    {

-        log_debug("can't get stat of '%s': not a problem directory", dirname);

-        errno = ENOTDIR;

-        return -1;

-    }

-

-    errno = 0;

-

     int ddstat = 0;

-    if (uid == 0 || (statbuf.st_mode & S_IROTH))

+    if (uid == 0 || (dd->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;

     }

 #if DUMP_DIR_OWNED_BY_USER > 0

-    if (uid == statbuf.st_uid)

+    if (uid == dd->dd_uid)

 #else

-    if (uid_in_group(uid, statbuf.st_gid))

+    if (uid_in_group(uid, dd->dd_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;

     }

@@ -1417,6 +1433,30 @@ 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)

+{

+    struct dump_dir *dd = dd_opendir(dirname, DD_OPEN_FD_ONLY);

+    if (dd == NULL)

+        return -1;

+

+    int r = dd_stat_for_uid(dd, uid);

+    dd_close(dd);

+

+    return r;

+}

+

+int dd_accessible_by_uid(struct dump_dir *dd, uid_t uid)

+{

+    int ddstat = dd_stat_for_uid(dd, 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);

-- 

2.1.0