summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStephen Gallagher <sgallagh@redhat.com>2010-04-01 16:12:29 -0400
committerStephen Gallagher <sgallagh@redhat.com>2010-04-06 14:33:43 -0400
commit55a0f220ba8b35d7ea8e47ad19babdb05dd2bbe9 (patch)
tree0ad879883ea70686ca9a3caf012b29413b3315a5
parent3bd250d73e7d77cf8ceb72133ce13059c52a70ed (diff)
downloadsssd-55a0f220ba8b35d7ea8e47ad19babdb05dd2bbe9.tar.gz
sssd-55a0f220ba8b35d7ea8e47ad19babdb05dd2bbe9.tar.bz2
sssd-55a0f220ba8b35d7ea8e47ad19babdb05dd2bbe9.zip
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.
-rw-r--r--src/tests/check_and_open-tests.c4
-rw-r--r--src/util/check_and_open.c76
-rw-r--r--src/util/util.h29
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);