Protect against check-and-open race conditions

There is a small window between running lstat() on a filename and
opening it where it's possible for the file to have been modified.
We were protecting against this by saving the stat data from the
original file and verifying that it was the same file (by device
and inode) when we opened it again, but this is an imperfect
solution, as it is still possible for an attacker to modify the
permissions during this window.

It is much better to simply open the file and test on the active
file descriptor.

Resolves https://fedorahosted.org/sssd/ticket/425 incidentally, as
without the initial lstat, we are implicitly accepting symlinks
and only verifying the target file.

1 files changed, 48 insertions, 28 deletions

diff --git a/src/util/check_and_open.c b/src/util/check_and_open.c
index d010670b..db926f10 100644
--- a/src/util/check_and_open.c
+++ b/src/util/check_and_open.c
@@ -29,6 +29,10 @@
 
 #include "util/util.h"
 
+static errno_t perform_checks(struct stat *stat_buf,
+                              const int uid, const int gid,
+                              const int mode, enum check_file_type type);
+
 errno_t check_file(const char *filename, const int uid, const int gid,
                    const int mode, enum check_file_type type,
                    struct stat *caller_stat_buf)
@@ -36,7 +40,6 @@ errno_t check_file(const char *filename, const int uid, const int gid,
     int ret;
     struct stat local_stat_buf;
     struct stat *stat_buf;
-    bool type_check;
 
     if (caller_stat_buf == NULL) {
         stat_buf = &local_stat_buf;
@@ -51,6 +54,39 @@ errno_t check_file(const char *filename, const int uid, const int gid,
         return errno;
     }
 
+    return perform_checks(stat_buf, uid, gid, mode, type);
+}
+
+errno_t check_fd(int fd, const int uid, const int gid,
+                 const int mode, enum check_file_type type,
+                 struct stat *caller_stat_buf)
+{
+    int ret;
+    struct stat local_stat_buf;
+    struct stat *stat_buf;
+
+    if (caller_stat_buf == NULL) {
+        stat_buf = &local_stat_buf;
+    } else {
+        stat_buf = caller_stat_buf;
+    }
+
+    ret = fstat(fd, stat_buf);
+    if (ret == -1) {
+        DEBUG(1, ("fstat for [%d] failed: [%d][%s].\n", fd, errno,
+                                                        strerror(errno)));
+        return errno;
+    }
+
+    return perform_checks(stat_buf, uid, gid, mode, type);
+}
+
+static errno_t perform_checks(struct stat *stat_buf,
+                              const int uid, const int gid,
+                              const int mode, enum check_file_type type)
+{
+    bool type_check;
+
     switch (type) {
         case CHECK_DONT_CHECK_FILE_TYPE:
             type_check = true;
@@ -77,28 +113,28 @@ errno_t check_file(const char *filename, const int uid, const int gid,
             type_check = S_ISSOCK(stat_buf->st_mode);
             break;
         default:
-            DEBUG(1, ("Unsupprted file type.\n"));
+            DEBUG(1, ("Unsupported file type.\n"));
             return EINVAL;
     }
 
     if (!type_check) {
-        DEBUG(1, ("File [%s] is not the right type.\n", filename));
+        DEBUG(1, ("File is not the right type.\n"));
         return EINVAL;
     }
 
     if (mode >= 0 && (stat_buf->st_mode & ~S_IFMT) != mode) {
-        DEBUG(1, ("File [%s] has the wrong mode [%.7o], expected [%.7o].\n",
-                  filename, (stat_buf->st_mode & ~S_IFMT), mode));
+        DEBUG(1, ("File has the wrong mode [%.7o], expected [%.7o].\n",
+                  (stat_buf->st_mode & ~S_IFMT), mode));
         return EINVAL;
     }
 
     if (uid >= 0 && stat_buf->st_uid != uid) {
-        DEBUG(1, ("File [%s] must be owned by uid [%d].\n", filename, uid));
+        DEBUG(1, ("File must be owned by uid [%d].\n", uid));
         return EINVAL;
     }
 
     if (gid >= 0 && stat_buf->st_gid != gid) {
-        DEBUG(1, ("File [%s] must be owned by gid [%d].\n", filename, gid));
+        DEBUG(1, ("File must be owned by gid [%d].\n", gid));
         return EINVAL;
     }
 
@@ -111,36 +147,20 @@ errno_t check_and_open_readonly(const char *filename, int *fd, const uid_t uid,
 {
     int ret;
     struct stat stat_buf;
-    struct stat fd_stat_buf;
-
-    *fd = -1;
-
-    ret = check_file(filename, uid, gid, mode, type, &stat_buf);
-    if (ret != EOK) {
-        DEBUG(1, ("check_file failed.\n"));
-        return ret;
-    }
 
     *fd = open(filename, O_RDONLY);
     if (*fd == -1) {
         DEBUG(1, ("open [%s] failed: [%d][%s].\n", filename, errno,
-                                                        strerror(errno)));
+                  strerror(errno)));
         return errno;
     }
 
-    ret = fstat(*fd, &fd_stat_buf);
-    if (ret == -1) {
-        DEBUG(1, ("fstat for [%s] failed: [%d][%s].\n", filename, errno,
-                                                        strerror(errno)));
-        return errno;
-    }
-
-    if (stat_buf.st_dev != fd_stat_buf.st_dev ||
-        stat_buf.st_ino != fd_stat_buf.st_ino) {
-        DEBUG(1, ("File [%s] was modified between lstat and open.\n", filename));
+    ret = check_fd(*fd, uid, gid, mode, type, &stat_buf);
+    if (ret != EOK) {
         close(*fd);
         *fd = -1;
-        return EIO;
+        DEBUG(1, ("check_fd failed.\n"));
+        return ret;
     }
 
     return EOK;