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.

3 files changed, 79 insertions, 30 deletions

diff --git a/src/tests/check_and_open-tests.c b/src/tests/check_and_open-tests.c
index 7ec8f3bc..e3d98868 100644
--- a/src/tests/check_and_open-tests.c
+++ b/src/tests/check_and_open-tests.c
@@ -100,11 +100,11 @@ START_TEST(test_symlink)
     ret = symlink(filename, newpath);
     fail_unless(ret == 0, "symlink failed [%d][%s]", ret, strerror(ret));
 
-    ret = check_and_open_readonly(newpath, &fd, uid, gid, mode, CHECK_REG);
+    ret = check_file(newpath, uid, gid, mode, CHECK_REG, NULL);
     unlink(newpath);
+
     fail_unless(ret == EINVAL,
                 "check_and_open_readonly succeeded on symlink");
-    fail_unless(fd == -1, "check_and_open_readonly file descriptor not -1");
 }
 END_TEST
 
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;
diff --git a/src/util/util.h b/src/util/util.h
index db8e1ac3..fae8096a 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -268,9 +268,38 @@ enum check_file_type {
     CHECK_LNK,
     CHECK_SOCK
 };
+
+/* check_file()
+ * Verify that a file has certain permissions and/or is of a certain
+ * file type. This function can be used to determine if a file is a
+ * symlink.
+ * Warning: use of this function implies a potential race condition
+ * Opening a file before or after checking it does NOT guarantee that
+ * it is still the same file. Additional checks should be performed
+ * on the caller_stat_buf to ensure that it has the same device and
+ * inode to minimize impact. Permission changes may have occurred,
+ * however.
+ */
 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);
+
+/* check_fd()
+ * Verify that an open file descriptor has certain permissions and/or
+ * is of a certain file type. This function CANNOT detect symlinks,
+ * as the file is already open and symlinks have been traversed. This
+ * is the safer way to perform file checks and should be preferred
+ * over check_file for nearly all situations.
+ */
+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);
+
+/* check_and_open_readonly()
+ * Utility function to open a file and verify that it has certain
+ * permissions and is of a certain file type. This function wraps
+ * check_fd(), and is considered race-condition safe.
+ */
 errno_t check_and_open_readonly(const char *filename, int *fd, const uid_t uid,
                                const gid_t gid, const mode_t mode,
                                enum check_file_type type);